track_changes does not work when the map is joinable

Rerolling on top of 8.2.x

I can confirm the patch above works.

Updated the patch #3 for version 8.5.1.

Back to needs work for those tests, per #10.

+++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
@@ -266,18 +266,18 @@ protected function initializeIterator() {
+      // 1. If the map is joinable, join it (unless track_changes is set). We will
+      //    want to accept all rows which are either not in the map, or marked in

Also, the comment is > 80 characters. And it would be better if the comment explicitly states what happens when track_changes is set instead of somewhat burying it in a phrase in parenthesis.

I ran into this problem today and I confirm that the patch in #9 fixes this. I am raising the priority here because this problem essentially breaks the D7 -> D8 migration when using track_changes to update changed content. Without this patch none of the changes to fields on the content type were detected as changes. So, before the patch I had:

drush mim my_articles
Processed 0 items (0 created, 0 updated, 0 failed, 0 ignored) - done with 'my_articles'

But after the patch I got:

drush mim my_articles
Processed 13 items (0 created, 13 updated, 0 failed, 0 ignored) - done with 'my_articles'

As far as I can tell, this affects all D7-> D8 migrations that need to track changes for incremental migrations...only new content gets created.

Updated the patch #3 for version 8.6.2.

Testing on 8.6.x

@lemuelsantos I don't see any difference between your patch (#15) and the previous one (#9). Still needs test.

Attached a new version of #9 (15 is identical) with tests.

Removing hierarchy from test, as is not relevant. And test against 8.7

Resetting Version to 8.8

Adding missing comma for code style.

These tests look really good. I'm uploading a patch with just the test now, and I'm going to upload a tweak to the fix to improve readability shortly.

New fix. We have logic for determining joinability spread into the constructor, the iterator initialization and the dedicated mapJoinable() method. This moves all the joinability logic into the mapJoinable() method for clarity.

I've retest a migration with track_changes: true, without the patch changes in the source are ignored, with the patch changes in the source are picked up and migrations are updated.

Thanks, but because your code is in the patch, you aren't eligible to rtbc, and neither am I. We will need heddn or someone else to rtbc now.

I was able to replicate the problem and the solution! The term whose details I modified updated when I ran `drush mim` a second time without the update flag.

However, the other two terms that I had not modified also updated, so I think a test somewhere in this code is giving false positives. Sorry!

@BenStallings I'm unable to reproduce your findings. But if I understand correct, all your items get updated every time you run a migration if you have track_changes: true ?
Could you maybe provide more information on how to reproduce this?

Sorry to be unclear. Here is what I did:

1. ran `drush mim beer_term` and 3 terms imported.
2. In the database, edited the details of one term in the source table.
3. ran `drush mim beer_term` a second time, and nothing updated (problem replicated).
4. applied the patch
5. ran `drush mim beer_term` a third time, and the output indicated that all three terms updated, instead of just the one that I had changed.

Thanks!

I've done everything again from scratch and I can't reproduce.
This is what I've done, fresh D8.8 install and added contrib modules migrate_tools and migrate_plus.

1. Enabled migrate modules, core, contrib and example
2. edit migrate_plus.migration.beer_term.yml, and add track_changes: true to source
3. apply patch from #26
4. run drush mim beer_term, result: 3 terms creates
5. edit details column of 1st record in table migrate_example_beer_topic
6. run drush mim beer_term, result: 1 term updated

Please note that if you edit the style column you will get a new term as that's the primairy key.

As far as I can tell this is good for "Needs review".

This looks good. Tests cover things. Mentally debated the vocab being created in the test module's config but it isn't that big of a deal. Onward.

Committed and pushed fc11a4f5c1 to 8.8.x and 2c7fde88e2 to 8.7.x. Thanks!

This is a nice thing to get fixed! Backported to 8.7.x because this is a major bug fix.

Just to document here: We don't use "needs backport" tags for minor versions. Issues are backported (or not) according to release allowed changes policy. The branch field should be set to the earliest minor branch that is eligible for the fix based on this policy. Thanks!