d7_node_entity_translation migration is in the wrong module and is missing some mappings

Test tested this manually and analyzed node_field_data database table.

1. Translation outdated ('content_translation_outdated') property is now correctly migrated. OK.

2. Migration dependencies for 'language' and 'd7_entity_translation_settings' have now been added as described in the IS.

3. I also did manual regression testing, all OK.

• The relations between translations ('default_langcode' for the original + 'content_translation_source' for translations) are correctly migrated, checked from both database and UI.
• titles and langcodes are correctly migrated.
• 'created' and 'changed' are correctly migrated.
• author of the translation is correctly migrated.
• translation publishing status is correctly migrated.

4. Question:
D8 node_field_data table has a column 'revision_translation_affected'. What is this property? We are not mapping it here. Will that be part of some other issue, such as #2746541: Migrate D6 and D7 node revision translations to D8?

5. I reviewed test coverage for the source plugin, OK

• We are now testing the author of the translation (uid)
• We are now testing the translation publishing status (status)
• We are now testing the translation outdated status (translate)

6. I reviewed test coverage for MigrateNodeTest, NOT OK
Here are the asserts:

  protected function assertEntity($id, $type, $langcode, $title, $uid, $status, $created, $changed, $promoted, $sticky) {
    /** @var \Drupal\node\NodeInterface $node */
    $node = Node::load($id);
    $this->assertInstanceOf(NodeInterface::class, $node);
    $this->assertEquals($type, $node->getType());
    $this->assertEquals($langcode, $node->langcode->value);
    $this->assertEquals($title, $node->getTitle());
    $this->assertEquals($uid, $node->getOwnerId());
    $this->assertEquals($status, $node->isPublished());
    $this->assertEquals($created, $node->getCreatedTime());
    if (isset($changed)) {
      $this->assertEquals($changed, $node->getChangedTime());
    }
    $this->assertEquals($promoted, $node->isPromoted());
    $this->assertEquals($sticky, $node->isSticky());
  }

We don't have tests for

• default_langcode
• content_translation_source
• content_translation_outdated
• (revision_translation_affected, see point 4. above)

Could we please have test coverage for default_langcode, content_translation_source and content_translation_outdated? Kicking back to NW for those.

Cheers,
Markus

About 'revision_translation_affected', I really don't know how that property works exactly, and even less how and if we can migrate it.

I think @plach would be better qualified to weigh in on this authoritatively, but I believe revision_translation_affected is a completely internal field, used by the translation system, and should not be touched by us if we can at all avoid it.

1. +++ b/core/modules/content_translation/migrations/d7_node_entity_translation.yml
   @@ -23,10 +23,13 @@ process:
   +  content_translation_outdated: translate
   

   This could benefit from a comment explaining what this mapping is for. It's not at all obvious, and this is the sort of obscure sorcery that could come back later to bite us in the ass.

2. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7NoMultilingualTest.php
   @@ -207,7 +207,7 @@ public function testMigrateUpgradeExecute() {
   +    $session->pageTextContains("Install migrate_drupal_multilingual to run migration 'd7_node_translation:article'.");
   

   Why did this change?

3. +++ b/core/modules/node/src/Plugin/migrate/source/d7/NodeEntityTranslation.php
   @@ -88,14 +79,16 @@ public function prepareRow(Row $row) {
   +      'entity_id' => $this->t('The entity id this translation relates to'),
   +      'revision_id' => $this->t('The entity revision id this translation relates to'),
   

   'id' should be 'ID'.

4. +++ b/core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeTest.php
   @@ -109,22 +120,33 @@ protected function getFileMigrationInfo() {
   +   * @param string $field_integer
   +   *   The expected value of the field_integer field.
   +   * @param bool $default_translation
   +   *   Whether the node is expected to be the default translation.
   +   * @param string $source_langcode
   +   *   The expected translation source langcode.
   +   * @param bool $translation_outdated
   +   *   Whether the translation is expected to be outdated.
   

   I agree with @maxocub that this stuff shouldn't be in assertEntity(). We are dangerously close to over-abstracting the assertions; in tests, it's okay to repeat some code like this.

Reading the patch I see that all the changes in #10 have been addressed.

I haven't read the whole issue but see that phenaproxima suggested speaking to plach about #9.4. Is that still needed ?

Okay, so...let's just remove revision_translation_affected from the migration and see if any tests break. If they don't, great! If they do, we should understand exactly why they break and be able to justify to @plach why we should migrate that field.

Peekaboo, getting back to this issue after a while.

I tested this manually in #8. The changes in #9 (after my review) and #12 (after @phenaproxima's review) are so minor that my manual test results are still valid.

The only remaining topic that has been discussed is the D8 'revision_translation_affected' property. I did some research on this as well and came to the same conclusion with @maxocub in #16. There is no equivalent property in D7 as far as I can tell. The current patch does not map this property, which is correct behavior.

Triggering the testbot for PostgreSQL and SQLite. Once those are green, this is RTBC.

Cheers,
Markus

Per #17, moving to RTBC.

Does not apply to Drupal 8.7 currently:

$ git apply --index 2989627-12.patch 
error: patch failed: core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7NoMultilingualTest.php:207
error: core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7NoMultilingualTest.php: patch does not apply

Hm, you made this slight logic change but no tests failed and no tests needed to be updated. Looks like if there was no entity translation source language on the node the behaviour would be now different?

Re-parented so that all remaining multilingual migration issues can be found from the same meta. Moved the previous parent issue to related issues.

Re #21. I cross checked what we have in the other Entity Translation source plugins.

Proposed code (from #21) for the Node source plugin that is being discussed here:

+++ b/core/modules/node/src/Plugin/migrate/source/d7/Node.php
@@ -113,7 +113,8 @@ public function prepareRow(Row $row) {
     $entity_translatable = $this->isEntityTranslatable('node') && (int) $this->variableGet('language_content_type_' . $type, 0) === 4;
-    $language = $entity_translatable ? $this->getEntityTranslationSourceLanguage('node', $nid) : $row->getSourceProperty('language');
+    $source_language = $this->getEntityTranslationSourceLanguage('node', $nid);
+    $language = $entity_translatable && $source_language ? $source_language : $row->getSourceProperty('language');
 
     // Get Field API field values.

User source plugin:

+    $entity_translatable = $this->isEntityTranslatable('user');
+    $source_language = $this->getEntityTranslationSourceLanguage('user', $uid);
+    $language = $entity_translatable && $source_language ? $source_language : $row->getSourceProperty('language');
+    $row->setSourceProperty('entity_language', $language);
+
     // Get Field API field values.

Taxonomy term source plugin:

+    $translatable_vocabularies = array_keys(array_filter($this->variableGet('entity_translation_taxonomy', [])));
+    $entity_translatable = $this->isEntityTranslatable('taxonomy_term') && in_array($vocabulary, $translatable_vocabularies, TRUE);
+    $source_language = $this->getEntityTranslationSourceLanguage('taxonomy_term', $tid);
+    $language = $entity_translatable && $source_language ? $source_language : $default_language['language'];
+    $row->setSourceProperty('language', $language);
+
     // Get Field API field values.

Comment source plugin:

+    $entity_translatable = $this->isEntityTranslatable('comment') && (int) $this->variableGet('language_content_type_' . $node_type, 0) === 4;
+    $source_language = $this->getEntityTranslationSourceLanguage('comment', $cid);
+    $language = $entity_translatable && $source_language ? $source_language : $row->getSourceProperty('language');
+
+    // Get Field API field values.

The determination of $language is consistent.

@maxocub: I did not have time on the gate to check this but why do we have $row->setSourceProperty('language', $language); in user and taxonomy term source plugins but not in node and comment source plugins?

Cheers,
Markus

@maxocub, Just took a wee look and I have a question.

+++ b/core/modules/node/tests/src/Kernel/Plugin/migrate/source/d7/NodeTest.php
@@ -422,6 +422,139 @@ public function providerSource() {
+    // The source data with the source language missing from the
+    // entity_translation table.
...
+        'source' => 'en',

This looks like the source language. What field is supposed to be missing?

Thanks. I still have trouble understanding what is missing.

So, for the first time, I used entity translation with the test fixture. It took a while but I was finally getting new entries in the entity_translation table. So, when I make a new node a row is added to that table with the source language property empty. When that node is translated a new row is added with language = fr and source = en. Which is correct the source was English and the translation was French.

So, from that, I think what it meant by missing in the comment is that the row where language = the first language used, and the source is empty is what is missing. That is a row in the table is missing not that the property source is missing. Is that right?

Phew, glad I got that right. As for a suggestion, let's see.

+++ b/core/modules/node/tests/src/Kernel/Plugin/migrate/source/d7/NodeTest.php
@@ -422,6 +422,140 @@ public function providerSource() {
+    // The source data with the source language node missing from the
+    // entity_translation table. The source language node should have an empty
+    // source column. Here we only have the French translation.

The source data with a row missing from the entity_translation table. There should be a row where the language property is set and the source property is empty.

How about that?

Hmm, may be better to have two tests? Would it make sense to modify tests[1] to have that extra row and then make tests[2] the same as the current tests[1]? The first could have a comment 'Test with a correct entity_translation table' and then test[2] could be 'Repeat the previous test with an incorrect entity_translation table where the row with source property is empty is missing'.

Currently leaning to the second suggestion.

@maxocub, thanks for working through this. It is really clear and obvious what is going on in the test now.

Since the last RTBC there have been questions which have all been answered and the need for a new test was discovered. That test was added and then another test was added with comments to make it clear what a 'good set' of source data looked like and a 'bad set' of source data.

Back to RTBC!

Thanks for adding a test for the logic improvement, looks much better now.