Note: This is a clone from https://www.drupal.org/node/2225781 that deals with taxonomy vocabularies. This issue deals with taxonomy terms.

Problem/Motivation

i18ntaxonomy sub module adds two fields to the {term_data} that take care about the translations, we need to create the term entities using the new entity API.

Proposed resolution

We need to create a custom source plugin that takes into account these new fields in {term_data} and also the i18nstrings module features so we can use the new column trid for the migration.

Original report by Ryan Weal

Like nodes, taxonomies can use entity translation. Notes above in nodes should apply.

CommentFileSizeAuthor
#46 2784371-46.patch17.44 KBJo Fitzgerald
#46 interdiff-43-46.txt755 bytesJo Fitzgerald
#43 interdiff.txt656 bytesquietone
#43 2784371-43.patch17.57 KBquietone
#41 interdiff.txt12.26 KBquietone
#41 2784371-41.patch17.58 KBquietone
#37 2784371-37.patch17.71 KBquietone
#28 2784371-28.patch18 KBJo Fitzgerald
#28 interdiff-27-28.txt11.21 KBJo Fitzgerald
#27 2784371-27.patch17.92 KBJo Fitzgerald
#27 interdiff-22-27.txt14.67 KBJo Fitzgerald
#22 2784371-22.patch19.09 KBJo Fitzgerald
#21 interdiff.txt1.18 KBquietone
#21 2784371-21.patch19.12 KBquietone
#19 interdiff.txt2.43 KBquietone
#19 2784371-19.patch20.54 KBquietone
#17 interdiff.txt4.79 KBquietone
#17 2784371-17.patch22.88 KBquietone
#16 2784371-16.patch22.31 KBJo Fitzgerald
#16 interdiff-14-16.txt1.18 KBJo Fitzgerald
#14 2784371-14.patch21.73 KBJo Fitzgerald
#14 interdiff-12-14.txt4.23 KBJo Fitzgerald
#12 2784371-12.patch18.68 KBJo Fitzgerald
#12 interdiff-11-12.txt740 bytesJo Fitzgerald
#12 interdiff-10-12.txt1.19 KBJo Fitzgerald
#11 2784371-11.patch18.68 KBJo Fitzgerald
#11 interdiff-10-11.txt876 bytesJo Fitzgerald
#10 2784371-10.patch19.09 KBquietone
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

mpp created an issue. See original summary.

Gábor Hojtsy’s picture

Status: Needs work » Active

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

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

quietone’s picture

Looks like adding translations for taxonomy terms needs to be added to the fixture.

Gábor Hojtsy’s picture

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

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

DamienMcKenna’s picture

Cross-referencing the issue that's blocking this one.

quietone’s picture

quietone’s picture

Assigned: Unassigned » quietone

Should be able to post a first patch in a few days.

quietone’s picture

Here is a rough patch. All the files should be here, the source plugin, migration, tests and changes to the fixture. Although the changes to the fixture needs a closer look. The source plugin test passes locally but the not i18nTaxonomyTermTest. That results in the error below.

No reason to run the testbot.

1) Drupal\Tests\taxonomy\Kernel\Migrate\d6\MigrateI18nTaxonomyTermTest::testI18nTaxonomyTerms
SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate 6-0' for key 'PRIMARY': INSERT INTO {taxonomy_term_hierarchy} (tid, parent) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1), (:db_insert_placeholder_2, :db_insert_placeholder_3); Array
(
[:db_insert_placeholder_0] => 6
[:db_insert_placeholder_1] => 0
[:db_insert_placeholder_2] => 6
[:db_insert_placeholder_3] => 0
)
(/opt/sites/d8/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:770)
Failed asserting that false is true.

Jo Fitzgerald’s picture

Here's a little tweak that might shed some light, @quietone (but it still fails, although differently now, hence still not running the testbot).

Jo Fitzgerald’s picture

Assigned: quietone » Unassigned
Status: Needs work » Needs review
FileSize
1.19 KB
740 bytes
18.68 KB

Got it! @quietone you were so close.
#teamwork

Status: Needs review » Needs work

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

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
4.23 KB
21.73 KB

Edited a couple of tests to reflect changes to fixture.

I have also reverted a couple of fixture changes that were causing test failures:

  • reinstated url alias for node/1
  • edited hierarchy for two taxonomy vocabularies

@quietone let me know if these should be retained and we'll have edit the tests instead.

Status: Needs review » Needs work

The last submitted patch, 14: 2784371-14.patch, failed testing.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
1.18 KB
22.31 KB

Entity count change and a typo.

quietone’s picture

A few more fixes.

1) The yml ids
2) Added a requirement of d6_taxonomy_term to d6_i18n_taxonomy_term
3) Added tests for the parents to the Migration test. The parents are gathered in prepareRow, and should not be changed from the d6_taxonomy_term migration. I guess it isn't strictly necessary to include the parents in the migration.

That still leaves a closer examination of the changes to the fixture.

quietone’s picture

Status: Needs review » Needs work

Just discovered that there is an i18ntaxonomy_vocabulary variable. Anyone know if this needs to be migrated and even better, to what?

On a quick look, I think it is storing these for each vocabulary.
/**
* Modes for multilingual vocabularies.
*/
// No multilingual options
define('I18N_TAXONOMY_NONE', 0);
// Localizable terms. Run through the localization system.
define('I18N_TAXONOMY_LOCALIZE', 1);
// Predefined language for a vocabulary and its terms.
define('I18N_TAXONOMY_LANGUAGE', 2);
// Per-language terms, translatable (referencing terms with different languages) but not localizable.
define('I18N_TAXONOMY_TRANSLATE', 3);

quietone’s picture

Status: Needs work » Needs review
FileSize
20.54 KB
2.43 KB

Trim some more unnecessary changes to the fixture.

Status: Needs review » Needs work

The last submitted patch, 19: 2784371-19.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
19.12 KB
1.18 KB

Ah, apparently I wasn't working from HEAD in #17, removing changes to the Block migration test.

Jo Fitzgerald’s picture

Patch no longer applies. Re-rolled.

mikeryan’s picture

Assigned: Unassigned » mikeryan
jhodgdon’s picture

Just a question/note... On #2793947-14: D6 migration testing with i18n, today I tested migrating a d6 site that used i18n taxonomy and had some translated nodes and translated taxonomy terms.

I noticed that the Spanish translated nodes that had translated taxonomy terms attached to them, the migrated nodes didn't get the taxonomy terms associated. The English ones came through OK.

So... I just want to ask whether this issue/patch will take care of that problem, or is it just about migrating the terms themselves? I don't see anything in the test in this patch that addresses taxonomy fields on nodes. Do we need a separate issue? If so, is there one already? I don't see one in Related... Thanks!

jhodgdon’s picture

Actually, given the breakdown on the parent meta issue #2208401: [META] Multilingual migrations / i18n meta issue, I am fairly certain it should be a new issue, at least for a test. So I filed:
#2859297: Migrate d6 translated nodes with translated taxonomy

mikeryan’s picture

Assigned: mikeryan » Unassigned
Status: Needs review » Needs work
  1. +++ b/core/modules/migrate_drupal_ui/src/Tests/d6/MigrateUpgrade6Test.php
    --- /dev/null
    +++ b/core/modules/taxonomy/migration_templates/d6_i18n_taxonomy_term.yml
    
    +++ b/core/modules/taxonomy/migration_templates/d6_i18n_taxonomy_term.yml
    @@ -0,0 +1,38 @@
    +id: d6_i18n_taxonomy_term
    

    For nodes the translation migration corresponding to d6_node is d6_node_translation - I think for consistency we should name this d6_taxonomy_term_translation.

  2. +++ b/core/modules/taxonomy/src/Plugin/migrate/source/d6/I18nTerm.php
    @@ -0,0 +1,75 @@
    +class I18nTerm extends DrupalSqlBase {
    

    This source plugin is nearly identical to the d6_taxonomy_term plugin - it should inherit from that plugin class, then it only needs to override fields() to add 'language' and 'trid' to parent::fields().

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
14.67 KB
17.92 KB
  1. Renamed the migration template from d6_i18n_taxonomy_term to d6_taxonomy_term_translation (but not the migrate source plugin).
  2. I18nTerm now extends Term (with just the addition of language and trid to $fields).
Jo Fitzgerald’s picture

Renamed the migrate source plugin d6_i18n_taxonomy_term (and any related files and tests).

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, thanks!

The last submitted patch, 10: 2784371-10.patch, failed testing.

The last submitted patch, 11: 2784371-11.patch, failed testing.

quietone’s picture

Status: Reviewed & tested by the community » Postponed

Sadly, postponing this because the source plugin test is returning false positives. #2862006: MigrateSourceTestBase returns false positives for most plugin tests

heddn’s picture

Jo Fitzgerald’s picture

Status: Postponed » Reviewed & tested by the community

No longer blocked.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: 2784371-28.patch, failed testing.

Gábor Hojtsy’s picture

error: patch failed: core/modules/migrate_drupal/tests/fixtures/drupal6.php:45678
error: core/modules/migrate_drupal/tests/fixtures/drupal6.php: patch does not apply
error: patch failed: core/modules/migrate_drupal_ui/src/Tests/d6/MigrateUpgrade6Test.php:57
error: core/modules/migrate_drupal_ui/src/Tests/d6/MigrateUpgrade6Test.php: patch does not apply
quietone’s picture

Status: Needs work » Needs review
FileSize
17.71 KB

Reroll

phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Self-assigning for review.

phenaproxima’s picture

Status: Needs review » Needs work

Generally looking pretty good!! I'm really happy to see this coming along, and I'm sorry it took me forever to review it.

  1. +++ b/core/modules/taxonomy/migration_templates/d6_taxonomy_term_translation.yml
    @@ -0,0 +1,38 @@
    +label: Taxonomy terms
    

    This should mention the translations. So maybe "Taxonomy term translations" or "Taxonomy terms (translated)".

  2. +++ b/core/modules/taxonomy/migration_templates/d6_taxonomy_term_translation.yml
    @@ -0,0 +1,38 @@
    +  # If you are using this file to build a custom migration consider removing
    

    Nit: The should be a comma after the word "migration".

  3. +++ b/core/modules/taxonomy/migration_templates/d6_taxonomy_term_translation.yml
    @@ -0,0 +1,38 @@
    +  parent_id:
    +    -
    +      plugin: skip_on_empty
    +      method: process
    +      source: parent
    +    -
    +      plugin: migration
    +      migration: d6_taxonomy_term
    +  parent:
    +    plugin: default_value
    +    default_value: 0
    +    source: '@parent_id'
    

    I don't understand why these are two separate properties; couldn't we simply add default_value to parent_id's pipeline? If not, we need a comment as to why.

  4. +++ b/core/modules/taxonomy/migration_templates/d6_taxonomy_term_translation.yml
    @@ -0,0 +1,38 @@
    +  plugin: entity:taxonomy_term
    

    For strict YAML compatibility, 'entity:taxonomy_term' should be single-quoted.

  5. +++ b/core/modules/taxonomy/src/Plugin/migrate/source/d6/TermTranslation.php
    @@ -0,0 +1,27 @@
    +class TermTranslation extends Term {
    

    This class doesn't do much; I feel like we could merge all this directly into Term. If language is a column added by the i18n module, we could probably add a configuration key to Term which specifies that language and trid should be included, or check for the existence of the columns in the source table.

  6. +++ b/core/modules/taxonomy/src/Plugin/migrate/source/d6/TermTranslation.php
    @@ -0,0 +1,27 @@
    +    $fields['trid'] = $this->t('Not sure yet.');
    

    When will we be sure? :)

  7. +++ b/core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateTaxonomyTermTest.php
    @@ -83,8 +86,9 @@ public function testTaxonomyTerms() {
    +      $this->assertIdentical("{$language}term {$tid} of vocabulary {$values['source_vid']}", $term->name->value);
    +      $this->assertIdentical("{$language}description of term {$tid} of vocabulary {$values['source_vid']}", $term->description->value);
    

    We should use assertSame().

  8. +++ b/core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateTaxonomyTermTranslationTest.php
    @@ -0,0 +1,129 @@
    + * Upgrade taxonomy terms.
    

    Should be "Tests migration of translated taxonomy terms."

  9. +++ b/core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateTaxonomyTermTranslationTest.php
    @@ -0,0 +1,129 @@
    +  public static $modules = [
    

    Needs @inheritdoc.

  10. +++ b/core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateTaxonomyTermTranslationTest.php
    @@ -0,0 +1,129 @@
    +   *   The term reference id the migrated entity field should have.
    

    "id" should be "ID".

  11. +++ b/core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateTaxonomyTermTranslationTest.php
    @@ -0,0 +1,129 @@
    +    $this->assertTrue($entity instanceof TermInterface);
    

    This should be assertInstanceOf().

  12. +++ b/core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateTaxonomyTermTranslationTest.php
    @@ -0,0 +1,129 @@
    +    $this->assertEquals($expected_description, $entity->getDescription());
    +    $this->assertEquals($expected_format, $entity->getFormat());
    +    $this->assertEquals($expected_weight, $entity->getWeight());
    

    Let's use assertSame() here.

  13. +++ b/core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateTaxonomyTermTranslationTest.php
    @@ -0,0 +1,129 @@
    +    $this->assertEquals($parent_ids, array_filter($term->parents), "Term $tid has correct parents in taxonomy tree");
    

    I feel like the array_filter() call is a bit dangerous. If there are empty parent IDs, this could indicate some sort of problem with the hierarchy. Can we add a comment explaining why we're calling array_filter()?

  14. +++ b/core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateTaxonomyTermTranslationTest.php
    @@ -0,0 +1,129 @@
    +  public function testI18nTaxonomyTerms() {
    

    Can we rename this testTranslatedTaxonomyTerms(), for readability?

  15. +++ b/core/modules/taxonomy/tests/src/Kernel/Plugin/migrate/source/d6/TermTranslationTest.php
    @@ -0,0 +1,206 @@
    +class TermTranslationTest extends MigrateSqlSourceTestBase {
    

    If we merge the TermTranslation plugin into Term, as I suggested before, we can merge all of this into Term's test as another "pass" from the data provider.

quietone’s picture

Assigned: phenaproxima » quietone

Working on this. But it is taking awhile due to my cold.

quietone’s picture

Status: Needs work » Needs review
FileSize
17.58 KB
12.26 KB

1. Fixed
2. Let's make that change to all 15 occurrences of that sentence in a separate issue. Those lines were added by #2818157: Add comments to remove entity ids in migration.
3. This is just copy/paste from d6_taxonomy_term.yml. It looks like that was discussed herė https://www.drupal.org/node/2744639#comment-11285559. Í've only to find that discussion, not read it, as it is time to take me and cold to bed for a nap.
4. The existing migrations throughout core are not using single quotes. Maybe we should have an separate issue to meet strict YAML compatibility requirements.
5, 15. The class has been removed, added a 'translations' configuration key.
6. Oops, and fixed.
7. Fixed
8. Fixed
9. Fixed
10. Fixed
11. Fixed
12. Fixed
13. The hierarchy tests comes from d7/MigrateTaxonomyTermTest. It too was added in #274639: Unable to create fields / groups ... Possible table problem?.
14. Fixed

And I'm wondering if the destination should also use the key 'translations'.

Status: Needs review » Needs work

The last submitted patch, 41: 2784371-41.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
17.57 KB
656 bytes

Fix @covers to test the correct plugin.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Okay, I'm happy with the work done in #41. Proceed!

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateTaxonomyTermTranslationTest.php
@@ -0,0 +1,132 @@
+  public static $modules = [
+    'language',
+    'content_translation',
+    'comment',
+    'datetime',
+    'image',
+    'locale',
+    'link',
+    'menu_ui',
+    'node',
+    'taxonomy',
+    'telephone',
+    'text',
+  ];

Do we need all of these modules? Ones surprising me are: comment, datetime, image, locale, menu_ui, telephone.

Jo Fitzgerald’s picture

Assigned: quietone » Unassigned
Status: Needs work » Needs review
FileSize
755 bytes
17.44 KB

I did some digging and @Gábor Hojtsy is mostly correct: comment, datetime, image, locale and telephone are unnecessary. BUT a schema within menu_ui is required. In addition, content_translation, link and text are unnecessary.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Nice catch by Gábor, good to see this a little cleaner :) Back to RTBC!

  • Gábor Hojtsy committed a001cd8 on 8.4.x
    Issue #2784371 by Jo Fitzgerald, quietone, phenaproxima, Gábor Hojtsy,...
Gábor Hojtsy’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Superb, thanks a lot!

jhodgdon’s picture

Status: Fixed » Needs work

I tested the latest 8.4.x today by migrating a d6 site that had translated taxonomy terms (using the d6 i18n taxonomy contrib module). The taxonomy terms did not come through with their translations. So, I don't think this fix actually works?

jhodgdon’s picture

jhodgdon’s picture

I just looked at the last patch on this issue. I don't see anything in that patch that deals with *translations* of terms. I see some code and test fixtures that deal with taxonomy term *language*, but nothing for my use case, which is having a particular taxonomy vocabulary whose terms have been translated from English into Spanish using the i18n_taxonomy module.

Was this issue supposed to cover that use case or do we need to create a new issue? Because as far as I can tell, the migration in this patch just covers migrating the language of a given term, not term translations.

Gábor Hojtsy’s picture

Status: Needs work » Fixed

Right, so i18ntaxonomy alters the terms/vocabularies schemas a bit:

/**
 * Implementation of hook_schema_alter().
 */
function i18ntaxonomy_schema_alter(&$schema) {
  $schema['vocabulary']['fields']['language'] = array('type' => 'varchar', 'length' => 12, 'not null' => TRUE, 'default' => '');
  $schema['term_data']['fields']['language'] = array('type' => 'varchar', 'length' => 12, 'not null' => TRUE, 'default' => '');
  $schema['term_data']['fields']['trid'] = array('type' => 'int', 'not null' => TRUE, 'default' => 0);
}

(from http://cgit.drupalcode.org/i18n/tree/i18ntaxonomy/i18ntaxonomy.install?h...)

This issue does in fact only cover a bit of that functionality, where the term language is migrated. It still did not cover trid (taxonomy term translation sets where the terms are different, although I don't think core can do that in Drupal 8). It also did not cover translations which is also an i18ntaxonomy option. So yeah those would need their own issues.

All the options listed at http://cgit.drupalcode.org/i18n/tree/i18ntaxonomy/i18ntaxonomy.module?h=... (for Drupal 6).

Let's not reopen this issue, which did cover one of the options, and open/repurpose new ones :)

Gábor Hojtsy’s picture

Title: Migrate D6 i18n taxonomy terms » Migrate D6 i18n taxonomy term language (but not yet translations)

Some key words in the title.

jhodgdon’s picture

Fair enough. #2886609: Migrate D6 i18n translations of taxonomy vocabulary/terms is created. I'll add it to the parent issue too.

  • xjm committed 6800ac3 on 8.3.x authored by Gábor Hojtsy
    Issue #2784371 by Jo Fitzgerald, quietone, phenaproxima, Gábor Hojtsy,...

Status: Fixed » Closed (fixed)

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