track_changes not working when using the --sync flag for migrate:import

@KenowaX, thanks for the report and the patch. Can you upload the yml for the migration where track_changes wasn't working for you. Thanks.

@KenowaX, thanks. I might be able to have a closer look on the weekend.

I did some testing with the ud_migrations_csv_source module from https://github.com/dinarcon/ud_migrations The track_changes works as expected with CSV files. Records are updated only when the source data changes. From looking at the code in Drupal core and Migrate Tools, the track_changes configuration for source plugins and the --sync flag for the migrate:import Drush commands are not compatible. Or at least it will not provide the intended behavior which I understand to be only update records that have changed in the source CSV.

As @huzooka pointed out in slack, the place to look at is \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::next. Specifically this check:

      // Check whether the row needs processing.
      // 1. This row has not been imported yet.
      // 2. Explicitly set to update.
      // 3. The row is newer than the current highwater mark.
      // 4. If no such property exists then try by checking the hash of the row.
      if (!$row->getIdMap() || $row->needsUpdate() || $this->aboveHighwater($row) || $this->rowChanged($row)) {
        $this->currentRow = $row->freezeSource();
      }

When the --sync flag is not used, the $this->rowChanged($row) is reached. This would call \Drupal\migrate\Row::changed and compared the original hash against the current hash from the source. Things would work as expected in this case.

The incompatibility comes from setting the --sync flag. In \Drupal\migrate_tools\EventSubscriber\MigrationImportSync::sync there is a call to $id_map->prepareUpdate();. The implementation at \Drupal\migrate\Plugin\migrate\id_map\Sql::prepareUpdate changes the source_row_status for all source records to MigrateIdMapInterface::STATUS_NEEDS_UPDATE. So, when the check mentioned above is reached and $row->needsUpdate() is evaluated, the records will always be marked as needed to be imported. That is because \Drupal\migrate\Row::needsUpdate checks if $this->idMap['source_row_status'] == MigrateIdMapInterface::STATUS_NEEDS_UPDATE. That will always be TRUE when using the --sync. So, the condition returns early and $this->rowChanged($row) never gets evaluated.

I do not think this is a bug in Drupal core. It looks like a documentation issue. That is, the --sync flag for migrate:import is not compatible with track_changes if you only want to limit the import operations to values that changed in the source.

A few things I found interesting. The entity changed keeps the same value even if the report from migrate:import says that records have been updated. Also, \Drupal\migrate_tools\EventSubscriber\MigrationImportSync::sync goes over all source records to determine if something needs to be deleted before \Drupal\migrate\MigrateExecutable::import is reached.

For the record, I tested with:

• Drupal core 9.1.8
• Drush 10.3.6
• Migrate Tools 8.x-5.0
• Migrate Source CSV 8.x-3.4

Update: I wonder if this is related to CSV migrations at all. The way I understand it, this would be an issue with any source plugin. The "conflicting" settings are track_changes from SourcePluginBase and the --sync flag from Migrate Tools' migrate:import command. I do not see how the CSV source plugin plays a role there.

@dinarcon, thank you for taking the time to look into this!

The incompatibility comes from setting the --sync flag. In \Drupal\migrate_tools\EventSubscriber\MigrationImportSync::sync there is a call to $id_map->prepareUpdate();

Ah, that explains it!

I've updated the IS with this information.

Now what next. I think this should be documented somewhere. How about at https://git.drupalcode.org/project/migrate_tools/-/blob/8.x-4.x/src/Comm.... Moving to Migrate Tools and attaching a patch.

Any other place to document?

@quiteone thanks for the patch. A nit: It is missing a quote after 'track_changes. I think this is a good place to place to make note of the incompatibility.

Also, is it possible to throw an error when --sync is used with a migration that uses track_changes? An error in the console right away would make it easier to spot the incompatibility. Something like the way \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::__construct enforces high_water_property and track_changes cannot be used at the same time.

Update: Drush 10.4+ offers the --delete flag. I have not checked the implementation, but this is another place where documentation updates might be required.

Updating the issue title and summary.

Updating issue summary.

Changing to NR for the patch in #9

re #10 --delete flag,

I don't think the delete flag does anything. Or it sure doesn't look like it.

NW for #10:

Something like the way \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::__construct enforces high_water_property and track_changes cannot be used at the same time.

They could be separated, but then we'd have BC implications.

It may be a duplicate of #3104105: sync option doesnt work with track_changes ?

I know the current issue is about track_changes, but I'm seeing the same behavior with high_water_property.

# drush mim my_node_migration --sync
 [notice] Rolled back 1 item - done with 'my_node_migration'
 [notice] Processed 197 items (0 created, 197 updated, 0 failed, 0 ignored) - done with 'my_node_migration'

The migration runs twice.

[update]: In Drupal Slack # migration, @mikelutz says this is a «Works as designed» since --sync flags all rows as needing an update so it can process them and see if they need to be removed.

Isn't this rather a duplicate of https://www.drupal.org/project/migrate_tools/issues/3104105 than "works as designed"?