Missing condition in d7_term_localized_translation source plugin causing invalid migration

@herved, are you saying that the migration of the translation of the name and description for term 1 is correct and that the migration for the name and description for term 2 uses the name and description for term 1?

I do not believe that is true and it is tested in d7\MigrateTermLocalizedTranslationTest::testTranslatedLocalizedTaxonomyTerms where two terms with unique names and descriptions and translations are migrated.

Oh and the condition ( ->condition('i18n.objectid', $object_id);) you suggest be added is actually done in I18nQueryTrait, in order to get the translation for the property not already on the row. That is where we get the translations for name and description and put them on the row.

If I have missed your point please copy your source data and the results you are getting and what you expect to get.

@herved, thanks, I now see the problem. I don't know why this didn't use i18nQueryTrait properly, that was a mistake.

This will also need a migration test. That is, some new entries in the drupal7 fixture and the corresponding assertions in MigrateTermLocalizedTranslationTest.

Starting from the patch in #4 I expanded the source plugin test as well as the Kernel migration test.

That should have been testing with 9.1.x not 8.8.x. Changed the fixture to avoid the word 'Bajoran' and updated the tests.

Two corrections to the entity counts in Upgrade7Test

I checked the D6 source plugin and it does not use this fix, although ideally it could be modified to use I18nQueryTrait.

edit: s/use/need/

Another change to entity count

And again.

working on it.

@shaktik, hope you see this. I've already made a patch and just running tests locally.

Hi @quietone,

forgot to add interdiff added interdiff_16-19, just solved test case issue.

diff --git a/core/modules/taxonomy/tests/src/Kernel/Migrate/d7/MigrateTaxonomyTermTest.php b/core/modules/taxonomy/tests/src/Kernel/Migrate/d7/MigrateTaxonomyTermTest.php
index de8908a553..de22f1db94 100644
--- a/core/modules/taxonomy/tests/src/Kernel/Migrate/d7/MigrateTaxonomyTermTest.php
+++ b/core/modules/taxonomy/tests/src/Kernel/Migrate/d7/MigrateTaxonomyTermTest.php
@@ -126,7 +126,7 @@ public function testTaxonomyTerms() {

     // Reset the forums tree data so this new term is included in the tree.
     unset($this->treeData['forums']);
-    $this->assertEntity(25, 'en', 'Forum Container', 'forums', '', '', 0, [], NULL, NULL, 1);
+    $this->assertEntity(26, 'en', 'Forum Container', 'forums', '', '', 0, [], NULL, NULL, 1);

     // Test taxonomy term language translations.

@shaktik, In the future, please check if someone is actively working on a patch before assigning it to your self. I think that uploading 5 patches in the last 24 hours, which I have done here, when you assigned it to yourself qualifies as actively working on a patch. There are plenty of migrations issues tagged, Needs Work, that haven't had any updates in weeks and if you wish to start on one of those I am happy to assist.

The patch in #20 will fix the failing test but this should have an assertion added that the term in the newly added localized vocabulary is added.

Hi @quietone,

This issue as Unassigned state that's why I have assigned my self and worked on it.

Thanks,
Shakti.

@shaktik, I see, but the fact that an issue is not assigned to someone does not mean that it isn't actively being worked on. One needs to look at the recent comments and see what has been happening to decide if it is appropriate to assign it to yourself. The guidelines for Assigning ownership do discourage one from assigning an issue to oneself.

What I was experiencing was the description would use a localized variable from another entity type (i.e. menu instead of term). I managed to resolve this issue in 8.9.x with the attached patch.

Re-uploading and re-testing the patch from #21 as that is the correct one with tests.

Rerolled the patch

Looks like a related test failure that needs addressing.

Fixing failed tests.

Thanks for the rerolls. Please remember to update the tags when the reroll is complete.

1. +++ b/core/modules/taxonomy/tests/src/Kernel/Plugin/migrate/source/d7/TermLocalizedTranslationTest.php
   @@ -124,7 +126,54 @@ public function providerSource() {
   +       'plid' => 0,
   

   Indentation error

2. +++ b/core/modules/taxonomy/src/Plugin/migrate/source/d7/TermLocalizedTranslation.php
   @@ -17,6 +17,11 @@ class TermLocalizedTranslation extends Term {
   +  /**
   +   * Drupal 7 table name.
   +   */
   +  const I18N_STRING_TABLE = 'i18n_string';
   +
   

   This isn't necessary, let's remove the use of the constant.

3. +++ b/core/modules/taxonomy/src/Plugin/migrate/source/d7/TermLocalizedTranslation.php
   @@ -34,7 +39,7 @@ public function query() {
   +    $query->leftJoin(static::I18N_STRING_TABLE, 'i18n', 'CAST(td.tid AS CHAR(255)) = i18n.objectid');
   
   @@ -50,38 +55,22 @@ public function query() {
   +    $this->i18nStringTable = static::I18N_STRING_TABLE;
   

   Here too.

Made the changes suggested in #31. Please Review

I reviewed the patch in #33 and it fixes all the points in #31. I think this is RTBC but I can't change the status to that since I have made several patches here.

+1 for RTBC

I agree.

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/Upgrade7Test.php
@@ -127,7 +127,7 @@ protected function getEntityCountsIncremental() {
   }
diff --git a/core/modules/taxonomy/src/Plugin/migrate/source/d7/TermLocalizedTranslation.php b/core/modules/taxonomy/src/Plugin/migrate/source/d7/TermLocalizedTranslation.php

diff --git a/core/modules/taxonomy/src/Plugin/migrate/source/d7/TermLocalizedTranslation.php b/core/modules/taxonomy/src/Plugin/migrate/source/d7/TermLocalizedTranslation.php
index f61f384b69..970e1b85c6 100644

index f61f384b69..970e1b85c6 100644
--- a/core/modules/taxonomy/src/Plugin/migrate/source/d7/TermLocalizedTranslation.php

--- a/core/modules/taxonomy/src/Plugin/migrate/source/d7/TermLocalizedTranslation.php
+++ b/core/modules/taxonomy/src/Plugin/migrate/source/d7/TermLocalizedTranslation.php

+++ b/core/modules/taxonomy/src/Plugin/migrate/source/d7/TermLocalizedTranslation.php
+++ b/core/modules/taxonomy/src/Plugin/migrate/source/d7/TermLocalizedTranslation.php
@@ -50,38 +50,22 @@ public function query() {

@@ -50,38 +50,22 @@ public function query() {
    * {@inheritdoc}
    */
   public function prepareRow(Row $row) {
+    parent::prepareRow($row);
+
+    // Override language with ltlanguage.
     $language = $row->getSourceProperty('ltlanguage');
-    $tid = $row->getSourceProperty('tid');
+    $row->setSourceProperty('language', $language);
 
-    // If this row has been migrated it is a duplicate then skip it.
-    if ($this->idMap->lookupDestinationIds(['tid' => $tid, 'language' => $language])) {
-      return FALSE;
-    }
+    // Set the i18n string table for use in I18nQueryTrait.
+    $this->i18nStringTable = 'i18n_string';
 
     // Save the translation for the property already in the row.
     $property_in_row = $row->getSourceProperty('property');
-    $row->setSourceProperty($property_in_row . '_translated', $row->getSourceProperty('translation'));
 
     // Get the translation for the property not already in the row and save it
     // in the row.
     $property_not_in_row = ($property_in_row == 'name') ? 'description' : 'name';
-
-    // Get the translation, if one exists, for the property not already in the
-    // row.
-    $query = $this->select('i18n_string', 'i18n')
-      ->fields('i18n', ['lid'])
-      ->condition('i18n.property', $property_not_in_row);
-    $query->leftJoin('locales_target', 'lt', 'i18n.lid = lt.lid');
-    $query->condition('lt.language', $language);
-    $query->addField('lt', 'translation');
-    $results = $query->execute()->fetchAssoc();
-    if (!$results) {
-      $row->setSourceProperty($property_not_in_row . '_translated', NULL);
-    }
-    else {
-      $row->setSourceProperty($property_not_in_row . '_translated', $results['translation']);
-    }
-    parent::prepareRow($row);
+    return $this->getPropertyNotInRowTranslation($row, $property_not_in_row, 'tid', $this->idMap);
   }

This is very nice cleanup.

Committed 876073f and pushed to 9.1.x. Thanks!