sync option doesnt work with track_changes

Sounds like the help/docs for sync should include this limitation.

Sounds like the help/docs for sync should include this limitation.

Do you mean this limitation cannot be removed? If so, can you please explain why?

Self-answer: it's seems difficult to remove this limitation without some change to the migrate module (core), because:
* we need the source ids to be able to compare with previously imported,
* for that, we have to loop on the source,
* loops on the source call SourcePluginBase::next(),
* SourcePluginBase::next() only give rows that are new, flagged as needing update, have an updated highwater mark, or changed.

So it might be possible to do something not too hacky, but it would need changes to core. If I can't do without this feature (sync+track_changes), I may create a core issue/patch.

I slept on it and now I see that I headed to the same approach as what was first being thought in #2809433: Migrate support for deleting items no longer in the incoming data. It adds a check in SourcePluginBase::next() to be able to loop over all the source rows (allRows mode).

But as it was decided that the core would not change (#2809433-88: Migrate support for deleting items no longer in the incoming data), @quietone tried to move the existing core patch to migrate_tools: #2809433-95: Migrate support for deleting items no longer in the incoming data.
It initially failed because of the core next() problem I explained above:
"Not working yet. failing to get any rows from the map."
"Your source plugin isn't returning rows because the rows are already imported, and source plugin base filters out rows that have already been processed by default."
"An idea from #2809433-81 was missing: the "all rows" source configuration option and the related changes over next(); that is the reason I think we cannot work-around sources not implementing the SyncableSourceInterface interface"

This was the start of two new approachs, discussed somewhat in parallel which makes it hard to read:
A) @Marvil07 suggested to extend SourcePluginBase to override the next() method: #2809433-110: Migrate support for deleting items no longer in the incoming data. It meant that all existing source sub-classes needed to be extended.
B) @mikelutz suggested the prepareUpdate() workaround (#2809433-109: Migrate support for deleting items no longer in the incoming data). He understood it would be a problem to keep all the id map rows in "needs update" status (and indeed, this is what breaks track_changes). So he suggested: "at the end resave all the idmap rows to reset their status to imported."
@heddn was more into this approach: "Can we get the super simple solution here addressed first?"
@quietone said: "I am not convinced we should set the row status to imported after this rollbackmissing operation since that would also change failed and skip status to imported." which is why he helped on approach A.

Then, people mainly worked on A, even if @heddn (migrate_tools maintainer) wanted B (at least as a first step): #2809433-160: Migrate support for deleting items no longer in the incoming data, #3067311: [meeting] Migrate Meeting 2019-07-11. So, @heddn published a patch for B: #2809433-181: Migrate support for deleting items no longer in the incoming data, and finally B was committed.

But: the problem mentioned at the very start of B (how to keep the id map rows statuses unchanged at the end of the loop?) remains (not to mention performance concerns), so that track_changes cannot work!

Now, I'm wondering how we could fix that, as I agree with @quietone (in italic above).

I think I found a better workaround than prepareUpdate(): use hook_migrate_prepare_row(). Here's a POC patch that seems to work well for my needs. I can now use track_changes normally. :)

1. +++ b/migrate_tools.module
   @@ -56,3 +56,23 @@ function migrate_tools_migration_plugins_alter(array &$migrations) {
   +    $existing_ids = &drupal_static($static_id);
   

   Nice! Instead of using drupal_static, could we use state instead? I think that would handle any batch resets or memory clean-up that might happen.

2. Some phpcs nits found on the testbot:

   migrate_tools.module ✗ 3 more
   line 66	Namespaced classes/interfaces/traits should be referenced with use statements
   66	Namespaced classes/interfaces/traits should be referenced with use statements
   66	Namespaced classes/interfaces/traits should be referenced with use statements

3. Needs tests, so tagging.

This bit me too.

@GaëlG: A wholehearted "Wow!💪" for your research and work in #8.

Thank you @GaëlG. I was stuck using the old patch on #2809433: Migrate support for deleting items no longer in the incoming data until I found your patch here and was able to upgrade the module.

It needed a re-roll for 8.x-5.x. Here it is.

The great gitlab migration is upon us. See https://gitlab.com/drupalspoons/migrate_tools/-/issues/94. The latest patch from #12 has been uploaded to https://gitlab.com/drupalspoons/migrate_tools/-/merge_requests/4

#9 solved issue for us! Thanks for working on this!

#12 solved the issue for me. It's keeping the "track_changes: true"property while using --sync parameter.
The rows are getting updated only if there is update in the source data and if the rows are not available then the content is getting rollback.

Thanks

The patch in #12 works for me as well, and it also resolves #3104268: Sync is too strict during id comparison and can roll back everything. I'm reopening this as "Needs work" as there are still some items from #10 to address. I assume this was only closed due to the detour to DrupalSpoons.

The patch in #12 gave the behaviour I was expecting, rolling back the things that were deleted on the source site. Thanks!

#12 works for us as well, but source id (ids) needs to be a string. This didn't work for us if the ids was an integer.

I've re-rolled the original patch for 6.0.x including the improvements by @csmdgl and @PCateNIH in GitLab.

I've updated the tests to run with and without the --update flag set. The behaviour of --sync with --update is identical to the default behaviour without this patch. So this is technically breaking BC or improving import performance, depending on how you see it.

Tests run too fast to capture the full process output. I had to remove one assert.

I'm not sure I like the use of a static public property on MigrationImportSync. Could we use some other temp store instead to pass around data? It just seems brittle and icky. Still thinking what a better approach might look like.

I think we can use \Drupal::state()->get/set('migrate_tools_sync'), where we key the values by the migration name. And the first thing we do when executing a sync is to reset the values. That lets us store/retrieve the values and have them reset when syncing the migration again. And it gets away from the more ugly feeling of using public statics.

In my case the patch #22 don't work, Everything is rollback and everything is reimported.

After some research i found i need to combine this patch with the logic implemented in this patch #4

But patch #4 work only with version 8.5.x so i make this patch to combine the both in one.

Subscribe

The patch in #32 doesn't work for string IDs, such as UUIDs.
It currently parses the source IDs using intval, which causes UUIDs like this "48b79c41-d97c-4076-8e2d-cbb6c0357b7b" to get converted to "48".

Calling either strval or intval depending on the "type" defined in getIds() seems like a more robust solution.

I have created a new patch that works for both integer and string source IDs.

Uploaded new patch. Is same as #34 but replaces the static $source_id_values public property with the State API per #25.

Re-roll of #35 patch to work with latest dev version.

I confirm the latest patches works ...

Note: #35 applies to. latest stable releases - 6.0.0.

Minor feedback, will fix on commit.

1. +++ b/src/EventSubscriber/MigrationImportSync.php
   @@ -48,21 +60,37 @@ class MigrationImportSync implements EventSubscriberInterface {
   +            $map_source_id[$id_key] = strval($map_source_id[$id_key]);
   

   $map_source_id[$id_key] = (string) $map_source_id[$id_key];

2. +++ b/src/EventSubscriber/MigrationImportSync.php
   @@ -48,21 +60,37 @@ class MigrationImportSync implements EventSubscriberInterface {
   +            $map_source_id[$id_key] = intval($map_source_id[$id_key]);
   

   $map_source_id[$id_key] = (int) ($map_source_id[$id_key];

I do not understand what this path is supposed to do?

I installed #35 on 6.0.0.

I expected that when changing a value of an item (in my case an xml item) it would be updated when using the sync option.

<item>
<title>NEW TITLE</title>
</item>

but this is not the case.

Is that not what the patch is supposed to do ?

Thanks for clarifying