Migrate D6 and D7 node revision translations to D8

Make sure the current revision of each node has the correct data in it. This must be true even if the current revision is not the highest one (ie: revision was reverted).

Reverting a revision in Drupal 6/7 creates a new revision based on the older one, so the only case where the current revision is not the highest one would be in the case of a not-yet-published forward revision.

Postponed on #2225775: Migrate Drupal 6 core node translation to Drupal 8.

@vasi: some contrib modules implement forward revisions with some core hacks, allowing for the "latest revision" to not necessarily be the published revision. See https://www.drupal.org/project/revisioning, Workbench moderation, etc, eg. https://www.drupal.org/files/images/workbench-moderation-screenshot.png (note difference between current vs. published revision -- the published revision state is orthogonal to the published node status). While these are not core features, entities encountered in the migration may have gone through these modules earlier.

Here's a failing test to start with.

Postponing on #2921661: Add support to migrate multilingual revisions.

I know this is postponed, but I wanted to make sure that what was being done in #2921661: Add support to migrate multilingual revisions was going to help here, and it does.
So here's what I've got so far.

Fixing the tests

#2921661: Add support to migrate multilingual revisions is in, unpostponing. First, it needs a reroll.

Correct 1 of the test failures (the rest confuse me!)

Let's try this to fix the remaining tests.

I did quite a lot of manual testing with D6-D8 and ran into two different issues.

This fist one is easily reproducible:
1. Have 2 languages installed
2. Have a content type (e.g. Story) that has multilingual support enabled
3. Create a Story node in English
4. Translate the node to Finnish
5. Create a Rev 2 revision in Finnish
6. Revert to the original Finnish revision
7. Run the upgrade to D8
8. Notice that the original Finnish translation is not shown in the revision list. One picture tells more than a thousand words so have a look at the screenshot below.

The second issue that I found is more trickier to reproduce but it leads to fatal error. I have a hunch on the root cause but I don't want to speculate before I have exact steps to reproduce. Once I have, I'll post the findings as a separate comment for the sake of clarity.

Cheers,
Markus

Edit: typo

Ok, here we go with further test results. For the sake of clarity, I post each different test to a separate comment.

1. Have 2 languages installed
2. Have a content type (e.g. Story) that has multilingual support enabled
3. Add taxonomy to Story so that the taxonomy does **not** have multilingual support.
3. Create a Story node in English and make sure that you have a taxonomy term associated with the node.
4. Translate the node to Finnish and ensure that the term is associated.
5. Create a Rev 2 revision in Finnish
6. Revert to the original Finnish revision
7. Create a Rev 2 revision in English
8. Run the upgrade to D8
9. Notice that the current Finnish revision is shown on the English revision list. One picture tells more than a thousand words so have a look at the screenshot below.

Note: this is not yet the fatal error issue I reported in #24. I'll post that as the next comment.

Edit: typo

And here's the third issue I found in my manual tests. This leads to a fatal error, details below.

1. Have 2 languages installed
2. Have a content type (e.g. Story) that has multilingual support enabled
3. Add taxonomy to Story so that the taxonomy has 'Localized' multilingual setting enabled.
3. Create a Story node in English and make sure that you have a taxonomy term associated with the node.
4. Translate the node to Finnish and ensure that the term is associated.
5. Create a Rev 2 revision in Finnish
6. Revert to the original Finnish revision
7. Create a Rev 2 revision in English
8. Run the upgrade to D8

After the upgrade, go to admin/content on your D8 site to see the migrated nodes.
A. Try to access the Finnish version of the node. It will be otherwise OK except the 'localized' term is not associated with the node. This is because of #2886609: Migrate translations for D6 i18n taxonomy 'localized' terms and more specifically because of #2859297: Migrate taxonomy term references for D6 Node translations. The latter issue (2859297) is about making the association between the translated node and localized term.

B. Try to access the English version of the node. You'll get HTTP 500.

• Web server log shows a PHP Fatal error: Call to a member function isTranslatable() on null in /path/to/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php on line 1161.
• In addition to this, Drupal recent log messages shows a PHP Notice: Undefined index: forums in Drupal\Core\Entity\Sql\SqlContentEntityStorage->loadFromDedicatedTables() (line 1161 of /path/to/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php)

EDIT / Additional information

The fatal error occurs also when there are no revisions i.e. when there is only the English D6 node which is translated to Finnish and they both have the 'localized' term. Let's move this part to be handled as part of #2859297: Migrate taxonomy term references for D6 Node translations instead of this issue as this seems to have nothing to with revisions migration.

Edit 2

The fatal error is caused by the fact that the term record in taxonomy_term_field_data does not have a langcode as it should. So this is definitely out of scope of this node revision migrate issue.

I was testing this more. This is highly dependent on the multilingual taxonomy term migrations. I tested so that I had the following patches applied:
- 2746541#22 (this issue)
- 2975509-50
- 2886609-86

The results were better but still not complete.

When using 'localized' terms, the results seemed to be OK

When using 'Per language' terms, the EN revision list was showing the latest FI revision as 'current' and the current EN revision was not shown as current.

When using 'Fixed language' terms, the revision lists were OK but the Finnish term was not associated with the Finnish version of the node.

This is way too complex to test without the term migrations being done first, let's postpone this on #2975509: Migrate D6 vocabulary language settings and #2886609: Migrate translations for D6 i18n taxonomy 'localized' terms.

Markus

Put the issues this is postponed on at the top of the IS for convenience.

Re-parenting so that all multilingual migrations can be found from the same meta. Moved original parent to related issues.

Unpostponed as this landed: #2886609: Migrate translations for D6 i18n taxonomy 'localized' terms. D7 part may need a separate follow-up.

Let's reroll this.

Aside from the reroll, the changes include moving the migrations to content_translation, creating a property for a variable in both source plugins. The node revision translation tests fail, but let's see what else may fail.

Testing for the error reported in #24. I was able to reproduce the problem and it is not related to translations. The error occurs when there has been a 'revert' action on a content type.

Tested by making a new content type, with out translations and with revisions. Added a node of the content type, then made a revision, then reverted that revision. Then I ran 'd6_node' and 'd6_node_revision'. When looking at the revision history on Drupal 8, the original revision is not listed, just like what masipila reported.

Tracked down the problem with the display to the field revision_translation_affected field. It is NULL for the 1st revision migrated and if I change it to 1, the display shows all the revisions. And thus solves the problem reported in #24. The question remains, how to migrate that value?

This field is a bit mysterious. Finding documentation on it isn't easy. Eventually found #2933518: The semantics of the "revision_translation_affected" field are unclear to Decoupled Drupal developers (REST/JSON API/GraphQL) users, improve this, and in there was a link to Entity.api.php Lin 102 which is the documentation. In that it states

* If a translation has changes in a certain revision, the translation is considered
* "affected" by that revision, and will be flagged as such via the
* "revision_translation_affected" field

So with that one would assume, at least I do, that revision_translation_affected is set only when a translation has changes in a revision. That is not true.

I created a D8 site with the standard profile, did not install any translation module or language, created a new content type that is revision-able and added a few revisions. Then, looked at the db tables node_field_data/node_field_revision, and revision_translation_affected is set to 1. There are no translations yet the field is set.

Related to this is the revision_default flag for each revision. That same documentation states

The "default" revision is the canonical variant of
* the entity,

Every revision for the test content type has revision_default set to 1.

So, I made another content type and that is not revision-able and made some content. Just like before revision_translation_affected is set to 1 even though this is a content type that is not revision-able and on a single language site with no translation modules enabled.

It is a really hot day here any maybe I'm not thinking clearly or done something wrong in my tests but if I haven't this is really whacky.

From a migration perspective, the solution is to set revision_translation_affected to TRUE and have a followup to determine what to do about that field value.

Whoa...

@quietone, can you summarize what would be the result if we would do this as you suggested?

From a migration perspective, the solution is to set revision_translation_affected to TRUE and have a followup to determine what to do about that field value.

Markus

@masipila, Yea, seems radical. The result is that migrate is duplicating what I see D8 doing in my testing. If I can make a non revision-able, non translatable content type that has revision_translation_affected set to 1 and that content type behaves normally, then it shouldn't break a migration for a non revision-able and non translatable content type. And since the field is supposed to be set for revision-able content type and revision-able and translatable content types it should not break those either.

Ok, sounds logical to try that approach!

A patch implementing the idea in #36. Haven't worked on the failing MigrateUploadTest

Reroll

Change the order of migrations in MigrateContent so that d6_node_revision_translation only runs after d6_node_translation and d6_node_revision.

Also, removed the 'vid: vid', from MigrateUploadTest which is an indication of a larger issue with revision numbers. I'm thinking that the migration of vid needs to be removed from all migrations except perhaps d6_node.yml. More on that later, it is too late now.

Assigning to myself to review this week. I want to take a look at the revision_translation_affected stuff and the revision ids.

Woke up thinking that it might be worthwhile to move the vid issue to it's own issue and focus on the revision_translation_affected here. What do you think?

I was testing the patch #45 manually on a break between the games. The patch breaks something quite badly, see the attached screenshot from D8 /admin/content with the patch applied:

I gotta go to play another game now so I can't troubleshoot this more right now but I did test the migration with and without patch #45. Clean 8.7.x -- the /admin/content looks normal
With patch ¤45 -- the /admin/content shows corrupted data as illustrated in the screenshot above.

Cheers,
Markus

p.s. I unassigned @mikelutz who had assigned self for review. Now that I kicked this back to NW, the assignment would indicate that @mikeluz would work on the actual patch.

Edit:
I'm not able to easily reproduce this issue with the /admin/content view.
I orignally had an English story node in D6 without any revisions.
I also had a Finnish story node which was linked as a translation to the English node
The Finnish node had a revision and I had then reverted the original as current.
When I migrated to D8, this was the result on the /admin/content

However, I then later added a revision also to the English node and reverted the original as the current. When I did this, the /admin/content view looked normal.

I tried to repro the issue shown in the screenshot above by deleting the English revisions on my D6 site but at least in this setup the /admin/content looked normal.

This might have something to do with the fact that originally I had the English node which did not have any revisions + a linked Finnish node which did have revisions. I need to go to bed now so I'm not able to try to repro this from the scratch.

D8 standard behavior
I chose to go back to basics and double check how D8 shows the revisions of multilingual content types when the data is created in D8. This is what we should achieve with migrations.

• Content type Article has 'Enable translations'
• I created an article in D8 in English and translated it to Finnish. Both language versions share the same nid (nid 2 in the example above)
• I made an edit to the English language version and then reverted to the original.
• I made an edit to the Finnish language version and then reverted to the original.
• The revisions in D8 are language aware i.e. only the revisions of the current language are displayed.

D6 data
I have similar data setup in D6, look at the screenshot below.

• Content type Story has multilingual support 'Enabled, with translations'
• I created a node in English and translated it to Finnish. Both language versions are nodes of their own i.e. they have different nids. The nodes are linked to each other as translations of each other.
• I made an edit to the English node and then reverted to the original.
• I made an edit to the Finnish node and then reverted to the original.
• Since these are two different nodes, the revisions are obviously language aware i.e. only the revisions of the current language are displayed.

Migration results with patch #45 applied.

English revisions after the migration
The screenshot below shows the English revisions after the migration.

There are two problems:
1. The Finnish revisions are displayed on the English revisions list.
2. The current English revision is not displayed as the 'Current' revision because the current Finnish revision is displayed as current.

Finnish revisions after the migration
The screenshot below shows the Finnish revisions after the migration.

There is one problem:
3. The English revisions are displayed on the Finnish revision list.

@masipila, I followed your instructions on how to create the nodes in D6, ran the migration via the UI and got different results. The English revisions were OK but there was one revision missing in the Italian revisions list. I'll look into that but I'd really like to reproduce your results. Can you post the Drupal 6 node and node_revisions tables you used? Hopefully, you still have it around.

Ah, found that vid:vid was still being used. I have removed them it the migrations are working for me, at least for my test cases.

Manual Drupal 6 tests
Re #48: I'm not able to reproduce this issue anymore. I'm suspecting that I managed to screw up my D6 test data somehow in D6 which triggered this weirdness.

Re #49: I re-tested patch #51. My migration results with the nodes shown in #49 are quite weird but having a closer look in D6, it's even more weird there i.e. I somehow managed to trigger some D6 bug.

So I created completely new set of test data in D6 as follows:

English

• I created a new English node
• Created a revision to the English node
• Then reverted to the original English node

Finnish

• Created a Finnish translation to the English node
• Created a revision to the Finnish translation
• Then reverted to the original Finnish node.

The D6-D8 migration results were all OK.

• The D8 English revision list was showing the expected English revisions only
• The D8 Finnish revision list was showing the expected Finnish revisions only

Drupal 7 / node translation tests

• I have content type which has multilingual settings as 'Enabled, with translation'
• Same test scenario than what I used above for D6-D8

Results for D7 node translation tests were all OK.

• The D8 English revision list was showing the expected English revisions only
• The D8 Finnish revision list was showing the expected Finnish revisions only

Drupal 7 / entity translation tests
Drupal 7 also supports entity translation concept so I tested what happens with that.

In Drupal 7, the 'Revisions' tab shows a list of all revisions of the node

• When viewing en/node/21/revisions, I'm seeing all revisons regardless of the language
• When viewing fi/node/21/revisions, I'm seeing all revisons regardless of the language

Entity translation results in D8:

• When viewing en/node/21/revisions, I'm seeing all revisions regardless of the language
• When viewing fi/node/21/revisions, I'm seeing only two Finnish revisions

I'm close to have a brain meltdown trying to think what does the node revision conceptually mean in the context of entity translation and languages, because the different fields may be translatable or then not. I would be tempted to say that this is the best that we can achieve but others may present better ideas if you're able to explain a concrete example.

Conclusion:

• My manual tests were all OK but I can see that there were 4 failures in the automated tests.
• Leaving NW to address those
• I did not have time yet to do a full review to the patch and the test coverage we have there

Cheers,
Markus

I queueud this to PostgreSQL again to see if we are now down to 4 errors like on MySQL and SQLite. I assume that the difference in the error count was due to the fact that the whole 8.7.x was not passing with PostgreSQL but that seems to be fixed now. Let's see.

I read the patch #51 on my way home today.

1. Question: Could you please describe the background why we needed to remove the vid:vid mapping? I can see that it was removed also from d6_upload.yml which wasn't obvious to me. Do we have something similar for D7 that should be taken into account?

2. I checked the test coverage. Could we add a test to D7 fixture for a content type that uses entity translation? Point being that this is complex and if the migration result ever changes because of some other issue, we would notice it?

Cheers,
Markus

Re: #54. Just as I assumed and hoped, the PostgreSQL has now the same amount of fails than other database backends.

Fixing the failed tests requires changing the revision ids.

Hmm, looked at the patch in #58 and it is not right, ignore it. This starts again from #51

Still to do is to address the question in #55

Rerolling the patch in #59 but excluding the D7 work because there are changes to the source fixture that do not apply and that is too much to deal with just now.

Now add the d7 stuff. Rerolling the d7 test fixture was a disaster so I remade it and removed many, but not all, of the unnecessary changes. I haven't run all the tests, so lets see what may fail.

Oh and no interdiff since this is just adding the D7 code.

Might have a working d7 fixture now. There is no interdiff since this contains an untrimmed version of the fixture. Reducing the size will come later once this passes tests.

Try again.

Let's try this fixture, it has been trimmed a bit as well.

Try again, only changes are to the fixture.

And update the new d7 MigrateNodeRevisionTranslationTest to use the correct text for the title fields.

Update IS
NW for the remaining tasks

This is an attempt to add revisions to the test content type which uses entity translation. I expect lots of test failures.

Remove more changes from the fixture. Plus modify a test since the field field_text is now translatable.

This trims the d7 fixture some more and add tests. Specifically adding to d6/MigrateNodeRevisionTest, d6/MigrateNodeRevisionTranslationTest, d7/MigrateNodeRevisionTranslationTest and creating a d7/MigrateNodeRevisionTest. No interdiff because of all the changes to the fixture make it full of noise.

Did some manual testing for d6 and d7. The d7 one created extra nodes and crashed when trying to view a node, so pretty bad there. The d6 one looks much better. I noticed the following problems.

1) The page content type is not translatable and it should be, it has translated nodes in d6.
2) The tag field in the page content type is a required field but the drop down has no selections so you can't enter data. I deleted the field to keep testing.
3) The revisions listed for the french translation of the page are in vid order which is not in the date entered order.

Manual testing of d6 I discovered that the node revisions for node 1, which does not have translations, are NOT listed in the revisions tab. So, I deleted and started over this time without the translation modules enabled and ran the migration from the UI. Same results. What would prevent revisions from being listed in the revisions tab?

@quietone, looking at \Drupal\node\Controller\NodeController::revisionOverview(), it seems that the revisions tab only displays the revisions which have the revision_translation_affected field set to 1 in the database. Maybe the migration process doesn't populate that field?

If that's not the problem, can you give me some pointers (or a link to a docs page) on how to replicate your manual testing process? :)

@amateescu, revision_translation_affected is set to true in the d6 and d7 node_revision_translation.yml. But there is a problem with the revisions without translation enabled. The d6 fixture has node 1 with 3 revisions and post migration only the latest one shows up on the revisions tab, node/1/revisions. That needs to be fixed first before adding the translations on top.

Right, so for testing. First, I just checked and didn't see a docs page for how to test. So, here goes.

1. Add an entry in settings.local.php for the d6 database dump.
2. import the fixture using db-tools, php core/scripts/db-tools.php import --database=d6_dump core/modules/migrate_drupal/tests/fixtures/drupal6.php
3. enable migrate_drupal_ui. Don't enable the translation modules.
4. navigate to /upgrade and complete the form and run the migration
5. then goto /node/1/revisions and there will be one listed not the three it should be

Screenshots:
Drupal 6, node/1/revisions

Drupal8, node/1/revisions

This problem with the node revisions, without translation, should be moved to it's own issue. Hopefully, I will get to that before the end of the day.

Created a new issue for the problem with the node revision tab without translations. #3073088: d6 term node migration changes revision log on wrong revision.

Remove changes for node revisions tests which are now in #3073088: d6 term node migration changes revision log on wrong revision

This is a bit over my head to review. Can you update the issue summary in terms of what is the before/after migration scenario? There are a lot of data updates in the dumps to make this clear on review to me sorry.

+++ b/core/modules/node/tests/src/Kernel/Migrate/d6/MigrateNodeRevisionTranslationTest.php
@@ -0,0 +1,131 @@
+    // Translation revisions are not migrated.
+    $ids = [14, 16, 23, 2009];
+    foreach ($ids as $id) {
+      $revision = $this->nodeStorage->loadRevision($id);
+      $this->assertNull($revision);
+    }

What does "translation revision" mean here?

I'm working on reviewing this, but It's going to take me a couple days. I have a pretty good understanding of what is going on, and my initial test show some things in the migrated database that don't seem exactly right to me. I'm working on seeing what we need to adjust to make it work.

Item 2 of remaining task: Could you please describe the background why we needed to remove the vid:vid mapping?
I removed that change because honestly I don't remember all the details. And it does not seem to have caused any problems. So removing from the remaining task list.

I am really tempted to split this into a d6 an d7 version because the d6 has the added complication of the effect of the term node migrations and then d7 has the entity translation which d6 doesn't.

Any objections?

I think splitting them off is a good idea. I suspect removing vid:vid was necessary in d7 entity translation migrations because each vid has multiple translations, but we are trying to save them one at a time, which the entity API doesn't like. Problem with that is that we are then saving past revisions with higher revision IDs than default revisions, and that can cause problems in the system. We may need to go back to scratch and rework the source and destination
such that we can save a whole revision at once somehow.

Talked to mikelutz and heddn about this today and we decided to make a new issue for working solely on the d7 entity translation revisions. And that here, the priority is the D7 migration. It is much that same as D6 and since with D6 there is a possible complication with the term node migration it is best for right now to just get the D7 working correctly.

I've been staring and the revisions before and after a D7 migration and found there are 'extra' revisions made. Then I remembered the entity translation. Yes, d7_entity_reference_translation is migrating data for the article content type, which should not be using entity translation. Is there yet another problem with the fixture?

edit: Hmm, that is a follow up migration and is adjusting entity references. that is not causing the extra revisions

This patch only changes the d7 migration.

Spent quite a bit of time running the d7 migrations in different order and looking at the resulting tables. For now, going with d7_node, d7_node_revision, d7_node_translation then d7_node_revision_translation. And I added vid:vid back to the node translation migrations and that seems to work for d7 where the current revision is the highest revision id.

One problem is the revision tab post migration were incorrect, the revision tab for the english node was showing an icelandic revision. Tracked that to the d7_node_translation migration which will creates two revisions for the one icelandic row and both have the revision_translation_affected flag set TRUE. The only place I could see to set that to FALSE for the english revision is in onPostRowSave in NodeTranslatoinMigrateSubscriber, which has to load that revision, set the value FALSE, and then save again. It is unfortunate to do that extra work.

The migration dependencies haven't been changed yet and the migration of a field that changes on the revision (field_text_plain) needs to be checked.

The errors in the patch are all d6 related so that is fine. Right now just focusing on D7. Anyone reviewing, the next step is to check the results that are explained here.

Now to explain the results for d7.

The d7 source has 4 revisions, 2 'en' and 2 'is', with the following nid/vid.
d7 source
2/2 en first rev
3/3 is first rev
2/11 en 2nd rev
3/12 is 2nd rev

The d7 results has 6 revisions, 4 'en' and 2 'is'. They are shown below along with the migration that creates them. The translation migrations do not specifically migrate the 'en' versions, exactly where that is happening I don't know. There is only 1 row in the map table but two revisions are created. The 'is' translations and there fields are correct. The "extra" 'en' revisions are always the latest revision and have revision_translation_affected set to 0 or NULL so they don't display in the revisions tab.

d8 results
The values are nid/vid/langcode, revision, revision_translation_affected

d7_node
  2/11 --> 2/11/en, the 2nd revision, 1
d7_node_revision
  2/2 --> 2/2/en, the 1st revision, 1
d7_node_translation
  3/12 --> 2/12/is, the 2nd revision, 0
      |--> 2/12/en, the 2nd revision, 1
d7_node_revision_translation
  3/3 --> 2/3/is, the 1st is revision, 1
      |--> 2/3/en, the 2nd revision, NULL

Is this an acceptable solution? What problems might it cause?

edit: formatting

This is close, but not quite right.

There should be 4 en and 3 is when is all said and done. It should have

2/2/en/the 1st revision 1
2/3/en/the 1st revision NULL
2/3/is/the 1st revision 1
2/11/en/the 2nd revision 1
2/11/is/the1st revision NULL
2/13/en/the 2nd revision NULL
2/13/is/the 2nd revision 1

That being said, the entity api is really going to fight us trying to get that structure saved with the final default revision being saved first.

Right, that is what D8 does, which isn't possible to produce using the current way migrate works. Is it necessary to match the revisioning that D8 would do or is it sufficient to keep all the data and display it in the correct order?

Highlighting differences between what D8 does (as shown in above comment) and current migrated results

2/2/en, the 1st revision, 1
2/3/en, the 2nd revision, NULL ---> this is the the first revision in non migrated D8
2/3/is, the 1st is revision, 1
2/11/en, the 2nd revision, 1
2/11/is/the 1st revision NULL ----> missing from migrated version
2/12/en, the 2nd revision, 1 ---> revision translated affected NULL in non migrated D8
2/12/is, the 2nd revision, 0 ---> revision translated affected 1 in non migrated D8

edit: add comparison of results

So, is the solution in #103 sufficient. mikelutz disagrees but local testing has convinced me it isn't going to get any better. Anyone is welcome to prove me wrong.

Getting all the revisions to match what D8 would do out of the box doesn't seem possible in the current environment. The dN_node_revision migration will migrate ONLY the default language revision, there will NOT be a translated revision with that vid. Whereas D8 doesn't do that. When I make a new revision of a node in the default language (en) and save, the result is that both a default language (en) and a translated (it) version are saved. That is why this translation "2/11/is/the 1st revision NULL ----> missing from migrated version" is missing in the example above. Now, functionally I think that will work, at least in my local testing.

I had an idea of another approach and discussed with mikelutz and heddn on slack. The idea was to not use the existing node, node_revision and node_translation migrations. Instead, we need to ask the user if they want to run multilingual with or without revisions and then when they want revision translation we use a new migration (or migrations) with a new fancy source plugin that does what is needed (details TBD) and unset the dN_node* migrations that aren't needed. That will hopefully give us the ability to save the revision translations correctly.

mikelutz thought this "would work just fine for the core one button upgrade" but that it wouldn't for custom incremental migrations.

But as heddn pointed out unsetting the dN_node family of migration is a problem. They are used in migration_lookups in core and in migration_dependencies. Most of those are dN_node so that problem could be alleviated if dN_node was still run.

Here is a version of the patch with just D7 changes. Let's get this one sorted then we can go back and update the D6 version.

This the the node_field_revision table post migration in the test node/tests/src/Kernel/Migrate/d7/MigrateNodeRevisionTranslationTest.php. If the same database is used as the source for running the migration from the UI, the results are likely to be different since the migration may run in a different order.

MariaDB [test]> select nid,vid,langcode,title,revision_translation_affected from test44775560node_field_revision where nid=2 or nid=3;
+-----+-----+----------+---------------------------------------------+-------------------------------+
| nid | vid | langcode | title                                       | revision_translation_affected |
+-----+-----+----------+---------------------------------------------+-------------------------------+
|   2 |   2 | en       | The thing about Deep Space 9 (1st rev)      |                             1 |
|   2 |   3 | en       | The thing about Deep Space 9                |                          NULL |
|   2 |   3 | is       | is - The thing about Deep Space 9 (1st rev) |                             1 |
|   2 |  11 | en       | The thing about Deep Space 9                |                             1 |
|   2 |  12 | en       | The thing about Deep Space 9                |                             0 |
|   2 |  12 | is       | is - The thing about Deep Space 9           |                             1 |
+-----+-----+----------+---------------------------------------------+-------------------------------+
6 rows in set (0.001 sec)

The IS proposed that "for each D6 and D7 revision, the migration source should yield every translation that we think goes along with it. This means multiple rows per revision, which is slightly confusing. But it will provide a mostly-sane revision history.". I don't see how to make that work. First there needs to be some logic to determine which nid/vids goes along with a translation. But then you have to manage multiple node translations in a single Row and the mapping? Seems custom destination and idmap would be needed?

@quietone: so my mental model is languages are columns on a table where rows are revisions. When a new revision is saved, Drupal saves translations as well, so when you load the latest revision you get all translations in their most up to date form in that revision. If migrations don't save with this mental model into the revisions database tables then how does loading translations work?

I don't personally think we necessarily need to merge the revision rows creatively. The vids are globally consecutive (across entities) so all the migrations would need to do is to save revisions in vid order and rely on the entity API saving the other translations with the right value that was already available. That way merging various entities into one should result in the right order of revisions and all revisions should contain the most up to date translations at the time, no? Something like:

Node 1 French translation: revisions 4, 7, 12
Node 2 English translation: revisions 3, 8, 11
Node 3 Spanish translation: revisions 5, 6

(Node 1, 2 and 3 should all have unique vid values since it was not possible in D6/D7 to save a vid across entities).

So then we can order by vid and merge accordingly. Resulting merged node should contain revisions:

• 3 (only English at this point)
• 4 (English carries over and French added)
• 5 (English and French carry over and Spanish added)
• 6 (All languages carry over, Spanish updated)
• 7 (All languages carry over, French updated)
• 8 (All languages carry over, English updated)
• 11 (All languages carry over, English updated)
• 12 (All languages carry over, French updated)

If we cannot/don't want the vid to stay the same, that does not matter in terms of the local history of the entity, we can renumber them. As long as we saved with the original vid order, we should merge them perfectly on top of each other, no?

This is what I mean when I say that the migrations should be combined into one, and we need to use the entity api to 'replay' the data in the node revision table in the same order it was created to avoid fighting the entity API. Let the entity API do the merging for us.

I have managed to run a migration by doing the revisions in order, just for the article content type in the test fixture. The results of that are the same as the results in #103 but not taking into account the revision_translated_affected field which I am not worrying about right now. The key thing is that when migrating a revision that is not a translation the result is different than when adding such a revision through the UI in D8. When adding a English revision in D8 the revision table will have a new English and Italian revision with the same vid. When migrating an English revision the revision table will have only a new English revision. As such, I don't see how it is possible to match the D8 structure exactly. I don't see it is possible to achieve the results in #104.

I have not tested with a source like the example in #110.

Now, I do agree that it is better to run this in revision order and would like to pursue that and see what problems we run into but first, do we agree with the results I am getting.

edit: Forgot to add that using the UI post migration the revision tabs are wrong.

I believe I followed the logic of #112 but would it be possible to have screenshots or diagrams that highlight the conceptual differences between D8 and D6/D7? For example similar diagrams that we have been using here:
https://www.drupal.org/docs/8/upgrade/upgrading-multilingual-drupal-6-to...

I mean, the amount of energy that has gone into digging into this is so exessive that it is worth summarizing it so that we can minimize the risk of needing get back to this monster and tear up the whole thing...

Cheers,
Markus

I'm not good at making diagrams and screen shots are not sufficient to show the differences. I have settled on showing the database tables for the source D7, D8 post migration and D8 made via the UI. What I made via the UI should reflect the Drupal 7 source. All this is in the attached text file.

Are we making the assumption that since the post migration database tables does not match what is done via that UI that it is broken? What if the differences are OK and the site works just fine? I admit I have done limited testing but it did seem to be working with the patch in #88. Has anyone else tried?

Thanks for the details. I think this is a problem:

DRUPAL 8 POST MIGRATION

Now let's look at the same table post migration of Drupal 7 data which should be the same scenario. The Drupal 7 tables are at the end for you to check. Here I want to try to show the difference between the Drupal 8 UI and post migration. Of course the nid and vid are different, it is the pattern that is of importance.

The revisions post migrations are not the same as through the UI. The nid/vid pairings are different in two situations. One is when an English revision is added and the second is when the French translation is added. In these two cases the import of the migrated row does not generate the revisions in the other languages like what happens when saving via the UI.

MariaDB [d8]> select nid, vid, langcode, title from node_field_revision where nid=2 order by nid, vid;

+-----+-----+----------+-----------------------------------+
| nid | vid | langcode | title                             |
+-----+-----+----------+-----------------------------------+
|   2 |   2 | en       | The thing about Deep Space 9      | migrate first English node
|   2 |   3 | en       | The thing about Deep Space 9      | migrate Icelandic translation
|   2 |   3 | is       | is - The thing about Deep Space 9 |
|   2 |  11 | en       | The thing about Deep Space 9      | migrate English revision
|   2 |  12 | en       | The thing about Deep Space 9      | migrate Icelandic revision
|   2 |  12 | is       | is - The thing about Deep Space 9 |
|   2 |  15 | en       | The thing about Deep Space 9      | migrate 1st French translation
|   2 |  15 | fr       | fr - The thing about Deep Space 9 |
+-----+-----+----------+-----------------------------------+

8 rows in set (0.001 sec)

A. Your latest revision does not have an Icelandic translation. When you edit it does it even appear? How would Drupal know it was not intentionally removed? (There can be any number of independent changes in a revision).

B. Also, what happens when you revert back to revision 11, that also does not have the Icelandic translation. Ok it does not have French either but that was not yet supposed to have it. This B case may be solved with the revision translation affected logic.

I'll ask Francesco to chime in if he can as well, he knows how A and B expect data and where are the bounds of acceptable data.

Revert does not seem to be a problem, because revert only reverts the state of the translation being acted on, not the whole entity, as per @hchonov in #2766957-2: Forward revisions + translation UI can result in forked draft revisions https://api.drupal.org/api/drupal/core%21modules%21node%21src%21Form%21N... well, at least a node revert. How does loading/editing code with translations not being present in the latest revision?

and finally what I have been testing with. No point running tests but you are welcome to have a go migrating with drush.

There are two migrations to run to migrate the revision translations. d7_node_first_revision.yml followed by d7_node_revision_translation.yml. Both use the Node.php source plugin, which is modified to use some added configuration keys to make the desired query. It was the quickest way I could think of to test getting the revisions in the correct order.

It is definitely a serious problem when a new revision is created, which does not contain all the translations. As seen in #114 the last revision contains only 2 instead of 3 revisions, which might considered data loss. Even if the data is in the database it is not present on all revisions.

There might be 2 reasons for that:

1. The wrong revision is retrieved for a base when creating a new one.
2. For some reason only a subset of translations are retrieved from the current revision.

I've seen something strange that I don't understand in \Drupal\migrate\Plugin\migrate\destination\EntityRevision::getEntity() where the new entity revision is flagged as a non-default revision. When an entity revision is saved in a non-default revision, then this revision will not be returned by $storage::load(), but only by $storage::loadRevision() with the right revision ID.

In order to understand better what exactly is happening it would be better if every revision has an unique change on a specific translatable field - for example a counter. We need the source data and the migrated data with this counter. This should confirm whether we are retrieving the correct revision and using it for creating a new one.

Also could you please test by removing the following line from \Drupal\migrate\Plugin\migrate\destination\EntityRevision::getEntity():
$entity->isDefaultRevision(FALSE);

---

I also don't see a place where we're actively removing translations, therefore I would say that for some reason the wrong revisions are being used as a base.

How does migrations ensure that the default revisions are accurate and have all the translations and why can that not be used to do the same for revisions? They would need to save over each other for the various languages. In more discussion with @hchonov, he said one of the possible problems is "you cannot pick a revision and simply change the content language and see how this revision looks in a different language". Also while reverting a revision with the UI would work, using the Entity API could still lead to data loss.

Here is the relevant proof of concept master migrations for D6 and D7. There is a lot of work left to do to actually integrate these into the migrations, and make sure we are handling non-translatable and non-revisionable nodes correctly. Additionally, this works with core translations, and would probably fail with entity translations. Those will be trickier, but I think should also be done in one migration in a similar fashion.

I had to adjust the D7 fixture as it's currently invalid, using node 1 vid 1 as the current default instead of vid 2001 like it should.

I dropped the test suite down to just the two migration tests, and put an export of the node_field_data and node_field_revision tables in the test files for both so you can see what the migration should end up looking like.

There may be some additional work around making sure the fields and everything are correct across revisions too, I was just worried about the main node tables being correct.

I think we can also fix content translation source to the default translation, we don't really have the concept of a translation source in d6 or d7.

@mikelutz, thanks. That is much nicer, clearing I was on the wrong path. The removes any remaining files for that wrong patch and adds the assertRevision method to the d7 MigrateNodeMasterTest. That adds two things to the test that mike wrote, a) it load the revision to test the fields and b) it actually has a plain text field that has data that states the revision it is associated with. And that is encouraging.

Testing this with drush and it works very well. Although I do seem to be able to break it in some way that the is translations wind up on the revision tab of the english version. But I was also doing a fair bit of rolling back and remigrating so that may not mean a whole lot.

What definitely fails is running this from the UI. It is failing somewhere in the auditing done for the IdConflictForm'.

The website encountered an unexpected error. Please try again later.

LogicException: Cannot determine the highest migrated ID without an integer ID column in Drupal\migrate\Plugin\migrate\id_map\Sql->Drupal\migrate\Plugin\migrate\id_map\{closure}() (line 978 of core/modules/migrate/src/Plugin/migrate/id_map/Sql.php).
array_filter(Array, Object) (Line: 980)
Drupal\migrate\Plugin\migrate\id_map\Sql->getHighestId() (Line: 36)
Drupal\migrate\Audit\IdAuditor->audit(Object) (Line: 52)
Drupal\migrate\Audit\IdAuditor->auditMultiple(Array) (Line: 74)

This version has changes so that this would run from the UI. There are two changes.
One is cobbled together code in migrate_drupal_migrations_plugin_alter to not run d7_node, d7_node_translation and d7_revision. I'm still leaning to having a question in the UI that asks if they want to run multilingual migration with node revisions. Not sure about drush though.

The other change is to Sql::getHighestId() to prevent the exception shown in the previous comment. Original the getHighest method checked that every id had a type on integer. Obviously that doesn't work with a langcode id. (Which, btw has a type of 'language' - where is that defined?) Now, getHighest method should throw the exception only if an id with an integer type is not found.

Fortunately, the results are the as with drush.

The revision tabs post migration are not yet correct.
English revision tab
Icelandic revision tab

The reason that all revisions show up on both trans for node 2 in d7 is due to the untranslatable taxonomy fixed field. It's set in revision 2, removed in revision 3, and restored in revision 11 and removed again in revision 12. I can compensate depending on what that should is supposed to be doing, but based on that each revision affects each language, which is why they all show up.

Add a few things to the migration. Add the Multilingual tag, a migration for content_translation_source, declare the destination module, add a dependency on language migration. MigrateNodeMasterTest is updated with the content_translation_source values and what I think are the correct values for revision_translation_affected. The latter causes the test to fail.

Edit: only changed d7, not d6

The discrepancies in revision_translation_affected for D7 node 2 is the same issue as the revision tabs described in #126. (And in fact are simply two descriptions of the same thing, as the revision tab display IS the revision_translation_affected)

The untranslatable field is only stored on the original node, and isn't pulled for the iceandic copy
The Icelandic translation added in revision 3 removes field_vocab_fixed, which is an untranslatable field. This mean that when we are add revision 3 in icelanding we are affecting the english translation, as we cannot modify field_vocab_fixed in one translation and not the other. Revision 11 has a value in the D7 fixture for field_vocab_fixed, so the field is reset in revision 11 (again affecting both translations) Finally it is removed again in revision 12 (still affecting both translations) .

I can't quite seem to figure out What is supposed to be happening here. field_vocab_localize and field_tags both also come across as untranslatable fields, but they have entries in the d7 fixture for both languages that happen to match, so they are fine.

Is there something in the configuration of field_vocab_fixed such that is only has a record in the default language and for the translations we should go get the value from the original node? If we can find the condition that declares it in the D7 database we can either adjust the source to include the field data from the original node (which the entity api will correctly detect as no change for English, and thus set the rta flags correctly), or we can set the untranslated field to match the original language in the destination, again causing the entity api to manage the rta flag correctly. I just need to understand how D7 is interpreting the field, and why it's different in the fixture than the other untranslated fields.

Yes, the vocabulary fixed is problematic and behaves differently for me in D7 than in D8. I found that out when working on #3035392: Migrate vocabulary translations and taxonomy term references for Drupal 7 node translations. See Comment #18.

Interesting. I'll fire up a d7 instance with the fixture and play with it and pick Gabor's brain on exactly what data state we want on that type of field. I'm certain I can adjust the master migration to handle it once I know what we want the d8 version of that type of field to look like. Otherwise, I'm really happy with how the one shot approach is working though. I definitely think it's the right solution to pursue.

As per discussion with mikelutz on slack I'm going to look into fixing tests. This patch restores drupalci.yml and changes the @group on the two new MigrateNodeMaster tests. Now, lets see what fails besides the functional test.

Oh, and I did an IS update so removing that tag. Hope it is sufficient for now.

Move the check for a multilingual source database to MigrationConfigurationTrait.

@quietone

The IS proposed that "for each D6 and D7 revision, the migration source should yield every translation that we think goes along with it. This means multiple rows per revision, which is slightly confusing. But it will provide a mostly-sane revision history.". I don't see how to make that work.

So for Drupal 7 field translation this seems like the 'simplest' option since the Drupal 8 entity structure is similar.

But... is it possible for that approach to co-exist with a Drupal 6/7 tnid-style source?

Now update the failing D6 Node tests to use the values in the current fixture. Move the node master migrations to content_translation. And coding standard fixes. That should fix a few more tests.

In EntityContentBase instead of setting new revision true if the entity is revisionable set it if it revisionable and IsNewRevision. Found this while looking into the MigrateUploadTest was failing, where on import it was making a new revision when the content type has new_revision as false.

@catch, not really, and instead of that method we are trying what mikelutz proposes, in #121.

@Gábor Hojtsy asked my feedback on #114.

I just went through the whole thread (I didn’t read any code though) and I agree with the issues @Gábor Hojtsy and @hchonov brought up. In general I don’t think we can avoid matching the D8 data model strictly: a missing record would mean that the related translation does not exist in that revision. Since we don’t track parents, that may either mean that the translation was removed or that the “parent” revision had no such translation in the first place. In any case that outcome would be wrong, since the translation should always be there, as we wouldn’t be migrating it otherwise (the entire d6/d7 node would have been deleted in that case).

The revision_translation_affected flag acts as a poorman’s “parent” field, as it allows to reconstruct the translation history by connecting the revisions affecting it.

@plach, thanks for the input.

This patch has more test fixes and add brings the d6 migration and d6 tests up to date. And hanged the MigrateNodeMasterTests to be easier to read, at least that is the intention.

I have found a problem with this approach and the idAuditor. The auditor will search for the highestId() on the destination and compare that to the highestId in the relevant migrate_map table or tables. Because the destination of dN_node_master is node, we are not checking the node revisions and we can't inform the user if they existing revision ids that will conflict with the ones to be migrated.

Here is a hack that adds auditing code to IdConflictForm so that it will audit node revisions.

OK, I was wrong earlier about the tests being easy to fix. This is a bit of a rabbit hole. Looks like we will need to test fixtures per Drupal version, one to be used for i18n and one not. And when it is i18n use the node_master migration and not run the others in the dN_node family, and alter all the processes and dependencies to use node_master, and then add a new process to account for the fact that node_master returns 3 ids not 1 like the dN_node family. Fun eh but possible.

All that does nothing to get the revision_translation_affected field to be correct.

This needed a reroll because of changes to the fixture in #3031727: Migrate D7 synchronized fields. That change uses the field_text_plain field which is also used here. To avoid the conflict I changed this patch to use the field, field_text_long_plain. And then, of course, the MigrateNodeMasterTest needed to be changed.

For some reason this reroll is giving me a headache.

Missed some changes to the test fixture.

Here's a fix for the failing kernel test. Note I have only fixed the test - I've not checked that the test assertions are actually correct.

The migrate UI upgrade fails are interesting - the test could be more helpful and tell us what migrations are failing. Here are screenshots of what is going on the D6 migrate upgrade UI test:

Here's my patch :)

This just adds some comments.

The migrate UI upgrade fails are interesting - the test could be more helpful and tell us what migrations are failing.

Yes, that is a pain and should be a new issue.

For d6 I'm showing failures in these migrations, Books, Comments, File uploads, Term/node relationships, Term/node relationship revisions.

Edit; new issue made #3082211: Migrate UI upgrade tests should provide the complete log

To make sure that dN_node_master was being used everywhere for multilingual I tried altering all the migrations. Seems to work locally.

Now alter any migration_lookup in core that has a source migration of dN_node* to use dN_node_master and add a plugin to that pipeline that will retrieve the desired IDs. That is needed because dN_node_master has 3 source IDs but dN_node has 1 source ID and dN_node_translation has two. For example, a lookup on d7_node will return the nid but a lookup on d7_node_translation expects two ids to be return, the nid and the language. The new process plugins handle that.

Some fixes and code cleanup. Only alter migrations that are not dN_node family.

The changes so for for migration_lookups won't work. More on that later - I have to run to catch a bus.

I'm concerned that we might run into BC issues. If we didn't have to concern ourselves w/ existing migrations, this would be a lot easier.

That said, the proposed solution is probably the *only* path forward. And it might be a good reason to keep the migrate_drupal_multilingual module though as the way to tell core and migrate_upgrade to use the master migration in place of the existing "normal" migrations.

TLDR; +1 on architecture. Need more discussions on actual implementation.

@heddn, thanks good to have confirmation of mikelutz's node_master work.

Using migrate_drupal_multilingual to tell core and migrate_upgrade to use the master migration in place of the existing "normal" migrations is perhaps changing the meaning of that module. Right now that module only says 'this is multilingual' whereas for node_master we need something that says 'this is multilingual with revision translations'. Now for core UI that may be fine, after all the idea is to migrate a copy of the original site which should include revisions. That's OK, just want to be clear on the point.

But the real challenge is the existing migrations. Hopefully it is limited to the subset where migration_lookup on dn_node/node_revision/node_translation migrations. I think we need to alter those migrations to use a new smart wrapper around migration lookup that will check both the dn_node/node_revision/node_translation table or dn_node_master to find the destination ids. We could avoid altering those migrations by creating 'copies' of the affected migrations and change migration_lookups to use the new smart wrapper around migration lookup and do some unsetting of migrations to remove the ones we don't want to run. Make sense?

Are there other cases where the existing migrations will fail?

You know, random thought. I didn't actually get round to building anything out for #3061676-2: Create an audit plugin class/manager for migrations, but theoretically, that might be an excellent place to detect the existence of revisions+translations in d(6|7), ask the user if they even want to migrate revisions, and if so, disable the standard node migrations, enable the master one, and even tweak any other dependant migrations that might need a process adjustment.

I think migration_lookup issues will be helped by #3004927: Create Migration Lookup and Stub services and possibly a more advanced lookup plugin based on that service. The service has better handling of lookups with a subset of the source keys, and would be useful if we are running a master migration and need to lookup based only on nid for example.

Yes, I am looking forward to the migration lookup service.

This is a better way to do the migration lookup, a process plugin that will check both the original dn_node migration and if not found check the dn_node_master. It should be smarter and check the migration property so that if it is dn_node_master it checks that first.

And this is an even better way to change the migration_lookups. The Upgrade7Test now has two failures, d7_book and d7_comment. I haven't run Upgrade6Test locally.

I accidentally removed all the code in the migrations_plugin_alter that was altering the destination and the dependencies. This restores that code. And a change to the _isMultilinugalSource function in used in the alter so that it checks that the definition of the migration 'system_site' exists before using it.

Add the patch from #3082719: migrate_drupal_migration_plugins_alter() should only alter definitions that exist for a problem I discovered while working on this. And fix coding standard errors.

There will still be failures.

I made a small mistake in that patch.

One way to fix the Kernel tests is to create a copy of the drupal6 and drupal7 test fixtures and change the status of the 'i18n' module to 0/uninstalled. That way the master_node migrations are not used and the test can proceed as it did before. It would be nice to use migrateDumpAlter() to do this but that happens after the migration_plugin_alter. I've only changed the source fixture on the failing tests. This may not be the best solution but it is a step forward.

The interdiff excludes changes to the fixtures.

Well, the Kernel tests are passing now, that just leaves the Functional tests.

Needs a reroll.

Fix d7/MigrateNodeRevisionTest.php and remove unused use statement.

Now, back to fixing the functional tests. The Upgrade7Test looks straightforward but I need to bring up a D7 site and run the full migration to compare results before making a new patch. Unfortunately, Upgrade6Test is failing on an error I don't recall seeing before so will take some time.

1) Drupal\Tests\migrate_drupal_ui\Functional\d6\Upgrade6Test::testMigrateUpgradeExecute
Exception: TypeError: Argument 1 passed to Drupal\forum\ForumIndexStorage::updateIndex() must implement interface Drupal\node\NodeInterface, null given, called in /var/www/html/core/modules/forum/forum.module on line 264
Drupal\forum\ForumIndexStorage->updateIndex()() (Line: 93)

The d6 error is happening here

  if ($comment->getCommentedEntityTypeId() == 'node') {
    \Drupal::service('forum.index_storage')->updateIndex($comment->getCommentedEntity());
  }

and is related to comments which is a nuisance as I haven't worked with the forum or the comment migrations before. Anyone have some pointers?

OK, D7 failures are from followupMigrations. So make sure the followup migrations run when the node_master migration is being run.

That fixed Upgrade7Test leaving one failing test, Upgrade6Test.
Anyone keen to test this with Drupal 7, go right ahead. You will have to use the UI for testing.

I have run the failing test, Upgrade6, 3 times over the past few times and it always passes. I do not know why it is failing with the test bot.
I have also run the upgrade manually, setting up the D8 site the same as in the test. Again, the migration complete successfully and the Forum node display correctly with all the comments.
I am going to start a retest, not that I expect anything different. Ideas welcome.

Yicks new disk and rebuild most of dev environment and Upgrade6Test takes way too long, "The test run finished in 1 hour 21 min.".
It does fail locally. Now to run it again an preserve the tables.

Ran the test again and saved the tables. No migrate messages in forum or comment migrations and nothing in watchdog either. Anyone have a suggestion?

Ah, now have a failing migration test for the problem. No solution, just the test.

Trying to fix formatting of list in IS with help from tbradbury

Yes, it was d6_term_node causing the test to fail. This patch should fix it by going back to the way the migration lookups were done in #160. If you read the code the comments are not all updated - I've had a tough day with my local test servers losing their ip address in the middle of running tests.