Migrate D6 i18n taxonomy vocabularies

Blocked by #2594263: Add translation data to Migrate Drupal's test fixtures.

Unblocked

A work in progress.

This needs to truncate the vocabulary name to 32 chars. So, pulled the substr part of the dedupe process plugin into it's own plugin. Maybe this should go into Migrate?

Marking as postponed, as this relies on the data in #2670170: Add i18n string & variable data to d6_dump.

rerolled for 8.1.x

Failure was due to accidentally including a working copy of the test.

This needs to truncate the vocabulary name to 32 chars. So, pulled the substr part of the dedupe process plugin into it's own plugin. Maybe this should go into Migrate?

Why not use dedupe_entity on the vocabulary name?

The substr plugin does seem generally useful - if we're going to add it, that should be its own issue (with its own tests).

1. +++ b/core/modules/config_translation/src/Plugin/migrate/destination/I18nTaxonomyVocabulary.php
   @@ -0,0 +1,90 @@
   + *   id = "i18n_taxonomy_vocabulary"
   

   This destination plugin does not seem to actually be used anywhere? The new migration uses a destination of entity:taxonomy_vocabulary.

2. +++ b/core/modules/config_translation/src/Plugin/migrate/destination/I18nTaxonomyVocabulary.php
   @@ -0,0 +1,90 @@
   +  public function fields(MigrationInterface $migration = NULL) {
   +  }
   

   fields() must return an array (the fact that the Entity destination does not has its own issue: #2630732: Implement Entity::fields() for migration destinations).

3. +++ b/core/modules/config_translation/src/Plugin/migrate/source/d6/I18nTaxonomyVocabulary.php
   @@ -0,0 +1,70 @@
   +    $query->Join('i18n_strings', 'i18n', 'i18n.objectid = v.vid');
   

   s/Join/join/

Why not use dedupe_entity on the vocabulary name?

Because we want to add the translated terms to an existing vocabulary. Dedupe will detect that a given vocabulary name exists and create a new one. And then the translated terms would be added to that new vocabulary not the existing one.

Created a new issue for the substr plugin, #2752591: Add substr process plugin. This is now postponed on that.

1-2. Removed the extraneous destination plugin.
3. Fixed

substr is in!

Thanks for working on this, reviewed!

Also the summary says this would deal with term_data, while it in fact deals with the vocabulary itself and not the terms. That is a good way to approach the problem, but I think we should clone the issue summary for a new issue and repurpose this for the vocabulary then (so we don't try to do both at once and since this is about the vocabulary in practice already).

1. index 0000000..c5b716e
   --- /dev/null
   
   --- /dev/null
   +++ b/core/modules/config_translation/migration_templates/d6_i18n_taxonomy_vocabulary.yml
   
   +++ b/core/modules/config_translation/migration_templates/d6_i18n_taxonomy_vocabulary.yml
   @@ -0,0 +1,25 @@
   

   Should not use templates anymore, should we?

2. +++ b/core/modules/config_translation/src/Plugin/migrate/source/d6/I18nTaxonomyVocabulary.php
   @@ -0,0 +1,70 @@
   +  /**
   +   * The source table containing profile field info.
   +   *
   +   * @var string
   ...
   +  /**
   +   * The source table containing the profile values.
   +   *
   

   Some copy-paste comments.

3. +++ b/core/modules/config_translation/src/Plugin/migrate/source/d6/I18nTaxonomyVocabulary.php
   @@ -0,0 +1,70 @@
   +  public function query() {
   +    $query = $this->select('vocabulary', 'v')
   +      ->fields('v', array('vid', 'name', 'description'))
   +      ->fields('i18n', array('lid', 'type', 'property', 'objectid'))
   +      ->fields('lt', array('lid', 'translation'))
   +      ->condition('i18n.type', 'vocabulary');
   

   This kind of query would fail if the i18n module was not enabled. Where does it ensure that this depends on i18n module in the source?

4. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
   @@ -70,10 +121,18 @@ public function getIds() {
   -    foreach ($row->getRawDestination() as $property => $value) {
   -      $this->updateEntityProperty($entity, explode(Row::PROPERTY_SEPARATOR, $property), $value);
   +    if ($row->hasDestinationProperty('langcode') && ($this->languageManager instanceof ConfigurableLanguageManagerInterface)) {
   +      // Then this is a row with a translation string.
   +      $config = $entity->getConfigDependencyName();
   +      $config_override = $this->languageManager->getLanguageConfigOverride($row->getDestinationProperty('langcode'), $config);
   

   The original entity may also have a langcode of its own. So IMHO we should check if different from the original.

Is this issue fixed?

Also the summary says this would deal with term_data, while it in fact deals with the vocabulary itself and not the terms. That is a good way to approach the problem, but I think we should clone the issue summary for a new issue and repurpose this for the vocabulary then (so we don't try to do both at once and since this is about the vocabulary in practice already).

See https://www.drupal.org/node/2784371.

@viirak: it would not be set to needs work if it would be fixed.

@mpp: thanks! Now this is a much smaller scope, do you think you may be able to work on this? :)

@Gábor, thanks. I just did not see any more activities while waiting for it. I think this is also important as i18n node translation. Usually sites that have node translations also have term translations. Hence, without this patch D6 with i18n term translation migrations are broken, nodes are not properly connected to the correct terms.

@Gábor, atm I solve taxonomy/vocabulary translations with an event subscriber, it doesn't scale well from a code perspective (it requires 1 column per language) but I also experience an exponential performance issue with migrations (https://www.drupal.org/node/2765729). That problem makes my multilingual migrations much slower as this approach requires to process each language separately 0(nxnxc) n=items to migrate; c=amount of languages. Once that issue is solved I can look into this.

<?php

/**
 * @file
 * Contains \Drupal\my_migrate\EventSubscriber\MigrationEventSubscriber.
 */

namespace Drupal\my_migrate\EventSubscriber;

use Drupal\Component\Utility\Unicode;
use Drupal\config_translation\ConfigMapperManagerInterface;
use Drupal\Core\Language\LanguageManagerInterface;
use Drupal\migrate\Event\MigrateEvents as MigrateEventsCore;
use Drupal\migrate\Event\MigratePostRowSaveEvent;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

class MigrationEventSubscriber implements EventSubscriberInterface {

  /**
   * The mapper plugin discovery service.
   *
   * @var \Drupal\config_translation\ConfigMapperManagerInterface
   */
  protected $configMapperManager;

  /**
   * The language manager.
   *
   * @var \Drupal\Core\Language\LanguageManagerInterface
   */
  protected $languageManager;

  /**
   * Constructor.
   *
   * @param \Drupal\config_translation\ConfigMapperManagerInterface $config_mapper_manager
   *   The mapper plugin discovery service.
   * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager
   *   The language manager service.
   */
  public function __construct(ConfigMapperManagerInterface $config_mapper_manager, LanguageManagerInterface $language_manager) {
    $this->configMapperManager = $config_mapper_manager;
    $this->languageManager = $language_manager;
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('language_manager')
    );
  }

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents() {
    $events[MigrateEventsCore::POST_ROW_SAVE][] = array('onPostRowSave');
    return $events;
  }

  /**
   * MigrateEvents::POST_ROW_SAVE event handler.
   *
   * Update taxonomy translations.
   *
   * @param \Drupal\migrate\Event\MigratePostRowSaveEvent $event
   *   The post-row-save event.
   */
  public function onPostRowSave(MigratePostRowSaveEvent $event) {
    $migration_id = $event->getMigration()->getBaseId();

    if ($migration_id == 'migrate_vocabularies') {
      // Get source values.
      $source_values = $event->getRow()->getSource();

      // Enable taxonomy translation.
      \Drupal::service('content_translation.manager')->setEnabled('taxonomy_term', $event->destinationIdValues[0], TRUE);
      \Drupal::service('entity.definition_update_manager')->applyUpdates();

      // Translate taxonomy name.
      $name = 'taxonomy.vocabulary.' . $event->destinationIdValues[0];
      $config_translation = $this->languageManager->getLanguageConfigOverride('nl', $name);
      $config_translation->set('name', Unicode::truncate($source_values['name_nl'], 255));
      $config_translation->save();
    }

    // Translate taxonomy terms.
    if ($migration_id == 'migrate_terms') {
      $migrated_entity = $event->destinationIdValues[0];
      $entity = entity_load('taxonomy_term', $migrated_entity);
      try {
        // Load translation.
        $translated_entity = $entity->getTranslation('nl');
      }
      catch (\InvalidArgumentException $e) {
        // Create translation.
        $translated_entity = $entity->addTranslation('nl');
      }
      $translated_entity->name = Unicode::truncate($source_values['name_nl'], 255);
      $translated_entity->save();

      // Save mapping.
      $map = $event->getMigration()->getIdMap();
      $map->saveIdMapping($event->getRow(), array($migrated_entity));
    }
  }

}

@mpp, any patch for this?

Rerolled the patch in #11. Also, instead of having 2 Migrate Taxonomy Vocabulary tests, one with i18n and one without, they are merged into one test file, MigrateTaxonomyVocabularyTest. The same was done in the d6 MigrateNodeTest.

Rerolled to use the new test base class for source plugins.

This should fix I18nVocabularyTest but not MigrateUpgrade6Test.

This and a couple of other i18n issues that make changes to EntityConfigBase destintations. Of those I believe #2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields has received the most feedback from Gábor Hojtsy and a review from phenaproxima (who has asked for a test for the changes to the destination).

Therefor, I am marking this as Postponed, until the destination is finalized in #2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields.

Yeah it would be important to commit this at one place instead of working on the same thing in parallel, thanks @quietone, was about to say the same :)

#2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields is in so no longer postponed.

Oops, it is RTBC not fixed.

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

#2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields is in, no loner postponed.

Reroll

Thanks, quick review:

1. +++ b/core/modules/taxonomy/migration_templates/d6_i18n_taxonomy_vocabulary.yml
   @@ -0,0 +1,24 @@
   +id: d6_i18n_taxonomy_vocabulary
   +label: Taxonomy vocabularies
   +migration_tags:
   +  - Drupal 6
   +source:
   +  plugin: d6_i18n_taxonomy_vocabulary
   +process:
   +  vid:
   +    -
   +      plugin: machine_name
   +      source: name
   +    -
   +      plugin: substr
   +      length: 32
   +  langcode: language
   +  property:
   +    plugin: static_map
   +    source: property
   +    map:
   +      name: name
   +      description: description
   +  translation: translation
   +destination:
   +  plugin: entity:taxonomy_vocabulary
   

   This should depend on taxonomy and i18nstrings no?

2. +++ b/core/modules/taxonomy/src/Plugin/migrate/source/d6/I18nTaxonomyVocabulary.php
   @@ -0,0 +1,66 @@
   +    return array(
   +      'vid' => $this->t('The vocabulary ID.'),
   +      'language' => $this->t('Language for this field.'),
   +      'property' => $this->t('Name of property being translated.'),
   +      'translation' => $this->t('Translation of either the title or explanation.'));
   

   Array closing parenthesis needs to be on its own line IMHO.

3. +++ b/core/modules/taxonomy/tests/src/Kernel/Plugin/migrate/source/d6/I18nVocabularyTest.php
   @@ -0,0 +1,131 @@
   diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml
   
   diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml
   old mode 100644
   new mode 100755
   diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php
   
   diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php
   old mode 100644
   new mode 100755
   

   I don't think you intended to change modes on these.

Response to code review.

(includes corrections to modes that does not appear in interdiff).

+++ b/core/modules/taxonomy/migration_templates/d6_i18n_taxonomy_vocabulary.yml
@@ -24,0 +25,3 @@
+migration_dependencies:
+  required:
+    - d6_taxonomy_vocabulary

Yay, thanks. Now that this one is added, should we update the migrate form mapping to map from i18n to taxonomy rather than taxonomy to taxonomy? That is where we encode that dependency for the source module as per #2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields.

Thanks for fixing the bad patch. It was still 27 deg at 10pm and I was wilting...

Updating the user form as per #39. Also, removing the changes to d6/MigrateTaxonomyVocabularyTest.php and putting the i18n tests into there own file, config_translation/tests/src/Kernel/Migrate/d6/MigrateI18nTaxonomyVocabularyTest.php. This was done to be consistent with the i18n user profile field instance tests.

+++ b/core/modules/config_translation/tests/src/Kernel/Migrate/d6/MigrateI18nTaxonomyVocabularyTest.php
@@ -0,0 +1,52 @@
+    'config_translation',
+    'locale',

Neither config translation, nor locale would be needed to do this migration. Config translation is just a UI module to edit config translations and locale is just an integration module for community translations / shipped config, neither of which applies here.

Language module provides ConfigurableLanguageManager and LanguageConfigFactoryOverrideInterface which manage config translations. As well as all the APIs used in the test are within language module.

Put the test back into taxonomy module and removed config_translation and locale from the test.

Yay, superb :) Thanks!

Looks like a testbot fluke:

exception: [Uncaught exception] Line 495 of core/includes/install.core.inc:
Drupal\Core\Installer\Exception\AlreadyInstalledException:
To start over, you must empty your existing database and copy default.settings.php over settings.php.
To upgrade an existing installation, proceed to the update script.

Committed and pushed 28afba2 to 8.4.x and 7ac72e0 to 8.3.x. Thanks!

All the stuff below I fixed on commit.

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
@@ -77,7 +77,8 @@ public static function create(ContainerInterface $container, array $configuratio
-      array_keys($container->get('entity.manager')->getBundleInfo($entity_type_id)),
+      array_keys($container->get('entity.manager')
+        ->getBundleInfo($entity_type_id)),

Unrelated change.

FILE: ...taxonomy/src/Plugin/migrate/source/d6/I18nTaxonomyVocabulary.php
----------------------------------------------------------------------
FOUND 4 ERRORS AND 1 WARNING AFFECTING 5 LINES
----------------------------------------------------------------------
  6 | WARNING | [x] Unused use statement
 37 | ERROR   | [x] Short array syntax must be used to define arrays
 38 | ERROR   | [x] Short array syntax must be used to define arrays
 39 | ERROR   | [x] Short array syntax must be used to define arrays
 51 | ERROR   | [x] Short array syntax must be used to define arrays
----------------------------------------------------------------------
PHPCBF CAN FIX THE 5 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 24ms; Memory: 4Mb

PHPCS: core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateI18nTaxonomyVocabularyTest.php passed

FILE: ...tests/src/Kernel/Plugin/migrate/source/d6/I18nVocabularyTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
  97 | ERROR | [x] Expected 1 space before "=>"; 0 found
 109 | ERROR | [x] Expected 1 space before "=>"; 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Some code style fixes to make.

Unfortunately I think this has a regression on Postgres:
https://www.drupal.org/pift-ci-job/616067

Reverted and testing on all environments.

Here's a patch with my fixes from #46

We need to cast the vid to a string in postgreSQL. The only way I can think of doing this in a db agnostic way is to use the like operator because of existing code.

mysql> explain select * from test661879490vocabulary v join test661879490i18n_strings i18n on v.vid = i18n.objectid;
+----+-------------+-------+------------+------+---------------+------+---------+------+------+----------+----------------------------------------------------+
| id | select_type | table | partitions | type | possible_keys | key  | key_len | ref  | rows | filtered | Extra                                              |
+----+-------------+-------+------------+------+---------------+------+---------+------+------+----------+----------------------------------------------------+
|  1 | SIMPLE      | v     | NULL       | ALL  | PRIMARY       | NULL | NULL    | NULL |    5 |   100.00 | NULL                                               |
|  1 | SIMPLE      | i18n  | NULL       | ALL  | NULL          | NULL | NULL    | NULL |  126 |    10.00 | Using where; Using join buffer (Block Nested Loop) |
+----+-------------+-------+------------+------+---------------+------+---------+------+------+----------+----------------------------------------------------+
2 rows in set, 1 warning (0.00 sec)

mysql> explain select * from test661879490vocabulary v join test661879490i18n_strings i18n on v.vid like i18n.objectid;
+----+-------------+-------+------------+------+---------------+------+---------+------+------+----------+----------------------------------------------------+
| id | select_type | table | partitions | type | possible_keys | key  | key_len | ref  | rows | filtered | Extra                                              |
+----+-------------+-------+------------+------+---------------+------+---------+------+------+----------+----------------------------------------------------+
|  1 | SIMPLE      | v     | NULL       | ALL  | NULL          | NULL | NULL    | NULL |    5 |   100.00 | NULL                                               |
|  1 | SIMPLE      | i18n  | NULL       | ALL  | NULL          | NULL | NULL    | NULL |  126 |    11.11 | Using where; Using join buffer (Block Nested Loop) |
+----+-------------+-------+------------+------+---------------+------+---------+------+------+----------+----------------------------------------------------+

There's not much difference because actually there are no keys involved. And there's no key on the objectid on the original table so this is just an inefficient query. The like as expected is just a touch slower but regardless it has to scan the rows. Funky.

The Postgres fix looks good to me.

This also needs renaming. The migrations be in the form basicmigration_variation. The same pattern should be used for the related files like migration plugins and tests. See #26 and #27

Rename complete. I hope the testbot agrees.

Nothing but renames, back to RTBC!

Whilst looking to see if we can use an indexed field I noticed that the i18n_string table actually has a numeric field containing the object ID - we should use that. It'd be great if someone could work out how to join across the table using an index. It's not immediately apparent... I think we might have to join via locales_source.

It'd be great if someone could work out how to join across the table using an index. It's not immediately apparent... I think we might have to join via locales_source.

A bit unclear what you mean here. Join to what table? What do we need to get from locales_source?

Fixed coding standards and restructured VocabularyTranslationTest.

Forgot to upload the interdiff

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

No longer blocked.

Queued a full set of tests, given we've had db-specific issues before...

Tests pass, changes since this was previously committed look good - let's do it!

Hm, I asked for an issue summary update 10 months ago in #14. The issue summary defines the problem as:

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.

I don't know if that is a problem but that problem is definitely not the one this issue is trying to resolve. If the issue summary explains a problem that still needs to be resolved, then it should be copied over to another issue. If not, then it should just be removed. A new issue summary would be important so people arriving at this issue understand what it is/was about, eg. when reviewing changes made to Drupal core.

Re the technical question of (lack of) indexes, I am personally not concerned much for that given that the number of vocabularies should not be too much. Or am I missing something?

+++ b/core/modules/taxonomy/src/Plugin/migrate/source/d6/TaxonomyVocabularyTranslation.php
@@ -0,0 +1,71 @@
+ * Profile field source from database.
...
+  /**
+   * The source table containing profile field info.
+   *
+   * @var string
+   */
+  protected $fieldTable;
+
+  /**
+   * The source table containing the profile values.
+   *
+   * @var string
+   */
+  protected $valueTable;

Guess I didn't look closely enough last time - some copypasta to clean up here.

Issue summary updated.

My feedback in #73 can be addressed by a novice (give the source plugin an appropriate description, and remove the unused member variables).

Re-rolling patch after changing variable description.

@RajeevK, thanks for the patch. Because of that I took another look at my patch and found that those variables can be removed.

The attached patch does that and changes the source plugin description. And finally, renamed the source plugin file from TaxonomyVocabularyTranslation.php to VocabularyTranslation.php to be consistent with the existing d6 vocabulary source plugin name.

Will try to review soon.

Looks good to me!

Looks good now to me too, thanks everyone for the constant effort on this, it was not simple, blocked multiple times, committed and rolled back but it came out great at the end. :)