Problem/Motivation

Translated vocabularies from Drupal 6 need to be migrated to Drupal 8.

Proposed resolution

Add a vocabulary translation migration and associated source plugin, with tests.

Remaining tasks

None.

User interface changes

N/A

API changes

None.

Data model changes

None.

Original report by Ryan Weal

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

Note: this issue deals with taxonomy vocabularies. https://www.drupal.org/node/2784371 deals with taxonomy terms.

Files: 
CommentFileSizeAuthor
#77 interdiff.txt4.44 KBquietone
#77 2225781-77.patch8.9 KBquietone
#76 interdiff-63-76.txt1.69 KBRajeevK
#76 2225781-76.patch9.18 KBRajeevK
#64 interdiff.txt6.07 KBquietone
#63 2225781-2-63.patch9.16 KBquietone
#62 2225781-2-62.patch9.15 KBalexpott
#62 58-62-interdiff.txt1.14 KBalexpott
#59 interdiff.txt15.41 KBquietone
#59 2225781-58.patch9.02 KBquietone
#53 2225781-53.patch8.87 KBalexpott
#53 52-53-interdiff.txt927 bytesalexpott
#52 42-52-interdiff.txt2.93 KBalexpott
#52 2225781-52.patch8.73 KBalexpott
#42 interdiff.txt3.79 KBquietone
#42 2225781-42.patch9.59 KBquietone
#40 interdiff.txt4.33 KBquietone
#40 2225781-40.patch9.69 KBquietone
#38 2225781-38.patch9.7 KBJo Fitzgerald
#38 interdiff-36-38.txt1.1 KBJo Fitzgerald
#36 2225781-36.patch9.86 KBquietone
#27 interdiff.txt6.99 KBquietone
#27 2225781-27.patch15.38 KBquietone
#25 interdiff.txt6.49 KBquietone
#25 2225781-25.patch15.36 KBquietone
#23 2225781-23.patch15.35 KBquietone
#11 2225781-11.patch11.46 KBquietone
#9 2225781-9.patch16.03 KBquietone
#7 2225781-7.patch18.32 KBquietone
#6 2225781-5.patch9.72 KBquietone

Comments

phenaproxima’s picture

Project: IMP » Drupal core
Version: » 8.0.x-dev
Component: Code » migration system
phenaproxima’s picture

svendecabooter’s picture

Status: Postponed » Active

Unblocked

quietone’s picture

quietone’s picture

Issue tags: +migrate-d6-d8
quietone’s picture

Status: Active » Postponed
FileSize
9.72 KB

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.

quietone’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Postponed » Needs review
FileSize
18.32 KB

rerolled for 8.1.x

Status: Needs review » Needs work

The last submitted patch, 7: 2225781-7.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
16.03 KB

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

mikeryan’s picture

Status: Needs review » Needs work

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/

quietone’s picture

Status: Needs work » Postponed
FileSize
11.46 KB

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

mikeryan’s picture

Status: Postponed » Needs review

substr is in!

Status: Needs review » Needs work

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

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-config

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.

viirak’s picture

Is this issue fixed?

mpp’s picture

Title: Migrate D6 i18n taxonomy terms » Migrate D6 i18n taxonomy vocabularies
Issue summary: View changes
Related issues: +#2784371: Migrate D6 i18n taxonomy terms
mpp’s picture

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.

Gábor Hojtsy’s picture

@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? :)

viirak’s picture

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

mpp’s picture

@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));
    }
  }

}
viirak’s picture

@mpp, any patch for this?

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

Status: Needs work » Needs review
FileSize
15.35 KB

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.

Status: Needs review » Needs work

The last submitted patch, 23: 2225781-23.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
15.36 KB
6.49 KB

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

Status: Needs review » Needs work

The last submitted patch, 25: 2225781-25.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
15.38 KB
6.99 KB

This should fix I18nVocabularyTest but not MigrateUpgrade6Test.

Status: Needs review » Needs work

The last submitted patch, 27: 2225781-27.patch, failed testing.

quietone’s picture

Status: Needs work » Postponed

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.

Gábor Hojtsy’s picture

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

quietone’s picture

quietone’s picture

Status: Needs work » Postponed

Oops, it is RTBC not fixed.

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

Status: Needs work » Needs review
FileSize
9.86 KB

Reroll

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
1.1 KB
9.7 KB

Response to code review.

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

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ 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.

quietone’s picture

Status: Needs work » Needs review
FileSize
9.69 KB
4.33 KB

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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ 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.

quietone’s picture

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

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Yay, superb :) Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 42: 2225781-42.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed 28afba2 on 8.4.x
    Issue #2225781 by quietone, Jo Fitzgerald, Gábor Hojtsy, mikeryan:...

  • alexpott committed 7ac72e0 on 8.3.x
    Issue #2225781 by quietone, Jo Fitzgerald, Gábor Hojtsy, mikeryan:...

  • xjm committed a84ac0d on 8.3.x
    Revert "Issue #2225781 by quietone, Jo Fitzgerald, Gábor Hojtsy,...
xjm’s picture

Status: Fixed » Needs work

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

Reverted and testing on all environments.

  • alexpott committed fc2220f on 8.4.x
    Revert "Issue #2225781 by quietone, Jo Fitzgerald, Gábor Hojtsy,...
alexpott’s picture

Status: Needs work » Needs review
FileSize
8.73 KB
2.93 KB

Here's a patch with my fixes from #46

alexpott’s picture

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.

mikeryan’s picture

Assigned: Unassigned » mikeryan
mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

The Postgres fix looks good to me.

mikeryan’s picture

Assigned: mikeryan » Unassigned
quietone’s picture

Status: Reviewed & tested by the community » Needs work

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

quietone’s picture

Assigned: Unassigned » quietone
quietone’s picture

Status: Needs work » Needs review
FileSize
9.02 KB
15.41 KB

Rename complete. I hope the testbot agrees.

quietone’s picture

Assigned: quietone » Unassigned
mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

Nothing but renames, back to RTBC!

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.14 KB
9.15 KB

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.

quietone’s picture

quietone’s picture

FileSize
6.07 KB

Forgot to upload the interdiff

quietone’s picture

Status: Needs review » Postponed

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 » Needs review

No longer blocked.

mikeryan’s picture

Assigned: Unassigned » mikeryan
mikeryan’s picture

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

mikeryan’s picture

Assigned: mikeryan » Unassigned
Status: Needs review » Reviewed & tested by the community

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

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

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?

mikeryan’s picture

Assigned: Unassigned » mikeryan
mikeryan’s picture

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

mikeryan’s picture

Assigned: mikeryan » Unassigned
Issue summary: View changes
Issue tags: -drupal6, -Needs issue summary update

Issue summary updated.

mikeryan’s picture

Issue tags: +Novice

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

RajeevK’s picture

Status: Needs work » Needs review
FileSize
9.18 KB
1.69 KB

Re-rolling patch after changing variable description.

quietone’s picture

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

mikeryan’s picture

Assigned: Unassigned » mikeryan

Will try to review soon.

mikeryan’s picture

Assigned: mikeryan » Unassigned
Status: Needs review » Reviewed & tested by the community

Looks good to me!