Problem/Motivation

Drupal 6 to 8 migrations currently don't include translations of nodes.

Translated nodes in D6 use a tnid property that associates them into translation sets. There's also i18ncontent and i18ncck that take care of the nodes and fields.

Proposed resolution

  • Add translation data to the drupal6 migration fixture.
  • Update the content entity destination to support translations.
  • Make the node migration sources include only the default translation.
  • Add auxiliary migrations to bring in the translations.

Related issues will be handled as follow-ups:

  • Make the auxiliary migrations auto-derived, so they don't need a template.
  • Migrate translated node revisions.
  • Migrate other entity types: users, terms, etc.
  • Migrate related data: menu items, taxonomy term relations, node access rules, path aliases, visitor stats, etc.
  • Handle references to translated nodes, ie: "node/5" might be a translation, will links to that still work?
Files: 
CommentFileSizeAuthor
#238 2225775-i18n-node.interdiff.235-238.txt770 bytespenyaskito
#238 2225775-i18n-node-238.patch68.69 KBpenyaskito
#235 interdiff-225-235.txt5.14 KBquietone
#235 2225775-i18n-node-235.patch69.3 KBquietone
#232 interdiff-2225775-225-232.txt8.32 KBquietone
#232 2225775-i18n-node-232.patch72.22 KBquietone
#225 2225775-i18n-node-225.patch67.89 KBvprocessor
#222 interdiff.txt4.17 KBquietone
#222 2225775-i18n-node-222.patch67.87 KBquietone
#214 2225775-i18n-node-213.patch67.77 KBpenyaskito
#201 interdiff.txt574 bytesvasi
#201 2225775-201.patch68.18 KBvasi
#199 interdiff.txt4.39 KBvasi
#199 2225775-199.patch67.94 KBvasi
#191 interdiff.txt1.78 KBvasi
#191 2225775-191.patch64.27 KBvasi
#190 interdiff.txt32.41 KBvasi
#190 2225775-190.patch62.48 KBvasi
#185 2225775-185.patch67.96 KBvasi
#177 interdiff.txt1.91 KBvasi
#177 2225775-177.patch67.44 KBvasi
#162 interdiff.txt6.27 KBvasi
#162 2225775-162.patch66.92 KBvasi
#157 interdiff.txt2.72 KBvasi
#157 2225775-157.patch66.62 KBvasi
#155 interdiff.txt24.67 KBvasi
#155 2225775-155.patch63.9 KBvasi
#152 interdiff.txt21.68 KBvasi
#152 2225775-152.patch46.1 KBvasi
#143 interdiff.txt8.24 KBvasi
#143 2225775-143.patch41.45 KBvasi
#141 2225775-140.patch33.55 KBvasi
#140 interdiff.txt6.98 KBvasi
#138 2225775-138.patch28.13 KBvasi
#132 i18n-2225775-8.1.x-132.patch62 KBmikeryan
#130 i18n-2225775-130.patch62 KBmikeryan
#129 interdiff.txt3.88 KBvasi
#129 2225775-129.patch61.99 KBvasi
#127 interdiff.txt446 bytesvasi
#127 2225775-127.patch58.12 KBvasi
#121 interdiff-2225775-113-121.txt991 bytesquietone
#121 2225775-121.patch57.68 KBquietone
#113 2225775-113.patch57.12 KBvasi
#110 2225775-110.patch54.88 KBquietone
#102 interdiff.txt7.02 KBvasi
#102 2225775-102.patch295.61 KBvasi
#101 interdiff.txt2.64 KBvasi
#101 2225775-101.patch290.04 KBvasi
#101 2225775-101-do-not-test.patch41.88 KBvasi
#99 interdiff.txt32 KBvasi
#99 2225775-99.patch288.06 KBvasi
#99 2225775-99-do-not-test.patch39.9 KBvasi
#97 2225775-97.patch267.2 KBvasi
#92 interdiff-2225775-89-92.txt2.26 KBquietone
#92 2225775-92.patch267.12 KBquietone
#89 interdiff.txt4.82 KBvasi
#89 2225775-89-do-not-test.patch18.26 KBvasi
#89 2225775-89.patch266.42 KBvasi
#82 interdiff.txt1.21 KBvasi
#82 2225775-82-do-not-test.patch14.8 KBvasi
#82 2225775-82.patch262.96 KBvasi
#79 interdiff.txt505 bytesvasi
#79 2225775-79-do-not-test.patch14.53 KBvasi
#79 2225775-79.patch262.69 KBvasi
#76 2225775_76.patch262.48 KBchx
#76 2225775_76-do-not-test.patch14.32 KBchx

Comments

Anonymous’s picture

Tested this today and I observed this behavior:

Source: EN default language, FR language added
Destination: FR default language, EN was not enabled by default (had to add it back in).

  • Source UND node → Destination UND node
  • Source EN node → Destination UND node = PROBLEM = content translation was not enabled. English was not installed. Fixed. Now shows as EN after deleting all the content and delete from migrate_map_d6_node
  • Source FR node → Destination FR node

One problem remains though: tnid is not mapped so the translations are not linked.

Anonymous’s picture

After talking to Gábor it sounds like the practice of having a separate node for each translation is over. Thus, the tnid field should not be mapped.

We will have to do something to detect when something is a translation of an existing entity and update the fields in that entity with the new "row" of data.

mikeryan’s picture

Project: IMP » Drupal core
Version: » 8.0.x-dev
Component: Code » migration system
Priority: Normal » Major
Issue tags: +Migrate critical
Parent issue: #2208401: [META] Multilingual migrations / i18n meta issue »
phenaproxima’s picture

Issue tags: +Needs tests
FileSize
2.07 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 108,216 pass(es), 36 fail(s), and 36 exception(s). View

Initial attempt at a patch, affecting d6_node only.

Gábor Hojtsy’s picture

@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 :)

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Status: Active » Needs review
Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-content, +sprint

Status: Needs review » Needs work

The last submitted patch, 4: 2225775-4.patch, failed testing.

Gábor Hojtsy’s picture

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

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
2.51 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 109,998 pass(es). View
293 bytes

Added schema for the node destination.

phenaproxima’s picture

FileSize
276.48 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,291 pass(es), 7 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 12: 2225775-12.patch, failed testing.

The last submitted patch, 12: 2225775-12.patch, failed testing.

steinmb’s picture

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}
steinmb’s picture

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

steinmb’s picture

Issue summary: View changes
FileSize
64.55 KB

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:

promotion options missing

neclimdul’s picture

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

ultimike’s picture

Issue summary: View changes

Removed rouge images from issue summary.

-mike

steinmb’s picture

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

steinmb’s picture

Issue summary: View changes
FileSize
114.3 KB

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.

no translations

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.

translation settins D8

steinmb’s picture

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
        (
        )

)
steinmb’s picture

Gábor Hojtsy’s picture

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

esclapes’s picture

FileSize
203.41 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,708 pass(es), 86 fail(s), and 86 exception(s). View

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
steinmb’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 25: 2225775-25-new-translation-fixture.patch, failed testing.

The last submitted patch, 25: 2225775-25-new-translation-fixture.patch, failed testing.

xjm’s picture

Issue tags: +rc target

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.

anavarre’s picture

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.

quietone’s picture

alexpott’s picture

Issue tags: -rc target +rc target triage

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

phenaproxima’s picture

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).

Status: Needs review » Needs work

The last submitted patch, 33: 2225775-30.patch, failed testing.

phenaproxima’s picture

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

steinmb’s picture

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:

steinmb’s picture

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]
&#039;d6migrate.imagecache_preset&#039; doesn&#039;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
phenaproxima’s picture

Status: Needs review » Closed (won't fix)

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.

Gábor Hojtsy’s picture

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.

phenaproxima’s picture

Status: Closed (won't fix) » Postponed

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

xjm’s picture

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

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Status: Postponed » Needs work

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

Gábor Hojtsy’s picture

Issue tags: -sprint, -rc eligible

Removing from D8MI sprint due to inactivity.

quietone’s picture

quietone’s picture

Issue tags: +Needs reroll
quietone’s picture

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.

quietone’s picture

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.

quietone’s picture

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.

quietone’s picture

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

quietone’s picture

Status: Needs work » Needs review

The last submitted patch, 48: 2225775-48.patch, failed testing.

The last submitted patch, 49: 2225775-49.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 50: 2225775-50.patch, failed testing.

quietone’s picture

Looks like I uploaded the wrong patch.

quietone’s picture

Status: Needs work » Needs review
FileSize
283.45 KB

This should be the right patch.

Status: Needs review » Needs work

The last submitted patch, 56: 2225775-56.patch, failed testing.

quietone’s picture

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".

Gábor Hojtsy’s picture

Issue tags: -Needs reroll +sprint
vasi’s picture

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.

quietone’s picture

@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.

Gábor Hojtsy’s picture

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

quietone’s picture

Status: Needs work » Needs review

for testbot

Status: Needs review » Needs work

The last submitted patch, 61: 2225775-61.patch, failed testing.

vasi’s picture

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?

Gábor Hojtsy’s picture

@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!

steinmb’s picture

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.

quietone’s picture

quietone’s picture

Add parent issue.

quietone’s picture

Status: Needs work » Needs review
FileSize
283.1 KB

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.

Status: Needs review » Needs work

The last submitted patch, 70: 2225775-70.patch, failed testing.

chx’s picture

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 .

chx’s picture

Status: Needs work » Needs review
FileSize
266.19 KB

Let's see this.

chx’s picture

FileSize
263.62 KB

That's not enough. I meant this :)

The last submitted patch, 73: 2225775_73.patch, failed testing.

chx’s picture

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

Status: Needs review » Needs work

The last submitted patch, 76: 2225775_76.patch, failed testing.

Gábor Hojtsy’s picture

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?

vasi’s picture

vasi’s picture

Status: Needs work » Needs review
vasi’s picture

Why is the vid condition removed?

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

D6 node table
nid vid tnid language
1 1 1 en
2 2 1 fr
D6 node_revisions table
nid vid
1 1
2 2

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:

D6 node table
nid vid tnid language
9 12 9 en
10 13 9 fr
D6 node_revisions table
nid vid
9 11
9 12
10 13
D8 node_field_data
nid vid langcode
9 12 en
D8 node_field_revision
nid vid langcode
9 13 fr
9 11 en
9 12 en
9 13 en

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?

vasi’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 82: 2225775-82.patch, failed testing.

Gábor Hojtsy’s picture

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.

quietone’s picture

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?

vasi’s picture

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.

vasi’s picture

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.

quietone’s picture

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

Like this?

translation set English French
0 1 1
1 2 1
.. .. ..
19 20 1
21 20 2
22 20 3
.. .. ..
24 20 5

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.

vasi’s picture

Hmm, more like this:

D8 vid D6 en vid D6 fr vid
1 1
2 2
3 2 3
4 4 3
5 5 3
...
23 23 13
24 24 13
25 24 25

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

Status: Needs review » Needs work

The last submitted patch, 89: 2225775-89.patch, failed testing.

quietone’s picture

@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.

quietone’s picture

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

quietone’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 92: 2225775-92.patch, failed testing.

quietone’s picture

Issue tags: +migrate-d6-d8
vasi’s picture

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.

vasi’s picture

Status: Needs work » Needs review
FileSize
267.2 KB

Re-rolled, fixed conflicts.

Status: Needs review » Needs work

The last submitted patch, 97: 2225775-97.patch, failed testing.

vasi’s picture

Status: Needs work » Needs review
FileSize
39.9 KB
288.06 KB
32 KB

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).

Status: Needs review » Needs work

The last submitted patch, 99: 2225775-99.patch, failed testing.

vasi’s picture

Status: Needs work » Needs review
FileSize
41.88 KB
290.04 KB
2.64 KB

Fixed the tests, I hope.

vasi’s picture

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.

Gábor Hojtsy’s picture

The strategy explained in #89 sounds good to me.

quietone’s picture

@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.

vasi’s picture

Issue summary: View changes

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:

node
nid tnid language
1 1 en
2 1 fr
3 1 de
node_revisions
vid nid title
1 1 Dog
2 2 Chien
3 1 Dog
4 3 Hund

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

Source rows
vid language nid title
1 en 1 Dog
2 fr 1 Chien
3 en 1 Dog
4 de 1 Hund

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:

Source rows, new versions
vid language nid title
1 en 1 Dog
2 en 1 Dog
2 fr 1 Chien
3 en 1 Dog
3 fr 1 Chien
4 en 1 Dog
4 fr 1 Chien
4 de 1 Hund

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.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

Issue tags: +neworleans2016
mikeryan’s picture

Version: 8.1.x-dev » 8.2.x-dev
Berdir’s picture

Status: Needs review » Needs work

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.

quietone’s picture

Status: Needs work » Needs review
FileSize
54.88 KB

First go at a reroll.

Status: Needs review » Needs work

The last submitted patch, 110: 2225775-110.patch, failed testing.

vasi’s picture

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.

vasi’s picture

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.

vasi’s picture

Status: Needs work » Needs review

The last submitted patch, 76: 2225775_76-do-not-test.patch, failed testing.

The last submitted patch, 79: 2225775-79-do-not-test.patch, failed testing.

The last submitted patch, 82: 2225775-82-do-not-test.patch, failed testing.

The last submitted patch, 89: 2225775-89-do-not-test.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 113: 2225775-113.patch, failed testing.

xjm’s picture

Issue summary: View changes
Issue tags: +Needs followup

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.

quietone’s picture

@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.

Status: Needs review » Needs work

The last submitted patch, 121: 2225775-121.patch, failed testing.

quietone’s picture

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.

vasi’s picture

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".

mikeryan’s picture

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...

The last submitted patch, 121: 2225775-121.patch, failed testing.

vasi’s picture

Status: Needs work » Needs review
FileSize
58.12 KB
446 bytes

Status: Needs review » Needs work

The last submitted patch, 127: 2225775-127.patch, failed testing.

vasi’s picture

Status: Needs work » Needs review
FileSize
61.99 KB
3.88 KB

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.

mikeryan’s picture

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

Status: Needs review » Needs work

The last submitted patch, 130: i18n-2225775-130.patch, failed testing.

mikeryan’s picture

Status: Needs work » Needs review
FileSize
62 KB

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

vasi’s picture

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.

vasi’s picture

Status: Needs review » Needs work
mikeryan’s picture

bwinett’s picture

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:

nid vid langcode
1 1 en
1 2 en
1 3 en
1 4 en

It created 8 rows in the node_revision_body table:

entity_id revision_id langcode body_value
1 1 en This is the English source node, revision 1
1 2 en This is the English source node, revision 1
1 2 fr This is the French translation of the English source node, revision 1
1 3 en This is the English source node, revision 2
1 3 fr This is the French translation of the English source node, revision 1
1 4 de This is the German translation of the English source node, revision 1
1 4 en This is the English source node, revision 2
1 4 fr This is the French translation of the English source node, revision 1

It created 3 rows in the node_field_data table:

nid vid langcode title default_langcode
1 4 de German translation of English source node 0
1 4 en English source node 1
1 4 fr French translation of English source node 0

It created 8 rows in the node_field_revision table:

nid vid langcode title default_langcode
1 1 en English source node 1
1 2 en English source node 1
1 2 fr French translation of English source node 0
1 3 en English source node 1
1 3 fr French translation of English source node 0
1 4 de German translation of English source node 0
1 4 en English source node 1
1 4 fr French translation of English source node 0
esclapes’s picture

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());
}
vasi’s picture

Status: Postponed » Needs review
FileSize
28.13 KB

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.

Status: Needs review » Needs work

The last submitted patch, 138: 2225775-138.patch, failed testing.

vasi’s picture

FileSize
6.98 KB

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?

vasi’s picture

Status: Needs work » Needs review
FileSize
33.55 KB

Grr, patch didn't attach.

Status: Needs review » Needs work

The last submitted patch, 141: 2225775-140.patch, failed testing.

vasi’s picture

Status: Needs work » Needs review
FileSize
41.45 KB
8.24 KB

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
vasi’s picture

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?
esclapes’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 143: 2225775-143.patch, failed testing.

esclapes’s picture

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.

mikeryan’s picture

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?

vasi’s picture

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.

mikeryan’s picture

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?

vasi’s picture

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.

vasi’s picture

Here's a first try at the new approach.

vasi’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 152: 2225775-152.patch, failed testing.

vasi’s picture

Status: Needs work » Needs review
FileSize
63.9 KB
24.67 KB

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.

Status: Needs review » Needs work

The last submitted patch, 155: 2225775-155.patch, failed testing.

vasi’s picture

Status: Needs work » Needs review
FileSize
66.62 KB
2.72 KB

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.

Status: Needs review » Needs work

The last submitted patch, 157: 2225775-157.patch, failed testing.

vasi’s picture

Status: Needs work » Needs review
mikeryan’s picture

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?

Anything else we can break out so make the patch more manageable/reviewable? Like, maybe the destination-side support for translations (tested with non-Drupal data) could be a separate issue...

  1. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentTest.php
    @@ -25,6 +25,7 @@ class MigrateCommentTest extends MigrateDrupal6TestBase {
    +    $this->installSchema('node', ['node_access']);
    

    I'm curious why this is added?

  2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
    @@ -109,7 +109,8 @@ public function fields(MigrationInterface $migration = NULL) {
    -      $this->updateEntity($entity, $row);
    +      // Allow updateEntity() to change the entity.
    +      $entity = $this->updateEntity($entity, $row) ?: $entity;
    

    EntityConfigBase::updateEntity() should also return the entity.

  3. +++ b/core/modules/migrate/tests/modules/migrate_external_translated_test/migrate_external_translated_test.info.yml
    --- /dev/null
    +++ b/core/modules/migrate/tests/modules/migrate_external_translated_test/migration_templates/migrate.migration.external_translated_test_node.yml
    

    We've stuck with the migration_templates directory for core modules already using it, but I think new test modules should use the favored 'migrations' directory.

  4. +++ b/core/modules/migrate/tests/modules/migrate_external_translated_test/migration_templates/migrate.migration.external_translated_test_node.yml
    @@ -0,0 +1,20 @@
    +  default_lang: true
    

    I'd rather spell it out - default_language. Although, is it more accurate to call is 'default_translation'?

  5. +++ b/core/modules/migrate/tests/modules/migrate_external_translated_test/migration_templates/migrate.migration.external_translated_test_node.yml
    @@ -0,0 +1,20 @@
    +  langcode:
    +  langcode:
    

    Dupe line.

  6. +++ b/core/modules/migrate/tests/src/Kernel/MigrateExternalTranslatedTest.php
    @@ -0,0 +1,94 @@
    +   * We only need migrate_drupal because of https://www.drupal.org/node/2560795 .
    

    I would make this
    @todo: Remove when https://www.drupal.org/node/2560795 is fixed.

  7. +++ b/core/modules/migrate/tests/src/Kernel/MigrateExternalTranslatedTest.php
    @@ -0,0 +1,94 @@
    +    $this->assertFalse($node->hasTranslation('de'));
    

    Perhaps we should install de? I'm not sure this is proving much, how would a node acquire a translation for a language that isn't even installed?

  8. +++ b/core/modules/node/migration_templates/d6_node_translation.yml
    @@ -0,0 +1,51 @@
    +id: d6_node_translation
    

    Rather than have a .yml which is a near-identical copy of d6_node.yml, could we have the deriver create it?

vasi’s picture

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

I'm curious why this is added? (re: MigrateCommentTest)

It turns out a bunch of tests break in the presence of languages, or nodes with languages. So this is needed to keep tests green. We could try splitting it out if you'd like.

EntityConfigBase::updateEntity() should also return the entity.

There are a bunch of different subclasses that override this method. I made returning the entity optional so that I don't have to change all of them. Is that ok? Would you rather I change all of them? Or just change the base class?

default_lang: true

This is part of an test of non-Drupalish data being imported, since that was a problem with a previous version of the patch—thanks for finding that! I intentionally chose a name different from Drupal's here, to make sure we weren't relying on it being the same.

Perhaps we should install de? I'm not sure this is proving much, how would a node acquire a translation for a language that isn't even installed?

Yeah, it's perhaps a bit unlikely that we'd assign a random language. Maybe I'll take it out.

Rather than have a .yml which is a near-identical copy of d6_node.yml, could we have the deriver create it?

And the same for node_revisions. Maybe let's do that as a follow-up?

Everything else you mentioned sounds correct, I'll update the patch.

vasi’s picture

mikeryan’s picture

I've run this with a real D6 site on 8.1.x (latest patch applies to 8.1.x cleanly) and translations were properly migrated. I think everything in the code review that needs to be addressed has been, so to me this is RTBC. I'd like to get more eyes on it, though, in particular someone with in-depth understanding of the D8 translation system (e.g., Gabor).

vasi’s picture

Issue summary: View changes
penyaskito’s picture

Assigned: Unassigned » penyaskito

Gábor asked me to review this one. Assigning it, as I think it's complex enough for needing more time to actually test it.
This is a first code review pass, but first of all, I'm impressed how good this looks and it's simpler than expected (as simple as this could be).

  1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
    @@ -75,6 +75,7 @@ protected function updateEntity(EntityInterface $entity, Row $row) {
    +    return $entity;
    

    Nitpick: we need to add @return to the phpdoc.

  2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
    @@ -185,4 +234,32 @@ protected function processStubRow(Row $row) {
    +  public function rollback(array $destination_identifier) {
    ...
    +      // Attempt to remove the translation.
    

    If I'm re-migrating a translation, and I have already one, I'm deleting it. It's that really ok?

  3. +++ b/core/modules/node/migration_templates/d6_node.yml
    @@ -6,7 +6,7 @@ deriver: Drupal\node\Plugin\migrate\D6NodeDeriver
    -  nid: nid
    +  nid: tnid
    

    What happens when there is no multilingual site? Is tnid there?

  4. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
    @@ -15,6 +15,11 @@
    +   * An expression for the translation set of a node.
    +   */
    +  const NODE_TRANSLATION_SET = 'CASE n.tnid WHEN 0 THEN n.nid ELSE n.tnid END';
    +
    +  /**
    

    Ok, this answered my question, but it's tricky. Let's add a comment in the migration for clarification?

  5. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
    @@ -123,6 +127,11 @@ public function prepareRow(Row $row) {
    +    // Make sure we always have a translation set.
    +    if ($row->getSourceProperty('tnid') == 0) {
    +      $row->setSourceProperty('tnid', $row->getSourceProperty('nid'));
    +    }
    

    Yep. This. Let's add a comment in the migration for clarification.

  6. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
    @@ -251,4 +260,46 @@ public function getIds() {
    +   * Wuery to get the max vid of the translation set in the node table.
    

    Typo. Query.

vasi’s picture

Thanks for the review, @penyaskito. Most of those I can fix up. I'm not sure I understand this though:

If I'm re-migrating a translation, and I have already one, I'm deleting it. It's that really ok?

This is in rollback(), so it only happens when you rollback a translation that you previously imported. What's the behaviour you're expecting?

penyaskito’s picture

This is in rollback(), so it only happens when you rollback a translation that you previously imported. What's the behaviour you're expecting?

I'm thinking about incremental migrations. If I already succeeded running a migration, I will have a node with a translation. If that node runs again, and something happens that ends up with a rollback, I'm deleting the translation, not only the one I just imported but the existing one.
It doesn't look like critical to me, but maybe we can think of better options here. In any case that could be part of a follow-up.

mikeryan’s picture

If that node runs again, and something happens that ends up with a rollback

Migrate's rollback is a distinct operation from import - it only happens explicitly (drush migrate-rollback), it's not something that gets called during import when there's a failure.

vasi’s picture

I'm thinking about incremental migrations. If I already succeeded running a migration, I will have a node with a translation. If that node runs again, and something happens that ends up with a rollback, I'm deleting the translation, not only the one I just imported but the existing one.

@mikeryan already addressed that rollback only happens explicitly. But I should also add that we detect existing translations, in which case we don't delete on rollback. See EntityContentBase::updateEntity() :

// By default, an update will be preserved.
$rollback_action = MigrateIdMapInterface::ROLLBACK_PRESERVE;

// [snip]

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

  // We're adding a translation, so delete it on rollback.
  $rollback_action = MigrateIdMapInterface::ROLLBACK_DELETE;
}
penyaskito’s picture

@mikeryan, @vasi: Thanks, clear now :)

penyaskito’s picture

Assigned: penyaskito » Unassigned
Issue summary: View changes
FileSize
96.72 KB

OK, I have tested this with a site. I set it Story content type as translatable in D6, and created two stories: one in English and one in Neutral language. I've translated the English one to Spanish.

I run the migrations on the Drupal 8 site using the experimental UI, and the node in Neutral was assigned the "Language not specified" language. The English node and its translation was assigned the right language, and both translations were in the same node as expected. So in terms of migration, it works perfectly.

The only thing I somehow missed is that the source translation is not migrated. I added another translation manually in the D8 site and got it filled:

I think that we can assume that the source translation is the original tnid, but in any case I wouldn't block this on that, we can create a follow-up.

I would RTBC this one, but the points in #165 are quick to fix, so better if can do them before committing.

Amazing job everyone :)

Gábor Hojtsy’s picture

Title: Migrate D6 i18n nodes » Migrate Drupal 6 core node translation to Drupal 8

Fixing title. The node translation feature is not specific to i18n module.

penyaskito’s picture

I think that we can assume that the source translation is the original tnid, but in any case I wouldn't block this on that, we can create a follow-up.

Gábor asked for clarification on what I meant here on IRC. I was referring to the "source language" column in that table, the original source of the translation. I still think it's ok to do in a follow-up.

mikeryan’s picture

@penyaskito: So, what do we need to get this to RTBC? I feel I've been a little too closely involved with this (suggesting the approach of a separate translation migration), and I'm also not particularly translation-fluent, to do it. Do you feel confident enough in the last patch to set to RTBC?

vasi’s picture

@mikeryan: I'll fix the minor issues @penyaskito identifier tonight. Then hopefully it can go RTBC! Sorry for the delay.

Gábor Hojtsy’s picture

Yeah the source translation would/should be as indicated by the tnid yes, Drupal core translation in 6/7 does not have another way to identify if you translated from a different source. It is fine to fix in a followup or here as appropriate.

vasi’s picture

Ok, this should handle the issues @penyaskito identified above.

Source translation looks more complicated, it'll take an extra join on the node table to figure that out. So let's leave it for a follow-up.

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

@mikeryan and I tested the patch at #162, and the new one only includes comments fixes, so it's good to go.
All the concerning issues have been fixed.

Created #2746293: Migrate content_translation_source when migrating node translations as a follow-up. We need to check if the follow-ups at #120 have been created.

For me this is RTBC. Thanks everyone :D

vasi’s picture

I created some follow ups:

I also updated the META issue #2208401: [META] Multilingual migrations / i18n meta issue with all the follow-ups.

I'd also like to look into how to adapt the solution we have to non-D6 migrations, just to make sure we don't end up making those impossibly hard.

Gábor Hojtsy’s picture

@vasi: yeah the D7 node translation schema / data structure is the same as D6 I think, so the D7 to D8 should be the same except of course dedicated test coverage :) Thanks for opening those followups.

mikeryan’s picture

I can't find an issue for D8-to-D8 migration, but surely one must exist somewhere...

#2607524: Migrations from Drupal 8 to Drupal 8 and Drupal 8 to Drupal 9

vasi’s picture

@Gábor Hojtsy: As you know, many (most?) D7 sites use D8 style field-translation with the entity_translation module. And of course we'll have to support D8-to-D8 migration too at some point! So we'll have to figure out a way to support that.

penyaskito’s picture

Issue tags: -Needs followup

All follow-ups have been created, so removing tag.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 177: 2225775-177.patch, failed testing.

vasi’s picture

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed the conflicts parts, still looks good to me if green.

steinmb’s picture

+1 from me. BTW, did we create a followup issue on supporting entity translation (entity_translation)? Do not see it listed as one of the child isssues.

Gábor Hojtsy’s picture

@steinmb: yeah I only found #929402: Add support for migrate module which is a destination handler for entity_translation D7. I think that issue still needs to be opened. Not sure it logically/technically should be a children of this one since the two translation methods are different and entity_translation was not in core either. It should definitely be a related issue.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
    @@ -105,14 +111,36 @@ protected function save(ContentEntityInterface $entity, array $old_destination_i
    +        throw new MigrateException('This entity type does not support translation.');
    

    Is this exception tested?

  2. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentBaseTest.php
    @@ -0,0 +1,159 @@
    +    $this->assertFalse($this->storage->load(4));
    
    +++ b/core/modules/migrate/tests/src/Kernel/MigrateExternalTranslatedTest.php
    @@ -0,0 +1,92 @@
    +    $this->assertFalse($node->hasTranslation('es'));
    ...
    +    $this->assertFalse($node->hasTranslation('fr'));
    +    $this->assertFalse($node->hasTranslation('es'));
    ...
    +    $this->assertNull($storage->load(4));
    

    assertNull and assertFalse should have an assertion message

  3. +++ b/core/modules/node/src/Plugin/migrate/D6NodeDeriver.php
    @@ -99,9 +99,10 @@ public function getDerivativeDefinitions($base_plugin_definition) {
    +        // If this migration is based on the d6_node_revision migration, or
    

    No need for a comma before the or.

  4. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
    @@ -15,6 +15,11 @@
    +   * An expression for the translation set of a node.
    

    Might be worth saying "An SQL expression..."

  5. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
    @@ -251,4 +260,46 @@ public function getIds() {
    +    $subquery = $this->select('node', 'n');
    

    If we're going to break everything up like this - no need to call the variable $subquery. I'm not convinced of having both maxVidQuery() and translationQuery(). Do we envisage needing this break up of logic?

  6. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
    @@ -251,4 +260,46 @@ public function getIds() {
    +    $subquery->addExpression(self::NODE_TRANSLATION_SET, 'translation_set');
    

    Should be static:: for late static binding.

  7. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
    @@ -251,4 +260,46 @@ public function getIds() {
    +   * It should have a node table 'n', and a node_revision table 'nr'.
    

    The comma here is not needed.

  8. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
    @@ -251,4 +260,46 @@ public function getIds() {
    +    // Claim our vid is the maximum vid of our translation set.
    +    // Otherwise the revision the node is assigned in D8 may be confusing.
    

    Comment should break on 80 chars - also the second sentence is not clear what is meant.

  9. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
    @@ -251,4 +260,46 @@ public function getIds() {
    +    // Are we yielding original nodes, or translations?
    +    if (empty($this->configuration['translations'])) {
    +      $query->where('n.tnid = 0 OR n.tnid = n.nid');
    +    }
    +    else {
    +      $query->where('n.tnid <> 0 AND n.tnid <> n.nid');
    +    }
    

    Rather than asking a question in the comments I think it is clearer just to say which each statement is.

  10. +++ b/core/modules/node/tests/src/Kernel/Migrate/d6/MigrateNodeTest.php
    @@ -79,6 +80,16 @@ public function testNode() {
    +    $this->assertTrue($node instanceof NodeInterface);
    ...
    +    $this->assertTrue($node->hasTranslation('fr'));
    ...
    +    $this->assertNull(Node::load(10));
    

    This assertions need messages.

  11. +++ b/core/modules/user/src/Plugin/migrate/User.php
    @@ -2,13 +2,80 @@
     /**
      * Plugin class for Drupal 7 user migrations dealing with fields and profiles.
      */
     class User extends Migration {
    

    Why is an issue titled "Migrate Drupal 6 core node translation to Drupal 8" changing Drupal 7 migrations? Ah I see there is some discussion of this already (for example #125). In one of the comments @mikeryan suggested splitting this out as a blocker for this. I think that is a good idea given the complexity of this issue. Obviously it's a blocker for this issue and things might take a little bit longer but it means reviews are more likely to catch mistakes because this is complex stuff.

  12. +++ b/core/modules/user/src/Plugin/migrate/User.php
    @@ -2,13 +2,80 @@
    +      $container->get('language_manager')
    
    @@ -50,6 +117,15 @@ public function getProcess() {
    +        'default_value' => $this->languageManager->getDefaultLanguage()->getId(),
    

    Could just inject the language.default service for a less heavy dependency that's what you're getting with the first call on the languageManager

vasi’s picture

Status: Needs work » Needs review
FileSize
62.48 KB
32.41 KB

Ok, here's an update. What's changed:

  1. Removed the changes to users, now we just skip attempting to add a translation unless the destination is specifically marked as wanting translations. We can add a follow up for the issue where users are currently migrated without language codes.
  2. Removed the maxVidQuery stuff, it's redundant now that the translations migration doesn't include revision IDs.
  3. Made the node-translations migration optional in MigrateDrupal6TestBase::migrateContent(), so we need fewer changes to other tests.
  4. Since node-translation migrations will inevitably fail in the absence of language/content-translation, the D6NodeDeriver will now not generate them unless content_translation is enabled.
  5. Added two tests (unit and integration) for the "does not support translations" exception.
  6. Fixed nitpicks about wording, assertion messages, etc.
vasi’s picture

Added a test for D6NodeDeriver.

vasi’s picture

Added a follow-up for the user language code issue: #2751223: D6 & D7 users are migrated into D8 with incorrect langcode

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +DrupalNorth2016

All the comments from #189 have been addressed. The user migrations changes are gone from this patch with the new strategy described in #180.1. The follow-up has been created.

So IMHO this is good to go.

catch’s picture

The follow-ups all look good, left some comments on a couple of them, thanks for opening.

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -120,8 +146,29 @@ public function getIds() {
+   *
+   * @return NULL|\Drupal\Core\Entity\EntityInterface
+   *   An updated entity, or NULL if it's the same as the one passed in.
    */

Why does this have to be null/entityinterface vs. just returning the $entity all the time?

Looks ready to me too otherwise.

vasi’s picture

Why does this have to be null/entityinterface vs. just returning the $entity all the time?

I just didn't want to change BC. There are destinations that extend EntityContentBase all over core, definitely in my custom code, and probably in other peoples' code too. If it's this easy to keep them working, I think that's a plus.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 191: 2225775-191.patch, failed testing.

Gábor Hojtsy’s picture

Looks like some legit fails in migrate :/

vasi’s picture

Ok, I investigated the failures. They're coming from #2225271: Migrate content type language settings from Drupal 6 & 7. I think there's two things going on:

  1. $MigrateUpgradeForm::moduleUpgradePaths needs an entry for each migration. But there's was no entry added for d6_language_content_settings, or the D7 equivalent. I should have caught this in review, sorry :(
  2. After we import d6_language_content_settings, the content_translation module causes a schema change, see ContentTranslationHandler::getFieldDefinitions(). We never apply these changes though, so the rest of the migrations happen with a difference between the table mapping and the actual DB contents. This issue will happen with any migration that involves d6_language_content_settings and happens with content_translation enabled.
vasi’s picture

Status: Needs work » Needs review
FileSize
67.94 KB
4.39 KB

I think this fixes it. Let me know if you like/dislike the approach.

Do we want a new issue for this problem?

Status: Needs review » Needs work

The last submitted patch, 199: 2225775-199.patch, failed testing.

vasi’s picture

Status: Needs work » Needs review
FileSize
68.18 KB
574 bytes

Forgot to commit a file :O

Marc Angles’s picture

Working on a D6 > D8 migration since several days.

I've worked with drupal stable until I wanted to try this patch.

On drupal-8.2.x-dev and all the dev modules (migrate_tools, migrate_upgrade, migrate_plus) at first glance it is working well, translations are mapped and language switcher works.

However the images (image fields) appears to be migrated into the english translation.

Great job @vasi. Thanks.

vasi’s picture

@Marc Angles: Can you clarify what happened?

* Did the translated nodes have images in D6?
* Did the image files for these translations not get applied to the other translations in D8?
* Were there other fields in translated content? Did they migrate ok?

vasi’s picture

@Marc Angles: Can you clarify what happened?

* Did the translated nodes have images in D6?
* Did the image files for these translations not get applied to the other translations in D8?
* Were there other fields in translated content? Did they migrate ok?

Marc Angles’s picture

@vasi,

* My setup is fr/en, the drupal 6 source has 'French' as the default language
* yes, drupal nodes have images in D6. Using image field. And using the "synchronize translation" feature (if it may be relevant...). So, in drupal 6 the images can be edited both on each translation but they are synchronized (when one image change in the field, it changes for both nodes).
* Now in drupal 8, I get the images imported in the images fields of English nodes but not in the french ones
* There where other fields and also aliases. The fields are ok in the english version but except the titles none are populated on the the french translation.
* On the other hand, aliases are applying only on the french nodes... (there is no url aliases on english nodes)

vasi’s picture

Ok, the "synchronize translations" feature is interesting. I didn't remember about that at all. I think the right solution to that is to change the content translation settings in D8 so that those fields are marked as untranslatable

Are the other fields you have also "synchronized"? Or are they normal translated fields, that somehow weren't imported into the french translations? I think we are testing that case, but maybe something is going wrong.

penyaskito’s picture

I would say we should create a different issue for the image problem, as this may be a special case that happens with other field types too (thinking at least on entity references).

For the aliases, we should do the same and investigate if it's related to #2689459: If you don't want to translate your URL alias, the original URL alias won't work with your translations. The path field is so special I'm not surprised you run into this, but not sure if we should stop this because of that one neither.

Marc Angles’s picture

@vasi, yes, almost all the fields I'm working on are "synchronized" in drupal 6

Other field that cause some headaches are taxonomy terms, I try to report the problems here: https://www.drupal.org/node/2653984

Berdir’s picture

synchronized fields is a feature of the i18n module(s). Since the title specifically says Drupal 7 core node translation, maybe supporting that should be a follow-up issue? I have a feeling that's not going to be trivial.

Gábor Hojtsy’s picture

Yeah I agree synched fields is/was an i18n module feature, and this core migration is complicated enough already to try to include more :) Should be possible in a followup.

vasi’s picture

Added a follow-up for synchronized fields: #2754493: D6 synchronized fields aren't migrated properly

Marc Angles’s picture

I doubt the "synchronization" feature have anything to do with the problems we have when running the upgrade.

I modified the source DB so there is no more synchronization and the problems are still there.

* All the 'en' nodes are correctly migrated (except their url aliases)
* The 'fr' nodes are having their url aliases but are missing their bodies and images
* for the terms, there might be a fix here https://www.drupal.org/node/2653984#comment-11333199

steinmb’s picture

@Marc Angles did you see any errors/warnings during the migration? If so, could you post them?

penyaskito’s picture

Rerolling (automerge worked). No functional changes so hope I'm still eligible to RTBC.

+++ b/core/modules/content_translation/src/ContentTranslationUpdatesManager.php
@@ -2,10 +2,14 @@
+use Drupal\migrate\Plugin\migrate\destination\Entity as EntityDestination;

+++ b/core/modules/language/migration_templates/d7_language_content_settings.yml
@@ -39,6 +39,8 @@ process:
diff --git a/core/modules/migrate/src/Plugin/migrate/destination/DestinationBase.php b/core/modules/migrate/src/Plugin/migrate/destination/DestinationBase.php

This is unused. Removed this and some other minor complaints from phpcs. Interdiff attached.

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

@Marc Angles I tried this again, and it worked for me. I have article nodes, with title and body fields filled.
My neutral node was migrated to a Language neutral node, as I would expect.
My english node was migrated to a English node as I would expect.
My spanish node, being a translation of the English one, was migrated as a translation of the english one.
Didn't really try images, as I'm sure we will need to work on that and @vasi already created a follow-up, which Berdir and Gábor agreed to.

So unless @Marc Angles or someone else can explain how to reproduce the issue of not having bodies migrated, I would love to RTBC and move forward.

penyaskito’s picture

Issue tags: +DevDaysMilan

Tagging for the sprint.

Marc Angles’s picture

@penyaskito to me it seems related to the content_translation_* columns not being in the fields tables at the right time. I did not save the logs but I remember having to run something like drupal update:entities to get those columns added. However, the upgrade was not complete after that and I had some other things to do.

This patch seems to be a huge improvement in any case.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 214: 2225775-i18n-node-213.patch, failed testing.

penyaskito’s picture

@Marc: the way translations work in core doesn't require content translation to be installed actually for doing translations through the API, so those columns are not even mandatory if you want to manage your translations without content translation module. We have a follow-up for that one: #2746293: Migrate content_translation_source when migrating node translations.

Gábor Hojtsy’s picture

Issue tags: +Needs reroll

Seems like the patch application fails are not a biggie, but I have issues rerolling it on my local setup, so sorry but cannot help right now. The fails are in MigrateNodeTest.php and d6_node_translation.yml

Marc Angles’s picture

Ok, I restarted my upgrade process from scratch and everything looks correct now, in terms of translations.

quietone’s picture

Status: Needs work » Needs review
FileSize
67.87 KB
4.17 KB

Reroll. Result in an error in MigrateNodeTest on the test that a link field with an internal link is migrated.
$this->assertSame('internal:/node/10', $node->field_test_link->uri);

Status: Needs review » Needs work

The last submitted patch, 222: 2225775-i18n-node-222.patch, failed testing.

vprocessor’s picture

Assigned: Unassigned » vprocessor
vprocessor’s picture

Assigned: vprocessor » Unassigned
Status: Needs work » Needs review
FileSize
67.89 KB

rerolled

Status: Needs review » Needs work

The last submitted patch, 225: 2225775-i18n-node-225.patch, failed testing.

vprocessor’s picture

Assigned: Unassigned » vprocessor
Gábor Hojtsy’s picture

@vprocessor: planning to work on the fails?

Gábor Hojtsy’s picture

The fails sound very relevant:

testNode
fail: [Other] Line 37 of core/modules/node/tests/src/Kernel/Migrate/d6/MigrateNodeTest.php:
Drupal\Tests\node\Kernel\Migrate\d6\MigrateNodeTest::testNode
Trying to get property of non-object

/var/www/html/core/modules/node/tests/src/Kernel/Migrate/d6/MigrateNodeTest.php:89
rupal\Tests\node\Kernel\Migrate\d6\MigrateNodeTe
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-1156.xml:
PHPunit Test failed to complete

nkno
fail: [run-tests.sh check] Line 0 of :
FATAL Drupal\Tests\node\Kernel\Migrate\d6\MigrateNodeTest: test runner returned a non-zero error code (2).
quietone’s picture

This looks like it is because of a problem with the dump file. Nid 9 revision 11 is a content type planet and nid 9 revision 12 is a content type story. There is a previous issue about this type of problem, #2686651: Same nid in two content types in d6_dump. But that is all I can do on this now. I should be able to look more thoroughly into that this evening (6 hours from now).

mpp’s picture

Any chance we could have some documentation on how this is supposed to work?

Is this all that is needed?

destination:
  plugin: entity:node
  translations: true
...
process:
  langcode: language

I want to migrate translations in different rows to the same node so it should work with this patch (once it's fixed) or am I missing something?

Source:

id;language;title
id1;nl;Title NL
id1;en;Title EN

for id=id1 lookupDestinationId returns

Array
(
    [0] => 2
    [1] => nl
)

So it seems that the English version is missing?

Note that I am using a different source plugin (migrate_source_csv) but as it also uses the Sql id_map that should work, or is something else needed to support migration of translations?

quietone’s picture

Status: Needs work » Needs review
FileSize
72.22 KB
8.32 KB

I hope it is OK that I'm uploading a patch while this is assigned to someone else. I'm doing so because it is tagged migrate critical. Please let me know if I should have waited.

Status: Needs review » Needs work

The last submitted patch, 232: 2225775-i18n-node-232.patch, failed testing.

Gábor Hojtsy’s picture

Assigned: vprocessor » Unassigned
Issue tags: -Needs reroll

@quietone: well, @vprocessor went inactive on this, so I think its fine that you took it over, thanks!

quietone’s picture

Status: Needs work » Needs review
FileSize
69.3 KB
5.14 KB

The previous patch was made with a test version of the test fixture. So ignore that one. (I wish I had a quiet place to work). Anyway, this new patch passes tests locally. The interdiff is against the patch in #225.

quietone’s picture

@Gábor Hojtsy, thanks!

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/modules/migrate_drupal/tests/fixtures/drupal6.php
@@ -41405,25 +41432,6 @@
-$connection->insert('node_access')
-->fields(array(
-  'nid',
-  'gid',
-  'realm',
-  'grant_view',
-  'grant_update',
-  'grant_delete',
-))
-->values(array(
-  'nid' => '0',
-  'gid' => '0',
-  'realm' => 'all',
-  'grant_view' => '1',
-  'grant_update' => '0',
-  'grant_delete' => '0',
-))
-->execute();
-

Why did you remove the node access dataset? That looks totally unrelated. The rest of the changes look logical based on the explanation in #230.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
68.69 KB
770 bytes

Thanks quietone for working on this!

Attended #237 by applying the related chunk of the interdiff as a reverse patch, let's see if the bot is happy now.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good now, yay :)

quietone’s picture

@Gábor Hojtsy,

Why did you remove the node access dataset?

Oversight on my part. (We have a baby visiting and I'm not getting as much sleep as usual.) Thanks for spotting it.

chx’s picture

[Friday, January 31, 2014] [15:28:02] <chx> xjm: the biggest challenge will be multilingual. maybe we will get someone at Szeged to do them. To make this clearer, it's not even on my todo list. I dont know anything about multilingual. I dont know what to migrate, i dont know how to migrate them and i dont know how to test them. Sadly, it's not even a priority for me. Some members of the team might be able to help but knowledge in this field is scarce.

two and half years later or not: YAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAY someone did it! Thanks so much for your hard work.

Gábor Hojtsy’s picture

While this issue is a monumental achievement (especially once it lands #hinthint), there are many more (admittedly mostly smaller) issues to make multilingual migrations semi-complete: https://docs.google.com/spreadsheets/d/16SwQsp7nF1UKrZQCNdPYML_wi53ZjM1e...

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This looks an excellent start and all the followups are in place so I've committed and pushed 1fa26afd9618278aa98334eb15514a2e18cbefd6 to 8.2.x and 169c290 to 8.1.x. Thanks!

It'd be great if someone could answer @mpp's question in #231

  • alexpott committed 1fa26af on 8.2.x
    Issue #2225775 by vasi, quietone, phenaproxima, chx, penyaskito,...
mikeryan’s picture

/me returns from an unblocking spree...

Thanks to everyone who put in such hard work on this!

Gábor Hojtsy’s picture

This is amazing, thanks everyone :) #highfives

kriboogh’s picture

@mpp: we did it like this (from a JSON file):

You need two migration configs.yml files, one for the sources and one for the translations.

edit: your source plugin must return only the "source" data for the my_sources.yml file and the "translated" data for the my_translations.yml file. So if you use a custom source plugin make sure you filter the source data before using if by overriding the protected function initializeIterator() {}.

migrate_plus.migrations.my_sources.yml:

...
source:
  plugin: ...
  ...
  fields:
     ...
  ids:
    id:
      type: integer
destination:
  plugin: entity:node
process:
  type:
    plugin: default_value
    default_value: page
  langcode: langcode
  uid:
    plugin: default_value
    default_value: 1
  title: title
  'field_content/value': field_content_value
  'field_content/format':
    plugin: static_map
    source: field_content_format
    map:
      1: plain_text
      2: basic_html

migration_dependencies: {}

migrate_plus.migrations.my_translations.yml:

source:
  plugin: ...
  ...
  ids:
    id:
      type: integer
destination:
  plugin: entity:node
  translations: true
process:
  nid:
    plugin: migration
    source: tid
    migration: my_sources
  type:
    plugin: default_value
    default_value: page
  langcode: langcode
  uid:
    plugin: default_value
    default_value: 1
  title: title
  'field_content/value': field_content_value
  'field_content/format':
    plugin: static_map
    source: field_content_format
    map:
      1: plain_text
      2: basic_html
      3: full_html
migration_dependencies:
  required:
    - my_sources

JSON source data:

"pages": [
    {
      "id": 1,
      "tid": 1,
      "langcode": "en",
      "title": "Title EN",
      "field_content_value": "Lorem ipsum...EN",
      "field_content_format": 2
    },  
    {
      "id": 3,
      "tid": 1,
      "langcode": "nl",
      "title": "Title NL",
      "field_content_value": "Lorem ipsum ... NL",
      "field_content_format": 2
    },
]
mpp’s picture

Hi @kriboogh, thanks for the follow up. Are you using track_changes? See https://www.drupal.org/node/2749989, I've seen some bizarre effects with track_changes after this patch got in.

Another way of "filtering" the proper sources is by using a specific PREPARE_ROW:

   static function getSubscribedEvents() {
    $events[MigrateEvents::PREPARE_ROW][] = array('onPrepareRow', 0);
    return $events;
  }
  /**
   * React to a new row.
   *
   * @param \Drupal\migrate_plus\Event\MigratePrepareRowEvent $event
   *   The prepare-row event.
   * @return bool
   */
  public function onPrepareRow(MigratePrepareRowEvent $event) {
    $row = $event->getRow();
    if ($event->getMigration()->getSourceConfiguration()['translations']) {
      // Skip source.
      if ($row->getSourceProperty('language') == 'en') {
        return FALSE;
      }
    }
    else {
      // Skip translations.
      if ($row->getSourceProperty('language') != 'en') {
        return FALSE;
      }
    }
  }
kriboogh’s picture

no not using track_changes at the moment. Yes prepare row is also a good alternative, although I wanted the counts (number of rows to import, processed, ignored) to be correct when you show the migration status, so that's why I did it in the initialise iterator.

@mpp A bit of topic perhaps, but any reason why you used a subscribe event in stead of just overriding the prepareRow function in the extended source? Or didn't you use in a source class?

Gábor Hojtsy’s picture

So #2669964: Migrate Drupal 7 core node translations to Drupal 8 would be the next step and should be straightforward hopefully because its the same logic, probably some "new" tests and done(?) :)

webchick’s picture

Issue tags: +8.2.0 release notes

This seems like a pretty big deal. :)

  • alexpott committed 1fa26af on 8.3.x
    Issue #2225775 by vasi, quietone, phenaproxima, chx, penyaskito,...

  • alexpott committed 1fa26af on 8.3.x
    Issue #2225775 by vasi, quietone, phenaproxima, chx, penyaskito,...
mpp’s picture

The patch no longer applies to 8.1.8.

Gábor Hojtsy’s picture

@mpp: well, it was committed on 8.2.x and is not supposed to be maintained for 8.1.x as it will not get committed there.

mpp’s picture

Well I must have missed this: " I've committed and pushed to 8.2.x and 169c290 to 8.1.x."

So it is in 8.1.8? which would explain the patch failing :-)

Gábor Hojtsy’s picture

Indeed, the commit does not seem to have appeared as a comment here, but the files are there. Eg. http://cgit.drupalcode.org/drupal/tree/core/modules/node/migration_templ... Also https://www.drupal.org/project/drupal/releases/8.1.8 lists this issue as well being fixed.

m.c.t’s picture

Is there a way to get a full working example ?
I am struggling to implement this solutions as when I imported my translation, I get the error:
Migration test_fr did not meet the requirements. Missing migrations test_en. requirements: test_en.

But the dependency is there on my translated config file:
migration_dependencies:
required:
- test_en

Also I am migrating sources from mySQL, source is extending SqlBase, not sure if it makes a difference

quietone’s picture

@m.c.t, Welcome!

You'll get a better response if you ask for support in a new issue. This is closed and will have less folks looking at it. Just create a new issue using category 'Support request'. If you use IRC you can also ask in #drupal-migrate on Freenode.

Cheers

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Gábor Hojtsy’s picture

Issue tags: -sprint

Removing from sprint, thanks all again!

pbattino’s picture

Even if this is closed: I keep ending here if I google the error that m.c.t and that I also got.
Since I managed to solve it for me, I post my solution:

Migration test_fr did not meet the requirements. Missing migrations test_en. requirements: test_en.

I had to change the way the requirement is stated. As the example in#247 but with "migration." prepended to the name of the migration.

Instead of

migration_dependencies:
  required:
    - my_sources

I had to write

migration_dependencies:
  required:
    - migration.my_sources