Comment migration corrupts data with multilingual sites

Discussed with @maxocub in Slack.

At first glance, we first need to migrate a langcode for the comments, and the d6_comment source plugin already join the node table so we can get it there.

Then for the step 14, if we look at d6_comment.yml migration, instead of entity_id: nid, we will need to get the new nid with a migration_lookup. I haven't tried to reproduce the problem yet, so I'm not 100% sure about this.

Here's a start. The patch does not have tests yet, I'm setting this to NR just to trigger the testbot in order to see if this breaks any existing tests.

Changes:

• Comment langcode is now mapped from the D6 node language.
• The node of the comment is now mapped using migration_lookup so that we first check if nid is found in d6_node mapping table and if not, we check if it can be found from the d6_node_translation mapping table.

D7-D8 migration most probably has the same bug, I'll check that next.

I've heard rumors that the system works a bit better if I actually attach the patch to the post. Doh.

Seems that there are quite a few failing tests. Let's break some more by adding D7-D8 to the picture. The same bug is there, the only difference is that the D7 comments have language directly in the comments table so we can use that instead of taking the language of the node.

Setting to NR to trigger the testbot for the D7-D8 tests.

I analyzed the test failures a bit for the D6 comment migration. Currently we don't map the destination 'langcode' property at all in d6_comment. As a result, the migrated comments get the site default language ('en' if the D8 site was installed in English).

The patch #11 leaves the destination langcode property empty / null if the source node did not have a language specified. This will boil down in D8 and is not OK.

Option A: Should we map the destination 'langcode' property only if the source 'language' property has been set? This would be close to what the migration is currently doing i.e. the missing langcode mapping will the lead to site's default language code to be assumed.

Option B: Or should we set the destingation 'langcode' as 'und' if the source 'language' property has not been set?

Cheers,
Markus

Actually I think the most elegant approach is to indicate the source site language as a fallback in the source plugin if the comment / node language is not set. I'll write a patch with this approach hopefully still today.

Still work in progress but submitting the next patch version.

This now has fixtures for both D6 and D7.

• On D6 fixture I used the existing Employee content type which had both translations and comments enabled. There were no existing nodes that were suitable for this test so I added two new nodes as translations of each other + comments to both language versions.
• On D7 fixture there were existing nodes were I added comments. Hopefully this does not break any existing tests; if it does, then I need to add new nodes there as well.

This also has the first attempt on the D6 tests. There is no test-only patch at this point of time as this is still work in progress.

The language determination logic was discussed today in the weekly meeting. This patch has the D6 language fallback logic in the source plugin. However, it was concluded that it might be more elegant to either try to use migration_lookup + default_language or to write a process plugin that reads the default language of the destination Drupal site. I'll continue on this + test failures (I'm sure there will still be) + D7 part tomorrow.

Here's a new patch. It needed a re-roll due to the fact that the D7 fixture had changed. This changed quite much so there's no interdiff. There will most likely still be some real test failures but let's see what the testbot says.

There's only a test-only patch that should fail and show the bug that we are fixing here. As mentioned above, there's most probably some other test failures as well but core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentTest.php and core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentTest.php have been updated so they should definitely fail with the new test data in the fixtures.

I'm still trying to figure out a least painful workflow with the fixture changes so please bare with me...

Cheers,
Markus

p.s. Big thanks to @maxocub on coaching me with the fixtures!

edit: p.p.s This patch still has the language fallback logic in the source plugin. I want to see all tests green before I add more complexity by writing a new process plugin.

D6 fixture had changed so this needed a re-roll.

Doh. I somehow managed to have an extra whitespace in the patch #20 causing it not to apply. Here's a working re-roll.

Here's the next version which should hopefully fix most of the remaining test failures. I could use some help with the MigrateUpgrade7NoMultilingualTest test failure, time to go to bed now...

About the changes here:

   entity_id:
-    plugin: migration_lookup
-    migration:
-      - d6_node
-      - d6_node_translation
-    source: nid
+    -
+      plugin: migration_lookup
+      migration:
+        - d6_node
+        - d6_node_translation
+      source: nid
+    -
+      plugin: skip_on_empty
+      method: row

This fixes the d6\MigrateForumTest failure where things are done to all comments regardless if they are forum comments or any other comments. Thanks @maxocub for your help with this one!

Cheers,
Markus

Fixing entity counts for the UI tests.

Patch #26 did not apply anymore, let's start with a re-roll. Expecting to see the same test failures as previously so no need to do a review yet, I'm just triggering the testbot by setting this to NR.

Another round with testbot. Unfortunately I'm not able to run the full migration tests on my dev environment so need to test them here :( Please bare with me...

1. Fixed.

2. I don't think we can.

• In D6 the two language versions were different nodes.
• In D8 the translations are just different language versions of the same node (nid 21).

3. Same as above.

4. Fixed.

5. Fixed.

Let's see if I got any luck with the entity counts in the UI tests this time...

Markus

EDIT: typos