Migrations fail due to missing dependency when dependency has skipped rows by the source plugin

I have reviewed patches in commets #61 and #70.

If I understand things properly, #61 would be an API break. Per \Drupal\migrate\Plugin\MigrateSourceInterface::prepareRow, we allow returning FALSE to signal the row should be skipped. I tested this on a recent project. The error about skipped rows in dependent disappears, but that is because the rows where imported when they should have not. I do not have a Core example handy, but the ParagraphsItem source plugin relies on skipping on FALSE to intentionally prevent migrating paragraph entries that are no longer in use.

Patch #70 would mark the row as skipped effectively solving the issue. In this patch, this is a byproduct of calling throw new MigrateSkipRowException('Row skipped by source plugin.');. That is, MigrateSkipRowException signals that the row should be saved to the map. The actual saving is performed by MigrateExecutable in the import method. Namely, in this block of code:

        try {
          foreach ($pipeline as $destination_property_name => $plugins) {
            $this->processPipeline($row, $destination_property_name, $plugins, NULL);
          }
          $save = TRUE;
        }
        catch (MigrateException $e) {
          ...
        }
        catch (MigrateSkipRowException $e) {
          if ($e->getSaveToMap()) {
            $id_map->saveIdMapping($row, [], MigrateIdMapInterface::STATUS_IGNORED);
          }
          ...
        }

Throwing the exception probably suffices. I do not understand why the proposed changes to \Drupal\migrate\MigrateExecutable::processPipeline and \Drupal\migrate\Row are necessary. I propose a slightly different approach. Save to the map directly in \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::next Optionally, we can also include a message there. I have attached a patch with both, but only is saving to the map is strictly necessary.

This topic was discussed in this Slack thread some time ago. For references, copying part of the conversation below:

So, prepareRow row in my custom source plugin returned FALSE and the records was skipped. But, it failed to do something that \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::prepareRow does. In the method implementation for SourcePluginBase , which is where the migrate_prepare_row and migrate_MIGRATION_ID_prepare_row hooks are fired. If a record is marked to be skipped, it is saved to the idMap.

While there is a boolean to determine if the record should be saved or not ($save_to_map), saving to the map makes sure the record is accounted for when the migrate API checks if all records have been processed for a migration. This check is also performed in \Drupal\migrate\Plugin\Migration::checkRequirements So, if a migration dependency is set as required, it has to have all rows processed. A tangential note, but worth mentioning.

So, the solution was manually saving to the idMap in my custom source process' prepareRow method. This fixed the problem for me.

Writing to the idMap is accounted for by \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::prepareRow when the result comes from an implementation of the migrate_prepare_row and migrate_MIGRATION_ID_prepare_row hooks. But if you implement the method in your source plugin, you need to write to the map yourself.

The original issue asks for improving the documentation \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::prepareRow If we make this change in \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::next we keep BC. If we only improve the API, we would have to rely on developers to manually save to the map if they implement \Drupal\migrate\Plugin\MigrateSourceInterface::prepareRow in their custom source plugins.

I have updated the issue summary. Also changing state to Needs review for feedback on the proposed patch. No interdiff against previous patches as the approach is somewhat different.

This is affecting 🐛 Paragraph Migrations broken by archived flag RTBC

The last submitted patch, 74: 2797505-74.patch, failed testing. View results →

#74 helps a little but it's not enough.
For some unknown reason, i can't have all of my items.

So i found another workaround.
Put migrations in the same group, add an index before id (don't forget to rename tables to keep migration track) and launch import on group.

Btw, #70 have a good point.
By adding some intels to row, we may add reason for ignore.
Because #74 does not do that and it's pretty annoying.

Just ran into this once again. After trying to migrate a site using media_migration enabled and a dirty workaround to get it to work with D11 ( 🐛 Drupal 7 to Drupal 11 migration runs forever Active , #14), I tried the latest 3 patches provided here, but nothing seemed to work.

I ended up simply manually removing the failing dependencies from the migration ymls. Luckily, they still migrated in the correct order, and the files migrated to media as expected. I think this is how I fixed the issue last time I ran into this.

I hope this might help someone.

In general, I like the approach discussed in #74. It might not solve all the problems, but it does solve a single problem and should help with some people's issues. Let's roll it into an MR and add tests.

Patch in #74 to MR: https://git.drupalcode.org/project/drupal/-/merge_requests/10925/diffs

MR!10925 Has PHPUnit test failures that seem related.

See: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/migra...
See: https://git.drupalcode.org/issue/drupal-2797505/-/jobs/4043797

---- Drupal\Tests\migrate\Kernel\MigrateSkipRowTest ----
Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Fail      Other      phpunit-261.xml      0 Drupal\Tests\migrate\Kernel\Migrate
    PHPUnit Test failed to complete; Error: PHPUnit 10.5.38 by Sebastian Bergmann and contributors.
    
    Runtime:       PHP 8.3.15
    Configuration: /builds/issue/drupal-2797505/core/phpunit.xml.dist
    
    F                                                                   1 / 1 (100%)
    
    Time: 00:01.316, Memory: 8.00 MB
    
    Migrate Skip Row (Drupal\Tests\migrate\Kernel\MigrateSkipRow)
     ✘ Prepare row skip
       ┐
       ├ Failed asserting that an array is empty.
       │
       │ /builds/issue/drupal-2797505/core/modules/migrate/tests/src/Kernel/MigrateSkipRowTest.php:63
       ┴
    
    FAILURES!
    Tests: 1, Assertions: 3, Failures: 1.

And functional tests
See: https://git.drupalcode.org/issue/drupal-2797505/-/jobs/4043788
See: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/migra...
See: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/migra...

---- Drupal\Tests\migrate_drupal_ui\Functional\d6\Upgrade6Test ----
Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Fail      Other      phpunit-3.xml        0 Drupal\Tests\migrate_drupal_ui\Func
    PHPUnit Test failed to complete; Error: PHPUnit 10.5.38 by Sebastian Bergmann and contributors.
    
    Runtime:       PHP 8.3.15
    Configuration: /builds/issue/drupal-2797505/core/phpunit.xml.dist
    
    F                                                                   1 / 1 (100%)
    
    
    
    Time: 02:35.228, Memory: 59.34 MB
    
    Upgrade6 (Drupal\Tests\migrate_drupal_ui\Functional\d6\Upgrade6)
     ✘ Upgrade and incremental
       ┐
       ├ Failed asserting that 56 is identical to 39.
       │
       │ /builds/issue/drupal-2797505/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeExecuteTestBase.php:157
       │ /builds/issue/drupal-2797505/core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6Test.php:211
       ┴
    
    FAILURES!
    Tests: 1, Assertions: 4223, Failures: 1.

#74 🐛 Migrations fail due to missing dependency when dependency has skipped rows by the source plugin Needs work worked for me. It allowed for archived paragraphs to be ignored and the migration to continue.

So the MR (#74 approach) works and solve this kind of issues.

However, I did pushed some updates for fixing the tests ... And those updates indicates the solution do not seem right or complete, at least.
interfering with exception MigrateSkipRowException->saveToMap

Then I reverted those changes and "extended" SourcePluginBase with a new property to be aware of saveToMap and to have control for "non-saving cases".

Now we should add proper tests for this issue/solution cases.
Of course if this trick would be considered "OK".

Temporary on Needs Review

it seems there were some tests in an issue patch by @quietone (#59)

And I applied (partially) and cleaned up ... the hook removed as it has already tests + I don't think the hooks should return "values".

However, now it seems the MR has a failure which I don't think it's related.
And I can't replicate locally.

@vasike, some tests fail randomly, so it's common practice to manually re-run those that fail. Most of the time, they pass on the second attempt.

In this case, the test kept failing, and I remembered having the same error with a CSS stylesheet larger than 90kB in another issue. I think I rebased to make it work, so I rebased the issue fork branch onto the latest 11.x commit. Now it passes 🙂 (I've had to re-run Nightwatch tests, which have failed on the first attempt)

Left some comments on the MR.

MR threads addressed ... so back to review.