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.