Migrate Drupal 7 node entity translations data to Drupal 8

Yo! This issue looks great! Thanks maxocub for telling me about this issue.

I swapped the parent and related issue so that this can be found easier on the meta issue. I'll add the three follow-ups to the meta as well.

Applied the patch in #7. Can't see that any entity translations for nodes got imported.

Looks pretty good to me. I only have a few small requests.

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d7/FieldableEntity.php
   @@ -36,20 +42,20 @@ protected function getFields($entity_type, $bundle = NULL) {
   -   * @param string $field
   +   * @param string $field_name
   

   Changing this adds a bunch of patch noise. In the interest of making this even easier to review, can we revert this change?

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d7/FieldableEntity.php
   @@ -71,4 +80,39 @@ protected function getFieldValues($entity_type, $field, $entity_id, $revision_id
   +   * @param string $bundle
   +   *   (optional) The bundle.
   +   *
   +   * @return bool
   +   *   Whether the entity type uses entity translation.
   +   */
   +  protected function isEntityTranslatable($entity_type, $bundle = NULL) {
   +    return in_array($entity_type, $this->variableGet('entity_translation_entity_types', []));
   +  }
   

   $bundle is never used in this method, so let's remove it.

3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d7/FieldableEntity.php
   @@ -71,4 +80,39 @@ protected function getFieldValues($entity_type, $field, $entity_id, $revision_id
   +    $query = $this->select('entity_translation', 'et')
   +      ->fields('et', ['language'])
   +      ->condition('entity_type', $entity_type)
   +      ->condition('entity_id', $entity_id)
   +      ->condition('source', '');
   

   We're assuming the entity_translation table exists, and that's a big "if". We should probably add some sort of safeguard or checking here.

4. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d7/FieldableEntity.php
   @@ -71,4 +80,39 @@ protected function getFieldValues($entity_type, $field, $entity_id, $revision_id
   +    return $query->execute()->fetchField();
   

   Nit: We could chain these calls to everything else: return $this->select()->fields()->condition()->execute()->fetchField().

5. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7Test.php
   @@ -62,8 +62,8 @@ protected function getEntityCounts() {
   +      // Module 'language' comes with 'en', 'und', 'zxx'. Migration adds 'is' and 'fr'.
   +      'configurable_language' => 5,
   

   Nit: Over 80 characters.

6. +++ b/core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeTest.php
   @@ -151,8 +152,8 @@ protected function assertRevision($id, $title, $uid, $log, $timestamp) {
   +    $this->assertEntity(1, 'test_content_type', 'en', 'A Node', '2', TRUE, '1421727515', NULL, TRUE, FALSE);
   +    $this->assertRevision(9, 'A Node', '1', NULL, '1529615813');
   

   Why did this change?

Self-assigning for testing and review.

I tested this manually.

Drupal 7 setup

I have three languages enabled: EN (default), FI, SV

I have three content types: Article, Page and Forum (I included Forum in our tests since it has the hard coded special handling in migrations)

Content type language settings:

• Article multilingual settings: Enabled, with field translation (i.e. Entity Translation is enabled)
• Forum multilingual settings: Enabled, with field translation (i.e. Entity Translation is enabled)
• Page does not have multilingual settings enabled.

Article setup:

• Body has Entity Translation enabled.
• I have one custom text field with Entity Translation enabled.
• I have one language neutral custom text field which does not have Entity Translation enabled.

Forum setup:

• Body has Entity Translation enabled.
• I have one custom text field with Entity Translation enabled.
• I have one language neutral custom text field which does not have Entity Translation enabled.

Test content

Forum nodes:

• Forum node 1: Original in English (site default language), translated to both FI and SV
• Forum node 2: Original in English (site default language), no translations
• Forum node 3: Original in Finnish, translated to Swedish
• Forum node 4: Original in Swedish, no translations

Article nodes:

• Article node 1: Original in Finnish, translated to both EN and SV
• Article node 2: Original in Swedish, no translations

Page nodes:

• One page node. Content type does not have multingual support enabled.

Test results

Forum nodes

• OK - Forum node 1: Original in English (site default language), translated to both FI and SV
• OK - Forum node 2: Original in English (site default language), no translations
• NOT OK - Forum node 3: Original in Finnish, translated to Swedish
• NOT OK - Forum node 4: Original in Swedish, no translations

Test failures for Forum node 3 and Forum node 4:

• When the original forum node is created in non-default language
• Body field (field translation enabled) and the custom text field (field translation enabled) do not appear in D8 in node view / edit.
• The language neutral custom field (field translation not enabled) appears OK in all languages.

Article nodes

• OK - Article node 1: Original in Finnish, translated to both EN and SV
• OK - Article node 2: Original in Swedish, no translations

Page nodes

• OK - One page node. Content type does not have multingual support enabled.

Conclusion

• For some reason, the translatable fields of Forum nodes migrated correctly only if the the node's original language is the site default English.
• On Article content type the translations of all fields migrated correctly regardless of the original language of the node.

I managed to find the root cause for the issue reported in #21. It is a bug in D7, not in this migration.

Here's the proof that it is an issue in the source data quality and not in the migration.

Node 8 in the D7 is of type Forum. There is only a FI language version of this node, but the body value language is EN in the database.

Node 9 in the D7 is of type Article. Also this node has a FI language version but here the language of the body value is saved correctly.

Our migration can't do any assumptions and try to guess language codes so we can't start changing the language codes that the source database is indicating.

All feedback has been addressed. @phenaproxima and I have reviewed the patch. There is test automation coverage and my manual tests were all OK except this Forum issue where the bug is on the D7 source side and we can't do anything about it in the migration.

RTBC. These Entity Translation migrations are pretty heroetic stuff @maxocub, well done! Reminder to committers to also credit @jigarius whose blog post heavily influenced this patch.

I'll raise a bug to Entity Translation issue queue on the D7 bug.

Cheers,
Markus

wooooot, the tests were green already?

The D7 ET issue is here: #2986068: Incorrect field language codes for Forum content type

I have been fighting with a D7 -> D8 migration, and finally decided that either my D7 site is borked or there is a bug in FieldableEntity. Then I found this issue ...

I am still not sure whether my D7 site is borked, but I wonder about this change:

+++ b/core/modules/node/src/Plugin/migrate/source/d7/Node.php
@@ -104,11 +104,16 @@ public function query() {
    * {@inheritdoc}
    */
   public function prepareRow(Row $row) {
+    $nid = $row->getSourceProperty('nid');
+    $vid = $row->getSourceProperty('vid');
+    $type = $row->getSourceProperty('type');
+    $entity_translatable = $this->isEntityTranslatable('node') && (int) $this->variableGet('language_content_type_' . $type, 0) === 4;
+    $language = $entity_translatable ? $this->getEntityTranslationSourceLanguage('node', $nid) : $row->getSourceProperty('language');
+
     // Get Field API field values.
-    foreach (array_keys($this->getFields('node', $row->getSourceProperty('type'))) as $field) {
-      $nid = $row->getSourceProperty('nid');
-      $vid = $row->getSourceProperty('vid');
-      $row->setSourceProperty($field, $this->getFieldValues('node', $field, $nid, $vid));
+    foreach ($this->getFields('node', $type) as $field_name => $field) {
+      $field_language = $entity_translatable && $field['translatable'] ? $language : NULL;
+      $row->setSourceProperty($field_name, $this->getFieldValues('node', $field_name, $nid, $vid, $field_language));
     }

What is the significance of 4 for the language_content_type_* variables? They all seem to be set to 2 on my D7 site. That means ...

• $entity_translatable is FALSE
• $language is $row->getSourceProperty('language') (what I want)
• $field_language is NULL (not what I want)

@maxocub would it be possible to get an 19-27 interdiff?

@masipila, I think there is something here that should be documented, perhaps on Known Issues, or a page just for Multiligingual. Obviously, there is the bug to mention but the summary in #29 is really nice (thanks maxocub) and may help migraters in the future who may not know the details of the translation methods.What do you think?

@maxocub and @masiplia thanks for working on this during your holidays!

@maxocub: Thanks for the explanation. I am leaning towards the theory that my site is borked.

@masipila: I have attached an interdiff for 19-27.

Tests are now green again after the reroll. I added also PostgreSQL and SQLite tests which passed.

According to interdiff 19-27 (thanks @benjifisher!) my earlier manual test results are still valid.

Back to RTBC.

What comes to the handbook docs, I'm about to write a similar page to multilingual D7-D8 upgrades as we already have for D6-D8 and that will address #31. I don't think we should postpone this on that handbook page so I'm removing the doc tag.

Cheers,
Markus

Tested this. Looks pretty good. I've got ~1500 nodes that got correctly migrated from D7 to D8 with source language 'pt-pt' and 'en' translation. In D8, the 'en' translations are now present. Good job @maxocub

The only (minor) comment is that some of us also used the 'title' module to translate the titles, and that is still not supported here.

This applies to the 'migrate_drupal' experimental module. It can safely go into 8.6. We shouldn't need to wait 6 months for this important improvement to the D7 -> D8 migration path.

Grammatic nit from a first quick glance at the interdiff. I'll still have a closer look at this either this evening or tomorrow.

+ // If this entity was translated using Entity Translation, we need to get
+ // it's source language to get the field values in the right language.

/s/it's/its

Markus

I now had a chance to have a closer look at the improvements #39 and read the whole patch #41 once more.

Extending from FieldableEntity instead of Node make sense to me. A couple of suggestions for the inline comments.

1.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d7/FieldableEntity.php
@@ -22,13 +22,19 @@
+    // For multilingual migrations support we need to know if a field is
+    // translatable so we join the field_config table where the translatable
+    // status is stored.
+    $query->addField('fc', 'translatable');
+    $query->leftJoin('field_config', 'fc', 'fci.field_id = fc.id');

Maybe this would flow better: Join the 'field_config' table and add the 'translatable' setting to the query.

2.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d7/FieldableEntity.php
@@ -58,6 +64,11 @@ protected function getFieldValues($entity_type, $field, $entity_id, $revision_id
+    // We only need to use language as a condition if this entity was translated
+    // using Entity Translation and if the field is set as translatable.
     if ($language) {
       $query->condition('language', $language);
     }

The comment is implying that there would be two conditions but the code itself is only checking if $language is set. Suggestion: Add language as a query condition if it has been defined by Entity Translation.

3.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d7/FieldableEntity.php
@@ -71,4 +82,44 @@ protected function getFieldValues($entity_type, $field, $entity_id, $revision_id
+   * Check if an entity type uses entity translation.

Method description should start with a third person verb. In addition, we're using 'Entity Translation' instead of 'entity translation' elsewhere.

4.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d7/FieldableEntity.php
@@ -71,4 +82,44 @@ protected function getFieldValues($entity_type, $field, $entity_id, $revision_id
+   * Get an entity source language from the entity translation table.

Ditto.

5.

+++ b/core/modules/node/src/Plugin/migrate/source/d7/Node.php
@@ -104,11 +104,23 @@ public function query() {
+      // If the entity and the field are translatable, add the language as a
+      // condition to retrieve the field values in the right language.
       $field_language = $entity_translatable && $field['translatable'] ? $language : NULL;
       $row->setSourceProperty($field_name, $this->getFieldValues('node', $field_name, $nid, $vid, $field_language));
     }

I find the inline comment a bit confusing since we are not adding any conditions here directly (the underlying query and its condition is an implementation detail of getFieldValues()).

Maybe this would flow better: Ensure we're using the right language if the entity and the field are translatable.

6.

+++ b/core/modules/node/src/Plugin/migrate/source/d7/NodeEntityTranslation.php
@@ -0,0 +1,117 @@
+/**
+ * Drupal 7 node entity translations source from database.
+ *

Class description must start with third person verb. In addition, we're using 'Entity Translation' instead of 'entity translation' elsewhere.

7.

+++ b/core/modules/node/src/Plugin/migrate/source/d7/NodeEntityTranslation.php
@@ -0,0 +1,117 @@
+      // If the field is translatable, add the language as a condition to
+      // retrieve the field values in the right language.
+      $field_language = $field['translatable'] ? $language : NULL;
+      $row->setSourceProperty($field_name, $this->getFieldValues('node', $field_name, $nid, $vid, $field_language));

Same comment as 5. Maybe this would flow better: Ensure we're using the right language if the entity is translatable.

8.

+++ b/core/modules/node/tests/src/Kernel/Plugin/migrate/source/d7/NodeEntityTranslationTest.php
@@ -0,0 +1,291 @@
+ * Tests D7 node entity translation source plugin.

/s/entity translation/Entity Translation

RTBC once the tests are green.

And the tests are now green. RTBC.

+++ b/core/modules/node/migrations/d7_node_entity_translation.yml
+++ b/core/modules/node/migrations/d7_node_entity_translation.yml
@@ -0,0 +1,31 @@

@@ -0,0 +1,31 @@
+id: d7_node_entity_translation
+label: Node entity translations
+migration_tags:
+  - Drupal 7
+  - translation
+  - Content

This now needs the 'Multilingual' migration tag I think, so it gets caught by migrate_drupal_multilingual?

-    $session->pageTextContains("Install migrate_drupal_multilingual to run migration 'd7_node_translation:article'.");
+    $session->pageTextContains("Install migrate_drupal_multilingual to run migration 'd7_node_entity_translation:article'.");
   }

Was the line break intentional?

Maybe it's my mobile browser then. 'd7_node_entity_translation:article' appeared to be on a new line in the interdiff.

Tests are back to green and the newline thing in the previous comments was just because the line did not fit to my screen. Back to RTBC.

Yay, thanks so much for bringing this issue to completion :) The stable changes look fine, eg. the language parameter to the node plugin. Let's fix the followups :)