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.

Files: 
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

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