Migrate Drupal 6 core node translation to Drupal 8

Initial attempt at a patch, affecting d6_node only.

@phenaproxima: sorry I don't have enough migrate experience to tell if the patch makes sense.

What needs to be done is basically merging nodes that have the same tnid under the nid which is the tnid. So if node 4, 5 both have the tnid 3, then node 4 and 5 are to be removed and their content are to be migrated to entity translations under node 3. That sounds "simple" in itself. Where it will get complex is to migrate path aliases, menu items, other entity references and so on and on to the new nid as well. Sites where they may be unsure if they migrated all the things they may want to set up redirects from the old nids to the new nids (their old tnid). Unfortunately Drupal core does not provide redirection functionality to be used. Anyway, the basics are to merge the nodes from multiple copies to the one indicated by tnid as a first step :)

Ah, there is also node access. That has language variance in D8 now which should be used.

Looks like if the language_property would be defined in config schema, we would get better fails. Seems like fails are on that :)

Added schema for the node destination.

I'm back on this. It's still a work in progress, but I think I have it working at a basic level. The wrinkle is that translations are migrated with node revisions, due to the relationship between originals and their translations in D6 and D7. I'd like to change this so that there is no separate migration for revisions -- this patch is a bit of a kludge -- but that's going to take some additional hacking which might as well be done in a follow-up issue. The test coverage also needs to be beefed up significantly. Right now I only have very, very bare-bones assertions.

I apologize for the size of the patch; I had to install the D6 translation modules, and that obviously added a bunch of (necessary) crap to our database fixture.

Bare with me, getting my feet wet with migrate in D8 (done migrate in D7 though). I might have gotten this all wrong.

Config

• Drupal 6 with locale enabled
• Enabled 3 lang.
• Content created by devel generate
• Drupal 8 with language support turned on
• Enabled same 3 lang. in Drupal 8

drush en migrate_upgrade -y
The following extensions will be enabled: migrate_upgrade, migrate_tools
Do you really want to continue? (y/n): y
migrate_tools was enabled successfully.                                                                                                                                               [ok]
migrate_tools defines the following permissions: administer migrations
migrate_upgrade was enabled successfully.                                                                                                                                             [ok]
The Drupal Upgrade UI module has been enabled. Proceed to the upgrade form.                                                                                                           [status]


drush migrate-upgrade --legacy-db-url=mysql://test:test@localhost/d6migrate --legacy-root=http://localhost/migrate/drupal6

exception 'PDOException' with message 'SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd6migrate.content_node_field' doesn't exist' in                                     [error]
/Users/steinmb/sites/d8/drupal/core/lib/Drupal/Core/Database/Statement.php:64
Stack trace:
#0 /Users/steinmb/sites/d8/drupal/core/lib/Drupal/Core/Database/Statement.php(64): PDOStatement->execute(Array)
#1 /Users/steinmb/sites/d8/drupal/core/lib/Drupal/Core/Database/Connection.php(597): Drupal\Core\Database\Statement->execute(Array, Array)
#2 /Users/steinmb/sites/d8/drupal/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php(73): Drupal\Core\Database\Connection->query('SELECT cnf.fiel...', Array, Array)
#3 /Users/steinmb/sites/d8/drupal/core/lib/Drupal/Core/Database/Query/Select.php(481): Drupal\Core\Database\Driver\mysql\Connection->query('SELECT cnf.fiel...', Array, Array)
#4 /Users/steinmb/sites/d8/drupal/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php(179): Drupal\Core\Database\Query\Select->execute()
#5 /Users/steinmb/sites/d8/drupal/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php(218): Drupal\migrate\Plugin\migrate\source\SqlBase->initializeIterator()
#6 /Users/steinmb/sites/d8/drupal/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php(261): Drupal\migrate\Plugin\migrate\source\SourcePluginBase->getIterator()
#7 /Users/steinmb/sites/d8/drupal/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/CckMigration.php(32): Drupal\migrate\Plugin\migrate\source\SourcePluginBase->rewind()
#8 /Users/steinmb/sites/d8/drupal/core/modules/migrate/src/MigrationBuilder.php(53): Drupal\migrate_drupal\Plugin\migrate\builder\d6\CckMigration->buildMigrations(Array)
#9 /Users/steinmb/sites/d8/drupal/modules/migrate_upgrade/src/MigrationCreationTrait.php(67): Drupal\migrate\MigrationBuilder->createMigrations(Array)
#10 /Users/steinmb/sites/d8/drupal/modules/migrate_upgrade/src/MigrateUpgradeDrushRunner.php(39): Drupal\migrate_upgrade\MigrateUpgradeDrushRunner->createMigrations(Array,
'http://localhos...')
#11 /Users/steinmb/sites/d8/drupal/modules/migrate_upgrade/migrate_upgrade.drush.inc(40): Drupal\migrate_upgrade\MigrateUpgradeDrushRunner->configure()
#12 [internal function]: drush_migrate_upgrade()
#13 /Users/steinmb/.composer/vendor/drush/drush/includes/command.inc(359): call_user_func_array('drush_migrate_u...', Array)
#14 /Users/steinmb/.composer/vendor/drush/drush/includes/command.inc(210): _drush_invoke_hooks(Array, Array)
#15 [internal function]: drush_command()
#16 /Users/steinmb/.composer/vendor/drush/drush/includes/command.inc(178): call_user_func_array('drush_command', Array)
#17 /Users/steinmb/.composer/vendor/drush/drush/lib/Drush/Boot/BaseBoot.php(62): drush_dispatch(Array)
#18 /Users/steinmb/.composer/vendor/drush/drush/drush.php(70): Drush\Boot\BaseBoot->bootstrap_and_dispatch()
#19 /Users/steinmb/.composer/vendor/drush/drush/drush.php(11): drush_main()
#20 {main}

Oh, forget #15 bumped into a small issue. For now you need to have CCK enabled in D6 before running. I did run and created content type "Story" and mange to tag the right language to them.

Dump of run

Initialized Drupal 8.0.0-dev root directory at /Users/steinmb/sites/d8/drupal [notice]
Initialized Drupal site default at sites/default [notice]
Executing: mysql --defaults-extra-file=/private/tmp/drush_V47xG0 --database=d8 --host=localhost --port=3306 --silent < /private/tmp/drush_mCD7kM
Executing: mysql --defaults-extra-file=/private/tmp/drush_5Xw2Mj --database=d8 --host=localhost --port=3306 --silent < /private/tmp/drush_aAuXw9
Upgrading d6_date_formats
Upgrading d6_dblog_settings
Upgrading d6_file_settings
Upgrading d6_imagecache_presets
Migration failed with source plugin exception: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd6migrate.imagecache_preset' doesn't exist: SELECT icp.* [error]
FROM
{imagecache_preset} icp; Array
(
)

Upgrading d6_search_page
Upgrading d6_search_settings
Upgrading d6_system_cron
Upgrading d6_system_date
Upgrading d6_system_file
Upgrading d6_system_image
Upgrading d6_system_image_gd
Upgrading d6_system_logging
Upgrading d6_system_maintenance
Upgrading d6_system_performance
Upgrading d6_system_rss
Upgrading d6_system_site
Upgrading d6_upload_field
Upgrading d6_user_mail
Upgrading d6_user_settings
Upgrading menu
Upgrading menu_settings
Upgrading taxonomy_settings
Upgrading text_settings
Upgrading d6_filter_format
Upgrading block_content_type
Upgrading block_content_body_field
Upgrading d6_custom_block
Upgrading d6_user_role
Upgrading d6_block
Upgrading d6_file
Upgrading d6_user_picture_file
Upgrading user_picture_field
Upgrading user_picture_field_instance
Upgrading user_picture_entity_display
Upgrading user_picture_entity_form_display
Upgrading d6_user
Upgrading d6_node_type
Upgrading d6_node_settings
Upgrading d6_field
Upgrading d6_field_instance
Upgrading d6_field_instance_widget_settings
Upgrading d6_view_modes
Upgrading d6_field_formatter_settings
Upgrading d6_node__page
Upgrading d6_node__story
Upgrading d6_comment_type
Upgrading d6_comment_field
Upgrading d6_comment_field_instance
Upgrading d6_comment_entity_display
Upgrading d6_comment_entity_form_display
Upgrading d6_comment
Upgrading d6_comment_entity_form_display_subject
Upgrading d6_node_revision__page
Upgrading d6_node_revision__story
Upgrading d6_node_setting_promote
Upgrading d6_node_setting_status
Upgrading d6_node_setting_sticky
Upgrading d6_taxonomy_vocabulary
Upgrading d6_taxonomy_term
Upgrading d6_vocabulary_field
Upgrading d6_vocabulary_field_instance
Upgrading d6_vocabulary_entity_display
Upgrading d6_vocabulary_entity_form_display
Upgrading d6_user_contact_settings
Upgrading d6_menu_links

I have not found time to look at the data structures we created, but looking at the node UI I think we missed something when we exported/defined the content type:

I've actually run into the checkbox thing before and even mentioned it in IRC but I failed in my job of creating an issue. Here it is #2572897: Promote and sticky options migrate to empty checkboxes

Removed rouge images from issue summary.

-mike

Tested this again with patch #3 from #2572897: Promote and sticky options migrate to empty checkboxes and the checkboxes now looked OK. YAY!

Done some more monkeywork. Created two nodes (story, page) with a translation (D6). The content of the translated nodes never make it into drupal 8 fields from what I can tell. Only the node that tnid points to.

I also feel that we should pick up the content type translation settings from D6 and set them in D8. I would think that people would expect that to be done.

Let me also include a raw dump on a node that should contain a translation:

drush eval 'print_r(node_load(26)); '
Drupal\node\Entity\Node Object
(
    [in_preview] =>
    [values:protected] => Array
        (
            [vid] => Array
                (
                    [x-default] => 26
                )

            [langcode] => Array
                (
                    [x-default] => en
                )

            [revision_timestamp] => Array
                (
                    [x-default] => 1442957896
                )

            [revision_uid] => Array
                (
                    [x-default] => 1
                )

            [revision_log] => Array
                (
                    [x-default] =>
                )

            [nid] => Array
                (
                    [x-default] => 26
                )

            [type] => Array
                (
                    [x-default] => page
                )

            [uuid] => Array
                (
                    [x-default] => afcebf46-5bd3-400d-b900-eeae7446c3ff
                )

            [isDefaultRevision] => Array
                (
                    [x-default] => 1
                )

            [title] => Array
                (
                    [x-default] => English: Secundum Os Exputo Tego Acsi Nisl
                )

            [uid] => Array
                (
                    [x-default] => 0
                )

            [status] => Array
                (
                    [x-default] => 1
                )

            [created] => Array
                (
                    [x-default] => 1442345475
                )

            [changed] => Array
                (
                    [x-default] => 1442957896
                )

            [promote] => Array
                (
                    [x-default] => 0
                )

            [sticky] => Array
                (
                    [x-default] => 0
                )

            [revision_translation_affected] => Array
                (
                    [x-default] => 1
                )

            [default_langcode] => Array
                (
                    [x-default] => 1
                )

            [content_translation_source] => Array
                (
                    [x-default] => und
                )

            [content_translation_outdated] => Array
                (
                    [x-default] => 0
                )

            [body] => Array
                (
                    [x-default] => Array
                        (
                            [0] => Array
                                (
                                    [value] => English: node (page) - Iusto jus probo consequat. Voco vereor luctus validus sed. Refero voco nisl validus letalis abluo abigo saepius.


                                    [summary] => English: node (page) - Iusto jus probo consequat. Voco vereor luctus validus sed. Refero voco nisl validus letalis abluo abigo saepius.


                                    [format] => filtered_html
                                )

                        )

                )

        )

    [fields:protected] => Array
        (
        )

    [fieldDefinitions:protected] =>
    [languages:protected] =>
    [langcodeKey:protected] => langcode
    [defaultLangcodeKey:protected] => default_langcode
    [activeLangcode:protected] => x-default
    [defaultLangcode:protected] => en
    [translations:protected] => Array
        (
            [x-default] => Array
                (
                    [status] => 1
                )

        )

    [translationInitialize:protected] =>
    [newRevision:protected] =>
    [isDefaultRevision:protected] => 1
    [entityKeys:protected] => Array
        (
            [bundle] => page
            [id] => 26
            [revision] => 26
            [uuid] => afcebf46-5bd3-400d-b900-eeae7446c3ff
        )

    [translatableEntityKeys:protected] => Array
        (
            [label] => Array
                (
                    [x-default] => English: Secundum Os Exputo Tego Acsi Nisl
                )

            [langcode] => Array
                (
                    [x-default] => en
                )

            [status] => Array
                (
                    [x-default] => 1
                )

            [uid] => Array
                (
                    [x-default] => 0
                )

            [default_langcode] => Array
                (
                    [x-default] => 1
                )

        )

    [validated:protected] =>
    [validationRequired:protected] =>
    [entityTypeId:protected] => node
    [enforceIsNew:protected] =>
    [typedData:protected] =>
    [cacheContexts:protected] => Array
        (
        )

    [cacheTags:protected] => Array
        (
        )

    [cacheMaxAge:protected] => -1
    [_serviceIds:protected] => Array
        (
        )

)

@phenaproxima: you don't need any data in locales_* tables for node migration, that should cut down on the patch size definitely.

This is a patch with a drupal6 fixture based on a basic multilingual site and dumped with db-tools.php after #2568203: Remove migrate-db.sh in favor of core tools

The configuration/content changes are:

1. add new language spanish
2. set default language spanish
3. set story content type to multilingual support "enabled with translation"
4. add source node in english
5. add source node in spanish
6. add translations for both source nodes
7. add revision for one of the translations
8. truncate locale_* as for #24

Looking at the patch there are non-related changes in menu-links, which I did not edit. Is that normal?

I also did a bit of manual testing:

1. Installed a new D8 site and enabled some required modules drush -y si --account-pass=admin && drush -y en migrate_upgrade migrate_plus language content_translation telephone locale config_translation
2. Added the spanish language
3. Run the migration with the new dump drush migrate-upgrade --legacy-db-url=mysql://root:root@localhost/d6_dump --legacy-root=http://d6.dev

No errors except for a Missing filter plugin: filter_null, but the migration ignored (as expected?) all multilingual settings and translations

1. Spanish was not set as default language
2. Story multilingual settings were not migrated: both Show language selector on create and edit pages and Enable translation were disabled
3. Translations were imported as new nodes

Discussed with @phenaproxima. Given the self-contained nature of Migrate, the fact that it is marked experimental, and the fact that having this available to test would be a huge win, I think this is worth considering as an rc target.

Don't want to bikeshed this issue but in #16 I've noticed the following, which I could reproduce with RC1:

Upgrading d6_imagecache_presets
Migration failed with source plugin exception: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd6migrate.imagecache_preset' doesn't exist: SELECT icp.* [error]
FROM
{imagecache_preset} icp; Array
(
)

It doesn't seem to me that we're addressing that issue here and I couldn't find any other existing issue referencing this failure. Have I overlooked something? If not, I'm happy to file a new issue to address just that.

@anavarre. That is in #2558927: d6_imagecache_presets reports failure for missing table.

Using the new "rc target triage" tag - although I thought as migrate was experimental the RC target process should not apply.

A colossal new patch to test with! This implements basic translation migrations for Drupal 6 nodes, with tests -- the translations are migrated along with the node revisions.

The size of the patch is due to the addition of test data. I decided to leave the locale table in there as-is, because the point of Migrate's fixtures is to provide fully working databases that you can quickly spin up real sites to work on (so I'd rather not remove arbitrary pieces of the fixture, even if a particular patch doesn't need them).

Fixed failing unit tests. Let's hope I don't have to re-roll that patch ever again.

Nice to see this issue moving again :) That is one scary patch! Trying to help out doing some monkey work. Is this patch supposed to also migrate the translation setting pr. content type?

If so, it is not getting set correctly:

Sorry but still not able to get to work:

Config

• Drupal 6 with locale enabled
• Enabled 3 lang.
• Content created by devel generate
• Drupal 8 with language support turned on
• Enabled same 3 lang. in Drupal 8

There seems to be something missing on the created nodes in D8. Non of the nodes created with migrate can I get the translation tab to show. Resaving the node does not help. Locally created nodes work OK.

Only the data from origin (tnid) get imported. This is a node that in D6 have a translation (two nodes). This is what I dug out of the db after migration. Was expecting to have two entries in node__body field.

SELECT entity_id, langcode FROM node__body WHERE entity_id=26;
+-----------+----------+
| entity_id | langcode |
+-----------+----------+
|        26 | en       |
+-----------+----------+
1 row in set (0.00 sec)

MariaDB [d8]> SELECT * FROM node WHERE nid=26;
+-----+------+------+--------------------------------------+----------+
| nid | vid  | type | uuid                                 | langcode |
+-----+------+------+--------------------------------------+----------+
|  26 |   26 | page | 868a9ea1-1710-4996-be00-bef16955848e | en       |
+-----+------+------+--------------------------------------+----------+
1 row in set (0.00 sec)

Migration run:

drush migrate-upgrade --legacy-db-url=mysql://test:test@localhost/d6migrate --legacy-root=http://localhost/migrate/drupal6
Upgrading d6_date_formats
Upgrading d6_dblog_settings
Upgrading d6_default_language
get_object_vars() expects parameter 1 to be object, null given Callback.php:34                                                                [warning]
Upgrading d6_file_settings
Upgrading d6_imagecache_presets
Migration failed with source plugin exception: SQLSTATE[42S02]: Base table or view not found: 1146 Table                                      [error]
'd6migrate.imagecache_preset' doesn't exist: SELECT icp.*
FROM
{imagecache_preset} icp; Array
(
)

Upgrading d6_search_settings
Upgrading d6_system_cron
Upgrading d6_system_date
Upgrading d6_system_file
Upgrading d6_system_image
Upgrading d6_system_image_gd
Upgrading d6_system_logging
Upgrading d6_system_maintenance
Upgrading d6_system_performance
Upgrading d6_system_rss
Upgrading d6_system_site
Upgrading d6_update_settings
Upgrading d6_upload_field
Upgrading d6_user_mail
Upgrading d6_user_settings
Upgrading menu
Upgrading menu_settings
Upgrading search_page
Upgrading taxonomy_settings
Upgrading text_settings
Upgrading d6_filter_format
Upgrading block_content_type
Upgrading block_content_body_field
Upgrading d6_custom_block
Upgrading d6_user_role
Upgrading d6_block
Upgrading d6_file
Upgrading d6_user_picture_file
Upgrading user_picture_field
Upgrading user_picture_field_instance
Upgrading user_picture_entity_display
Upgrading user_picture_entity_form_display
Upgrading d6_user
Upgrading d6_node_type
Upgrading d6_node_settings
Upgrading d6_field
Upgrading d6_field_instance
Upgrading d6_field_instance_widget_settings
Upgrading d6_view_modes
Upgrading d6_field_formatter_settings
Upgrading d6_node__page
Upgrading d6_node__story
Upgrading d6_comment_type
Upgrading d6_comment_field
Upgrading d6_comment_field_instance
Attempt to create a field comment that does not exist on entity type node.                                                                    [error]
(/Users/steinmb/sites/d8/drupal/core/modules/field/src/Entity/FieldConfig.php:291)
Attempt to create a field comment that does not exist on entity type node.                                                                    [error]
(/Users/steinmb/sites/d8/drupal/core/modules/field/src/Entity/FieldConfig.php:291)
Upgrading d6_comment_entity_display
Upgrading d6_comment_entity_form_display
Upgrading d6_comment
Upgrading d6_comment_entity_form_display_subject
Upgrading d6_node_translation_settings
Upgrading d6_node_revision__page
Upgrading d6_node_revision__story
Upgrading d6_node_setting_promote
Upgrading d6_node_setting_status
Upgrading d6_node_setting_sticky
Upgrading d6_taxonomy_vocabulary
Upgrading d6_taxonomy_term
Upgrading d6_vocabulary_field
Upgrading d6_vocabulary_field_instance
Upgrading d6_vocabulary_entity_display
Upgrading d6_vocabulary_entity_form_display
Upgrading d6_user_contact_settings
Upgrading d6_menu_links

I've spent several frustrating days trying to get this to work.

What has become painfully clear is that trying to implement i18n support in a single mega-patch is unworkable and unsustainable. There is a lot of foundational work that needs to be done before node translations -- or indeed, translations of anything -- can be migrated. So I'm wontfixing this issue, but will file follow-ups so that I can submit smaller patches to build up i18n support in logical, bite-sized chunks.

All related follow-ups will be tagged with i18n-migrate.

For reference, the issues mentioned by @phenaproxima would be at https://www.drupal.org/project/issues/search?issue_tags=i18n-migrate -- I understand it may not be practical to connect all of them back here, but it would be nice to mark this as related issue there, so people can find the broken out issues. I updated #2594263: Add translation data to Migrate Drupal's test fixtures with appropriate related issue and tags.

Re-opening, but postponing until the groundwork has been laid for migrating node translations, and only node translations.

Today the D8 core committers agreed that patches that only change "Experimental" functionality (like Migrate) can be treated as "rc eligible" rather than rc targets. Reference: https://www.drupal.org/core/d8-allowed-changes#rc

@phenaproxima: now that #2594263: Add translation data to Migrate Drupal's test fixtures landed, should this be reopened?

Also #2596793: Migrate legacy languages to configurable language entites landed, so I think this should be good to unpostpone?

Removing from D8MI sprint due to inactivity.

Adding parent.

There is a lot of foundational work that needs to be done before node translations -- or indeed, translations of anything -- can be migrated

@phenaproxima, will you expand on this comment you made in #38. Specifically, what groundwork are you referring to and in what order to do that work.
Thx.

In #38 phenaproxima referred to this patch as a mega-patch. The large size is because it contains a D6 test fixture with an additional language. The work on the fixture was moved to #2594263: Add translation data to Migrate Drupal's test fixtures. Since that has been committed it can be removed from this patch.

Also, this patch includes the migration of the default_language variable which is an issue in itself, albeit a small one, but it has it's own scope. It is now #2657978: Variable to config: language_default [D6 & D7].

So, that should reduce the size of this patch and narrow the scope to migrating nodes.

This patch removes drupal6.php, d6_default_language.yml and MigrateDefaultLanguageTest.php. It still needs to be rerolled.

Moved the migration of the node translation settings to #2660028: Migrate node translation settings . This patch removes the files for that test and it still needs to be rerolled.

Turns out the test fixture in #35 is needed. So it is back in but it made the reroll difficult.

Looks like I uploaded the wrong patch.

This should be the right patch.

That's better.

Both EntityContentBaseTest and EntityRevisionTest fail because getEntityTypeId is no longer static.

-  protected static function getEntityTypeId($plugin_id) {
+  protected function getEntityTypeId() {
  

I tried to fix the tests but get this error, "array_keys() expects parameter 1 to be array, null given".

I'd like to work on field-translated content from D7. I have code for a completely different approach here: https://github.com/vasi/migrate_field_translated . But I'd like to try the approach you're using here—so would it be possible to split out just the MigrateDestination work from this? I think a less mega-patch approach would work better, anyhow.

@vasi, I'm too new at this to give a definitive answer. However, in my opinion, since this issue is for D6 nodes, the D7 field-translated content should be a separated issue.

Hoping this patch will fix the errors on EntityContentBaseTest.

@vasi: is that independently testable, etc? Then it would be nice to cut down on this patch and get a step in IMHO.

for testbot

I think it should be possible to test just the destination, yes. Just create a destination, create a Row manually, ask the destination to import() the Row, see what happens.

I'm a little uncertain about the current design of the destination, though. We assign the same destination ID to two separate input rows—is that considered ok? This means they can't be rolled-back independently of one another; the 'items created' report from migrate_tools can be misleading; the result of an import can be order-dependent... Maybe we should make the IDs a tuple of (entity_id, langcode) instead?

@vasi: I think the same id makes the translation mapping to the same node possible? I am not well versed enough in migrate to know if this is just an interim mapping, but if its just internal, we should be able to make it more granular, sure!

For context, have look back at #37 and #38 where we originally gave up on this mega patch. Config was split out and is now in, but it is still big and hard to test.

Add parent issue.

This will fix the problem in the previous patch and expose a new one, in the Unit test, EntityRevisionTest. Hopefully, the testbot will report the same error.

What I get is this:

PHP Fatal error:  Call to a member function loadRevision() on null in /opt/sites/d8/core/modules/migrate/src/Plugin/migrate/destination/EntityRevision.php on line 47
PHP Stack trace:
PHP   1. {main}() /opt/sites/d8/vendor/phpunit/phpunit/phpunit:0
PHP   2. PHPUnit_TextUI_Command::main() /opt/sites/d8/vendor/phpunit/phpunit/phpunit:47
PHP   3. PHPUnit_TextUI_Command->run() /opt/sites/d8/vendor/phpunit/phpunit/src/TextUI/Command.php:100
PHP   4. PHPUnit_TextUI_TestRunner->doRun() /opt/sites/d8/vendor/phpunit/phpunit/src/TextUI/Command.php:148
PHP   5. PHPUnit_Framework_TestSuite->run() /opt/sites/d8/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:436
PHP   6. PHPUnit_Framework_TestCase->run() /opt/sites/d8/vendor/phpunit/phpunit/src/Framework/TestSuite.php:735
PHP   7. PHPUnit_Framework_TestResult->run() /opt/sites/d8/vendor/phpunit/phpunit/src/Framework/TestCase.php:705
PHP   8. PHPUnit_Framework_TestCase->runBare() /opt/sites/d8/vendor/phpunit/phpunit/src/Framework/TestResult.php:601
PHP   9. PHPUnit_Framework_TestCase->runTest() /opt/sites/d8/vendor/phpunit/phpunit/src/Framework/TestCase.php:749
PHP  10. ReflectionMethod->invokeArgs() /opt/sites/d8/vendor/phpunit/phpunit/src/Framework/TestCase.php:889
PHP  11. Drupal\Tests\migrate\Unit\destination\EntityRevisionTest->testGetEntityDestinationValues() /opt/sites/d8/vendor/phpunit/phpunit/src/Framework/TestCase.php:889
PHP  12. Drupal\Tests\migrate\Unit\destination\EntityRevision->getEntity() /opt/sites/d8/core/modules/migrate/tests/src/Unit/destination/EntityRevisionTest.php:70
PHP  13. Drupal\migrate\Plugin\migrate\destination\EntityRevision->getEntity() /opt/sites/d8/core/modules/migrate/tests/src/Unit/destination/EntityRevisionTest.php:206

Staring at it and knowing the changes, I guessed that it had to do with storage. This test class prophesized storage but I'm not sure how the parent can make the entity when it doesn't have any storage. It's still not clear to me how the original code works. Anyway, I added storage back into the constructor and the test passed. Good. But other tests failed so not so good.

So, I restored the code to this patch. After some debugging I realized that creating the revision fails in getEntityTypeId. That was changed from static to protected in an earlier patch by phenaproxima. So, the next thing I'd like to do is make sure that the revision is created in the test. Not sure how to do that.

While this could be made to pass easily, after all, it's just one test that fails, I completely fail to see what and why is going on here. I am sorry but this is a kitten killer and should be trimmed back if possible. What's the minimal version of this patch that would pass? Is there anything besides the changes in Node and NodeRevision necessary? All the entity destination factory and constructor method changes , I doubt they are relevant. Also, the change in Node / NodeRevision looks atrocious (sorry) just put the "// Do not include translations." into an addTranslationConditions method in Node, override in NodeRevision with a method with an empty body and be done with it. I wonder just those changes, updateEntity in EntityContentBase plus tests, how would it fare.

Also, a merge has failed in MigrateNodeTest .

Let's see this.

That's not enough. I meant this :)

Alrighty, let's see this one. I attached a do not test patch without the fixture changes for easier review. Edit: git diff HEAD `git diff HEAD --name-only|grep -v fixtures` > 2225775_76-do-not-test.patch

Unfortunately did not pass. Some review comments:

1. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
   @@ -256,4 +260,17 @@ public function getIds() {
   +  protected function addTranslationConditions(SelectInterface $query) {
   +    // Do not include translations.
   +    $condition = (new Condition('OR'))
   +      ->where('n.tnid = n.nid')
   +      ->condition('n.tnid', 0);
   +    $query->condition($condition);
   
   +++ b/core/modules/node/src/Plugin/migrate/source/d6/NodeRevision.php
   @@ -42,4 +55,11 @@ public function getIds() {
   +  protected function addTranslationConditions(SelectInterface $query) {
   +    // Translations are included in the selection by default.
   

   Would be good to explain why are translation revisions migrated while translated nodes themselves are not? That is basically because we merge nodes along their tnid, so not all nodes are to be created but that would be good to document on the Node class.

2. +++ b/core/modules/node/src/Plugin/migrate/source/d6/NodeRevision.php
   @@ -19,7 +22,17 @@ class NodeRevision extends Node {
   -  const JOIN = 'n.nid = nr.nid AND n.vid <> nr.vid';
   +  const JOIN = 'n.nid = nr.nid';
   

   Why is the vid condition removed?

Minor bugfix.

Why is the vid condition removed?

I think this is so we don't exclude current translations! Eg, suppose I have a translated node:

We turn this into one row from the d6_node source, corresponding to just the English translation (nid 1). Then we produce two d6_node_revision rows, one for the English translation (nid 1, vid 1), and one for the French one (nid 2, vid 2). If we excluded revisions with node.vid = node_revisions.vid, then we would never see the French translation.

However, there are still some issues here. For one thing, the EntityRevision destination is setting $entity->isDefaultRevision(FALSE);, which we probably don't want in the case of current revisions. Maybe it doesn't have any consequences though?

The patch looks mostly good to me, but I'm a little worried by the result of our test migrations (from MigrateNodeRevisionTest). If I look at the initial D6 DB and the resulting D8 one, here's what I see:

Notice that the current revision in node_field_data is 12, even though revision 13 exists? And we have a fourth revision entry (vid 13 in english). I wonder what happens if we happen to get the node_revisions entries in a different order, do we still get ok results?

And now I've added a couple of assertions to test the problem.

What do you think the behavior should be for importing content translation, anyhow? It seems a bit suboptimal for each D8 revision to carry just one language—anybody who reverts to a previous revision will probably be surprised at losing translations. Maybe we can copy in previous translations into subsequent revisions.

Yeah ideally all revisions would have all languages and only the ones changed/added there would be different. I am not sure how complicated that is to implement.

Right, thanks Gábor Hojtsy. Lets see if I understand.

Say there are two languages in D6. A given node N has 5 revisions for language A and 20 for language B. If I understand correctly, in D8 that node would have 20 revisions with all languages. But, since there isn't a one to one correspondence of revisions, what happens to the 5 revisions for language A?

Lets assume that the first and last revision for node N in both languages would go into the first and last revision in D8. What about the second revision for language A? Does it go with the second revision of language B or another revision? How would one know? What assumptions can be made?

Thanks for the reply, quietone! I definitely appreciate the work you're putting in here. My assumptions were different in this case, though. I think if there's eg: 20 english revisions and 5 french ones, that should be 25 revisions overall. It corresponds better with the usual way people edit nodes: one language at a time. So my solution would look like this:

• For the revision destination, instead of creating a new revision based on the current version of the entity, we'd create it based on the previous revision, so we keep the translations as they were at that time.
  • We still have to import the current revision as part of the migration, so that it will have all languages. We'd have to special case it so that we set isDefaultRevision(TRUE).
  • The process I described does make the migration order-dependent, which is unfortunate. I guess we'd add a "SORT BY vid ASC"? But even so, it could get weird if someone imported one revision before its preceding ones, eg: with --idlist.
  • We're also assuming here that unusual changes haven't taken place, like nodes being removed or added to translation sets. As far as I know, there's no way to tell whether this happened, so I think it's fair to assume typical use. Thoughts?
  • We could be fancy and assume that two D6 revisions with different languages but the same timestamp really belong in the same D8 revision. But then we'd have the same destination ID for two rows...ugh.
• For the node source, we'd need to get the maximum vid for each translation set, so we don't accidentally end up with the wrong default revision. One way to do this is calculate a translation-set number, eg: "CASE tnid WHEN 0 THEN nid ELSE tnid END as tset". Then add "GROUP BY tset", and "MAX(vid) as vid".

I think our test data needs some improvements for this, too. I'd like to see more revisions, more than two languages, and also fields in multiple languages. I'm not really sure what the process is for adding this, though.

I'm also a little concerned about what will happen with other translated migrations, that aren't D6. We'll want to make sure that any D6-specific strategy we come up with doesn't adversely effect those. The major ones are probably:

* D7 content-translation, which should be very similar to D6.
* D7/D8 field-translation. This has the problem that we'll have multiple source rows with the same nid/vid. I guess we could add a third migration for each bundle, with source IDs (vid, language) or something? Ick.
* Other sources that may not even have a concept of revisions, like manually-editing CSV, or other CMSes. So it would be best if we didn't depend on revisions existing in the source to import multiple languages.

I don't know if this is the best place to discuss all of that, maybe I'll use the META issue. But any thoughts would be appreciated.

I think if there's eg: 20 english revisions and 5 french ones, that should be 25 revisions overall.

Like this?

For the revision destination, instead of creating a new revision based on the current version of the entity, we'd create it based on the previous revision, so we keep the translations as they were at that time.

What?

I think our test data needs some improvements for this, too. I'd like to see more revisions, more than two languages, and also fields in multiple languages. I'm not really sure what the process is for adding this, though.

First, let me state that working with the test fixtures (dumps) is, hmm, tedious. There is Generating database fixtures for D8 Migrate tests, which needs updating. For now, I think a few more revisions would be helpful. But anything more would be noise and distracting. Small steps first.

Referring to the test results in #81, for D8 node_field_data. On D8 I see that the node_field_data table has an entry per node per language. So, there should be another one, node 9, vid 12, langcode fr.

Hmm, more like this:

Does that kinda make sense? Each revision updates one translation, and keeps the others the same.

@vasi, thx for helping to realize my misunderstanding about version ids.
Just did a simple MigrateNodeRevision Test and the results in node_field_data and node_field_revision are what I expected. Looking good.

I'm inclined to get this passing tests and then get some feedback from others. I'd rather not change the test fixture at this point.

A fix for one Unit test. I'm not fluent enough in unit testing to fix the other one. I'll try again later.

I think I may have mentioned this in IRC, but I'm not inclined to "fix" the test for EntityRevision. I think it's sub-optimal to change the behavior of all revision destinations just for the one case of tnid-based content translation. The destination that I wrote also depends on order of import, which is also not great.

Instead, I think we should have a source and destination that use a key of tuple(vid, langcode). Then we just need to make our revision source for D6 produce potentially multiple rows per vid. That means we want a query something like this:

SELECT
  tvids.vid as tvid,
  tvids.language,
  rvid.*
FROM node_revisions rvid
JOIN (
  SELECT
    nr.vid,
    nt.language,
    max(nrt.vid) AS tvid
  FROM node_revisions nr
  JOIN node n ON nr.nid = n.nid
  JOIN node nt ON (
      (n.tnid <> 0 AND n.tnid = nt.tnid)
    OR
      n.nid = nt.nid
  )
  JOIN node_revisions nrt ON nt.nid = nrt.nid AND nrt.vid <= nr.vid
  GROUP by nr.vid, nt.language
) AS tvids ON rvid.vid = tvids.tvid;

Yes, that's kinda long, but not the worst thing I've ever seen. Should be portable and efficient for any DB engine.

Re-rolled, fixed conflicts.

Here's an attempt at the strategy I suggested in #96. I also added some more test coverage, including partial imports (which would break with the previous approach).

Fixed the tests, I hope.

I tested it out on an actual D6 site, and it went well. Except that the translation of the default revision wasn't being populated, because EntityRevision always calls isDefaultRevision(FALSE). It works better if I change EntityRevision to check if it's the default revision or not.

An alternative would be migrating translations in the node migration, and making node revisions always not the default. But then we'd need a two-column source/destination ID for Node, which would make the migration process hard to use—so I prefer just making EntityRevision check for defaults.

The strategy explained in #89 sounds good to me.

@vasi, this looks really good. Just a few comments/questions.

+++ b/core/modules/node/src/Tests/Migrate/d6/MigrateNodeRevisionTest.php
@@ -38,12 +49,80 @@ public function testNodeRevision() {
+   * Test partial node revision migrations from Drupal 6 to 8.

I'm all for tests but what is the use case here? Why do we need to test for partial node revision migrations?

+++ b/core/modules/node/src/Tests/Migrate/d6/MigrateNodeRevisionTest.php
@@ -38,12 +49,80 @@ public function testNodeRevision() {
+    // The English title and body did not change in this revision...
...
+    // ...but a French translation was introduced.

The comments make perfect sense to me and are easy to read, but are ellipsis allowed in comments? And if they are, shouldn't there be a space before and after it?

Since the addition of translations has increased the complexity of both the Node and NodeRevision source plugins, particularly the latter, can the respective class doc block include some explanation of what it is doing?

Finally, the queries in Node.php and NodeRevision.php really need a more practiced eye to look at them.

Hey, sorry for the delay, and thanks for the reviews. I realize I haven't given a very complete explanation of what this patch does now, and why. So here's a start.

Let's look at an example, suppose we have a node that started in English. Then it was translated into French, changed a little in English, and finally translated into German. Our D6 data would look like this:

In the old version of the patch, the source would generate four rows, one per revision:

This is simple, which is good. But then the EntityRevision destination had to do special processing to find the previous revision, and bring its translations along. This had a few downsides:

1. The EntityRevision destination is no longer generic. It's specialized for D6-to-D8 content translation, but the destination shouldn't really care where the data came from—that's the source's responsibility. And anyone who inherits from EntityRevision could be very surprised at the new behaviour.
2. This approach is not easily generalized to other translated migrations, eg: D7-field-translation-to-D8, or D8-to-D8, or any non-Drupal source. These data sources allow multiple translations to be updated in the same revision, so we can't generate the one-translation-per-revision input that EntityRevision expects.

3. The migration becomes fragile—it breaks if revisions are migrated out of order, or if one revision is skipped. There are a bunch of ways this could happen:

   • • The user specifies the --idlist parameter to drush migrate-import, which then imports just some revisions.
   • • A hook_migrate_prepare_row() implementation intentionally skips some rows, eg: if the user wants to not migrate revisions that are too old. Similarly for skip_on_empty or skip_row_if_not_set processes.
   • • An exception is thrown while importing a row, causing it to be skipped.

   In any of these cases, when a revision row is skipped, all subsequent revisions would not be able to find it. They would be migrated without the content from that revision, so we could end up with nodes missing entire translations, or subtly broken data.

   This is why I wanted to test "partial [..] revision migrations". I don't think we want recommend this use-case to users! But it's not hard for someone to end up there, intentionally or not, so we need to make sure it doesn't Break All The Things.

The new version of this patch tries to get rid of these issues. Instead of our source generating one row per revision, we now generate multiple rows. Each row corresponds to one translation that existed at the time of that revision. So in our example, we'd generate this:

As you can see, this is a bit more complex. The unique ID is no longer just 'vid', but a tuple of ('vid', 'language'). But now we avoid the problems from before:

1. EntityRevision goes back to being completely generic.
2. It would be quite easy to generate rows like this from field-translated content.
3. If some revisions are skipped, whether intentionally or not, the behaviour is still reasonable.

I hope this makes more sense now. I admit my code could use some more explanation too, so I'll go over it again and try to add that, especially for the DB queries.

Thanks @quietone for catching the comment formatting issue, I'll give that a shot too.

This will need a complicated reroll will all the changes that happened, e.g. migrations to plugins and kernel test conversions.

That said, If you're like me and don't care about all the revisions stuff but just want to import the latest revision, all you need from the patch is this part: https://gist.github.com/Berdir/9ac6aa8c7bc4f2fbc287ac6a3512c17d.

And then set up the nid/tnid mapping like this: https://gist.github.com/mikeryan776/2bd2bbf6c17bd09abcb74422e280288f (thanks @mikeryan!). Note that field_whatever is nid, A is tnid and B is nid. Also, you very likely want to remove vid from the mappings and let it generate a new one, otherwise translations might get messed up.

That worked quite nicely for me. One thing I noticed is that content_translation_source won't be imported like that, but I think it won't be set either with the full patch? I've seen problems caused by a missing source language in the field synchronizer code.

First go at a reroll.

Berdir, if you have any nodes that are translated you'll also need to turn track_changes on. Otherwise migrate will just skip the rows for the second language.

I looked at the failing tests:

• testTermRevisionNode: Failing because languages aren't configured, after I configure them it works
• testMigrateUpgrade (D6, field_config): We're apparently adding a field company.upload, not sure why. @quietone, I think it was introduced in one of your patches, any idea? Anyhow, it's perfectly legitimate, so we can just increment the expected count of fields.
• testMigrateUpgrade (D6, source row status): I think this is because of the languages not being configured. I'll see if I can reroll and review #2225271: Migrate content type language settings from Drupal 6 & 7
• testMigrateUpgrade (D7): No idea yet, it seems totally unrelated. Why would this happen in only D7?

Also had to reroll because of conflicts, so I don't have an interdiff.

Discussed with @alexpott, @weal, @benjifisher, and @mikeryan. #1952044: Migration path for legacy content translation module data is probably a duplicate of this issue. From that issue:

Beware: other related data such menu items, taxonomy term relations, node access rules, path aliases, visitor stats, etc. will be affected, and would need to be taken care of. This is likely what makes the task *huge*!

There is also a potential problem with links in e.g. the body field. If you have:

<a href="/node/5">Link!</a>

And node 5 was a translation of node 3 and now it's node 3. All your links like the above are now broken.

So we should create followups for all of those tasks.

@vasi, thanks for adding the comments, it really helps.

Regarding the points in #113.

• company.upload: Yes, it looks like I introduced some changes and didn't document the reason at the time. I think I recall why but until I can prove it is probably prudent to remove it. I'll give myself another day to recall why before removing it.
• testMigrateUpgrade D6): I added the language module and tweaked the entity counts and it passes. But, how to confirm that the entity counts are correct?
• testMigrateUpgrade (D7): When running this test the d7_user migrate map contains this error 'Invalid translation language () specified. (/opt/sites/d8/core/lib/Drupal/Core/Entity/ContentEntityBase.php:823)'.

This patch is expected to fail on MigrateUpgrade7Test.

Failed as expected. I still don't see the solution, nor why this is only occurs in D7.

The error msg from migrate_message_d7_user table:
Invalid translation language () specified. (/opt/sites/d8/core/lib/Drupal/Core/Entity/ContentEntityBase.php:823)

Edit: Fix copy/paste error. The original error msg shown was wrong and has been replaced with the correct one. Thanks to mikeryan for questioning that msg on IRC.

I think the d7_user issue is just happening because we have a mapping ' langcode: language'.

I'm not sure if that mapping makes sense? I thought "language" in the D6/D7 users table was supposed to mean "the language this user prefers for communication", not "the default translation for this entity".

Ah, I see, d7_user.yml has:

  langcode: language
  preferred_langcode: language
  preferred_admin_langcode: language

So, it looks to me like preferred_langcode is the right mapping, but as you point out the D8 langcode here is for translation of the user entity, not for their language preference. D7 didn't have translation of user entities, right? So we should just remove that langcode mapping...

Trying to fix MigrateUserTest. Until we have translated user migrations, we should assume the user's langcode is the site default. Their preferred langcodes could still be migrated.

First step in testing against my client's db is rerolling for 8.1.x...

A little naming change between 8.1.x and 8.2.x...

We observed that this only works properly when migrating revision IDs (vids), keeping the same ones in D8 as D6. If new revisions are generated, then there's no way for different-language-versions of the revision to find the same revision ID.

We're going to need a way to query the id_map by partial source IDs.

Postponed on #2724941: Need to be able to lookup destination IDs by partial source IDs.

Summary of testing I just did (tl;dr - looks good to me!)

I:

1. created a new D6 site.
2. Enabled Locale (required by Translate Content) and Translate Content modules, set up permissions
3. Set up page content type to allow translations
4. Enabled Node Tools (required by Revisioning) and Revisioning
5. Set up page content type to create new revisions
6. Created English page node
7. created French translation
8. created new revision of English
9. created German translation
10. patched D8 core multilingual files as specified in #132

In new 8.1.1 site:

1. Enabled Content Translation and Language
2. Added French and German

At http://d8test/upgrade, Page informed me that the following are missing from the upgrade path.

• admin_menu
• color
• devel
• devel_generate
• help
• node_tools
• revisioning
• translation

Clicked on "Perform upgrade" button. Got message:

Completed 61 upgrade tasks successfully
Congratulations, you upgraded Drupal!

And it provided a link to the detailed log.

It created a single row in the node table, nid=1, vid=4, langcode=en

It created 4 nodes in the node_revisions table:

It created 8 rows in the node_revision_body table:

It created 3 rows in the node_field_data table:

It created 8 rows in the node_field_revision table:

I followed the instructions from #109 as I don't care about revisions, and it works. However, I was wondering if the last line in that comment might be relevant:

One thing I noticed is that content_translation_source won't be imported like that, but I think it won't be set either with the full patch? I've seen problems caused by a missing source language in the field synchronizer code.

I have used this code in addition to the patch linked in #109 to update it and seems to work for me:

if(!$entity->isDefaultTranslation()) {
    $entity->set('content_translation_source', $entity->getUntranslated()->language()->getId());
}

Here's a first attempt at migrating nodes with the new destination-lookup. It doesn't yet include node revisions, just nodes. Probably some tests will fail.

Again, just nodes, not revisions. Maybe we should just review it this way? We could do a follow-up issue for revisions, which are much more complicated. It would be great to get one example of a migrated translated entity in core, so we can work on all the other entities.

Some issues that need thinking about, and maybe some testing:
* Is anything here actually order-dependent? Is it ok if non-default translations are imported first?
* Are we ok with the way import/rollback aren't symmetrical? When we import a row, we either add an entity, or add a translation to an existing one...but when we rollback a row, we remove the whole entity. I don't think there's a better solution until this is closed: https://www.drupal.org/node/2443991
* Make sure that the 'migration' process is working right, now that we have two-column source IDs for nodes. Might already be a test that exercises this?

Grr, patch didn't attach.

I had the User migration default to the site's default language code. I think that's a reasonable solution to that problem.

For the term_node issue, I had to mess with the 'migration' process. I'm not sure I like the result:

process:
  nid:
    -
      plugin: migration
      migration: d6_node
      source:
        - nid
    -
      plugin: skip_on_empty
      method: row
    -
      plugin: extract
      index:
        - 0

But I guess that's what's needed now that node migrations have two source keys. It would be nice to be able to say something like this instead, maybe a future enhancement?

process:
  nid:
    -
      plugin: migration
      migration: d6_node
      source: nid
      destination_id_count: 1

Also, I tested out rows where the default language isn't the first. We end up creating a node with the wrong default language, no good. Not sure what the right solution is here.

• Does D6 have the concept of a "default language"? Can we just take the node where nid = tnid as the default? Does such a node always exist?
• Assuming we can identify a default language, then what? I don't think we can say "create this node in french, but leave the default language english". So do we sort the default language first, and become order dependent? Or maybe we create a stub in the default language and then populate the values in whatever language we have? Are there other options?

Does D6 have the concept of a "default language"? Can we just take the node where nid = tnid as the default? Does such a node always exist?

Vasi, I think you can also have a tnid == 0, at least that is the case in some rows in my node_export csv. I guess tnid gets populated when there is at least one translation, but not sure.

Does D6 have the concept of a "default language"?

Reading again your question, I would say that D6 default language has no direct relation with the source node. You could have source nodes in any of the enabled languages. Source nodes do have tnid == nid set.

So far I did not have any problem with that in D8. My imported entities get created in order and can have a different source language, even if it is not the site's default. In each case x-default is assigned for the first entity:

[langcode] => Array
                (
                    [x-default] => es
                    [ca] => ca
                )

This might be a problem in D7, where field translation was possible, though.

Some initial review - @vasi, can we get some time tomorrow to walk through this in #drupal-migrate?

1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -85,7 +86,40 @@ public function import(Row $row, array $old_destination_id_values = array()) {
   +    if (!empty($this->configuration['source_language_keys'])) {
   

   I'm not sure I see where source_language_keys could be multiple (i.e., why is this not source_language_key)?

2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -85,7 +86,40 @@ public function import(Row $row, array $old_destination_id_values = array()) {
   +   * @param array &$destination_ids
   +   *   The old destination IDs, to be modified by this method.
   +   */
   +  protected function getTranslatedDestinationIds(Row $row, array &$destination_ids) {
   

   Since the purpose of this function is to generate modified destination IDs, I'd rather see them returned rather than modified as a side-effect.

3. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
   @@ -248,7 +260,44 @@ protected function getCckData(array $field, Row $node) {
   +    $ids['language']['type'] = 'string';
   +    $ids['language']['alias'] = 'n';
   

   On the source side, nid alone should always be unique, no? Why add language to the source ID?

4. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
   @@ -248,7 +260,44 @@ protected function getCckData(array $field, Row $node) {
   +  protected function maxVidQuery() {
   

   If I'm not mistaken, this is assuming the maximum vid is the active revision for a given language? Shouldn't it be the vid matching n.vid?

I'm not sure I see where source_language_keys could be multiple (i.e., why is this not source_language_key)?

I'll change that back to singular. Just erring on the side of generality.

Since the purpose of this function is to generate modified destination IDs, I'd rather see them returned rather than modified as a side-effect.

Ok, will do.

On the source side, nid alone should always be unique, no? Why add language to the source ID?

Yeah, that's true for D6 content translation. I was trying to make things the same for content-translation and entity-translation, so that all the related migrations don't have to change depending on that.

<

If I'm not mistaken, this is assuming the maximum vid is the active revision for a given language? Shouldn't it be the vid matching n.vid?

Yes. Maybe I should rename this function. It doesn't find the maximum node_revisions.vid, it finds the maximum node.vid in the current translation set. This is important so that we don't try to import multiple translations of the same node, that each claim that the current revision is something different, that really confuses Drupal.

Well, after hurting my head trying to deal with the whole patch here, I've implemented a custom solution for my current client which is quite simple and (based on *very* minimal testing) seems to work. Keep in mind that in my case we don't care about revision history, and also that we are not preserving nids and vids.

First thing is, defining separate migrations for the primary/default translation, and for other translations, which addresses the need to get the default translation right when the node is originally created. To accommodate this, I added a 'translation' configuration property in my source plugins (derived from d6_node) - when the value is 'primary' I filter the query on n.tnid=0 OR n.tnid=n.nid, when 'secondary' the filter is n.tnid>0 AND n.tnid <> n.nid.

So, for my node_bio migration, the only change I make is to add that configuration value:

source:
  plugin: bio_node
  node_type: bio
  translation: primary

I copied that node_bio migration to make a node_bio_translation migration - apart from changing the id and label, these are the only differences:

...
source:
  plugin: bio_node
  node_type: bio
  translation: secondary
process:
...
  nid:
    plugin: migration
    migration: node_bio
    source: tnid
...
migration_dependencies:
  required:
    - node_bio
...

Finally, I implemented an entity:node destination plugin to override updateEntity with the same bit the existing patches do here for adding/getting the translation:

  /**
   * {@inheritdoc}
   */
  protected function updateEntity(EntityInterface $entity, Row $row) {
    if ($entity instanceof TranslatableInterface) {
      $property = $this->storage->getEntityType()->getKey('langcode');

      if ($row->hasDestinationProperty($property)) {
        $language = $row->getDestinationProperty($property);

        if (!$entity->hasTranslation($language)) {
          $entity->addTranslation($language);
        }
        $entity = $entity->getTranslation($language);
      }
    }

    parent::updateEntity($entity, $row);
  }

So, would this approach work in the general case? How will the revision migrations deal with this?

I think we should handle revisions in a follow-up, and get just nodes themselves working in this issue.

I think this approach will work ok, but it will require each and every entity to have two migrations, with the exact same process steps. For now we can just have the deriver do double duty. In a follow-up we can find a way to make it easier on users writing their own migrations.

Here's a first try at the new approach.

Fixed some tests, I think.

Also added some new tests:
* Ensure that non-Drupalish content (without nids) can be imported with translations.
* Ensure that rolling back translations works.

One issue I've thought of: What happens to node references that refer to non-default translations? You can't use a 'plugin: migration' against the node migration, since the translations won't be there, so we'll need to refer to the node_translation migration instead....ick.

I changed 'tnid' in the source output so it is consistent and always means "the current translation set", even if there's just one node in it. This lets us use it in the process for 'nid'. Tests needed updating.

Some comments based on a quick review, but I still need to dig a bit deeper (and test manually).

Is the user module stuff really in scope? The node translations aren't dependent on that, are they?