Problem/Motivation

User profile fields in Drupal 6 are translated using the i18nprofile submodule of the i18n suite. We need to migrate these translations.

Proposed resolution

1. Establish a system to migrate configuration translations since this is the first issue that needs it. Drupal\migrate\Plugin\migrate\destination\EntityConfigBase is changed to take the language manager and the config factory in its constructor. Change import() and updateEntity() to support translations. Expand rollback() to support for rolling back config translations.

2. User profile field translations are stored within i18n_strings and locales_target tables connected by the lid key on Drupal 6. These profile values are fields in the user entity on Drupal 8. Migrate the title and description of the fields to Drupal 8.

3. Add tests.

Remaining tasks

Migrate field options translations (if field options themselves are migrated). This is to be handled in #2845975: Migrate Drupal 6 user profile field value option translations.

User interface changes

None.

API changes

Constructor of Drupal\migrate\Plugin\migrate\destination\EntityConfigBase gets two new arguments. As per https://www.drupal.org/core/d8-bc-policy the constructor of a plugin is considered internal and thus does not explicitly count as an API change.

Data model changes

None.

Files: 
CommentFileSizeAuthor
#68 2225717-68.patch27.11 KBJo Fitzgerald
#68 interdiff_63-68.txt937 bytesJo Fitzgerald
#63 interdiff.txt2.78 KBquietone
#63 2225717-63.patch27.1 KBquietone
#57 interdiff.txt1.97 KBquietone
#57 2225717-57.patch27.03 KBquietone
#56 Selection_006.png17 KBquietone
#50 interdiff.txt12.42 KBquietone
#50 2225717-50.patch27.13 KBquietone
#48 interdiff.txt18.48 KBquietone
#48 2225717-48.patch27.05 KBquietone
#35 interdiff.txt16.93 KBquietone
#35 2225717-35.patch16.57 KBquietone
#32 interdiff.txt4.66 KBquietone
#32 2225717-32.patch15.72 KBquietone
#31 interdiff.txt836 bytesquietone
#31 2225717-31.patch15.69 KBquietone
#29 interdiff.txt1.61 KBquietone
#29 2225717-29.patch15.61 KBquietone
#26 interdiff-2225717-24-26.txt2.42 KBquietone
#26 2225717-26.patch15.65 KBquietone
#24 interdiff-2225717-21-24.txt4.58 KBquietone
#24 2225717-24.patch15.64 KBquietone
#21 2225717-21.patch16 KBquietone
#19 interdiff-2225717-16-19.txt10.88 KBquietone
#19 2225717-19.patch16.25 KBquietone
#17 2225717-16-test-only.patch866.53 KBquietone
#16 2225717-16.patch16.55 KBquietone
#15 2225717-15.patch16.73 KBquietone
#13 2225717-13.patch5.95 KBquietone
#12 2225717-12.patch4.79 KBquietone
#9 Translate interface | Drupal default 2016-02-03 18-45-40.png212.38 KBGábor Hojtsy

Comments

phenaproxima’s picture

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

svendecabooter’s picture

Unblocked

svendecabooter’s picture

Status: Postponed » Active
quietone’s picture

So, how does this work in D6? I've enabled the modules, installed languages and user profiles don't translate. Not any better on D8, there I get WSOD. Nor can I find instructions, so I guess this should be straight forward.

penyaskito’s picture

Are D6 profiles fieldable? We may need to migrate those fields and the fields provided by the module first to the user entity. Then we can migrate the data from there using the user as destination.
Does this make sense?

quietone’s picture

Yes and no. I'm not even thinking about that yet because my starting point is to create D6 profile data with translations. I've tried without success. How does one make a profile data that is translated? What are the steps?

Gábor Hojtsy’s picture

Title: Migrate D6 i18n profiles » Migrate D6 i18n user profiles
Issue tags: +D8MI, +language-content, +sprint
Gábor Hojtsy’s picture

When enabling the profile translation in i18n, it says "You must enable the Profile, String translation, Locale modules to install Profile translation.". I went ahed with those. Added a new "Home page" text profile field at admin/user/profile (in the Home page category for simplicity). Now I get that category as a tab at user/1/edit/Homepage and I get the category title and the field title at admin/build/translate/search when filtering to profile fields:

(So its not just the fields but also the categories that would need to be supported).

Note that its not the actual field values that are translated, the field title, explanation, options (in case of select options) and the category labels are. Not the values. Similar to field translation in i18n where it translates the field settings, not the field values. Hope this helps.

Gábor Hojtsy’s picture

Title: Migrate D6 i18n user profiles » Migrate D6 i18n user profile field/category configuration
quietone’s picture

Not the values.

Ah, now it makes sense.

And I somehow missed (several times) the profile option at the bottom of admin/build/translate/search.

quietone’s picture

Status: Active » Needs work
FileSize
4.79 KB

A migration template and source plugin for D6.

quietone’s picture

Removed some copy/paste errors and added source plugin test.

quietone’s picture

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

FileSize
16.73 KB

Another iteration. Added static map to the template, adjusted the destination plugins to suit and added tests for all the user profile fields. Note this requires the data from #2670170: Add i18n string & variable data to d6_dump.

While the changes to the destination plugins work, I'm not sure if this is the correct/best way to accomplish this migration. And the other i18n_string migrations haven't been written yet, so these changes may not work for those.

Also, should this migration be in the config_translation module or somewhere else?

quietone’s picture

FileSize
16.55 KB

Running non translation migrations with the above patch fail. Tracked this down to the fact that container->get(language_manager) returns a non configurable language interface when config_translation is not installed. So, in this patch EntityConfigBase checks the instance of the language manager service, so that, if it is configurable and the row has a langcode property, then the row is imported as translation data.

quietone’s picture

Status: Needs work » Needs review
FileSize
866.53 KB

Add the testing data #2670170: Add i18n string & variable data to d6_dump so the testbot can check this.

quietone’s picture

Status: Needs review » Postponed

Since that passed tests, marking this as postponed on #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
16.25 KB
10.88 KB

Reroll.

Kristen Pol’s picture

Status: Needs review » Needs work

Thanks for the patch. Here are some minor nitpicks:

  1. +++ b/core/modules/config_translation/tests/src/Kernel/Migrate/d6/Migratei18nUserProfileFieldInstanceTest.php
    @@ -0,0 +1,68 @@
    +class Migratei18nUserProfileFieldInstanceTest extends MigrateDrupal6TestBase {
    

    Should this be MigrateI18NUserProfileFieldInstanceTest? (capitalize "I18N") or i18nMigrateUserProfileFieldInstanceTest?

  2. +++ b/core/modules/config_translation/tests/src/Kernel/Migrate/d6/Migratei18nUserProfileFieldInstanceTest.php
    @@ -0,0 +1,68 @@
    +  public function testUserProfileFields() {
    

    It's a bit odd that some of the values are actually in French while others are like "fr - some English text". Is there some reason for this? It would be good to use one or the other. My preference is to use the latter (English text) since I don't understand French. :)

  3. +++ b/core/modules/config_translation/tests/src/Kernel/Migrate/d6/Migratei18nUserProfileFieldInstanceTest.php
    @@ -0,0 +1,68 @@
    +    $config_translation = \Drupal::service('language_manager')->getLanguageConfigOverride('fr', 'field.field.user.user.profile_love_migrations');
    

    Add comment?

  4. +++ b/core/modules/config_translation/tests/src/Kernel/Migrate/d6/Migratei18nUserProfileFieldInstanceTest.php
    @@ -0,0 +1,68 @@
    +    $config_translation = \Drupal::service('language_manager')->getLanguageConfigOverride('fr', 'field.field.user.user.profile_color');
    

    Add comment?

  5. +++ b/core/modules/config_translation/tests/src/Kernel/Migrate/d6/Migratei18nUserProfileFieldInstanceTest.php
    @@ -0,0 +1,68 @@
    +    $config_translation = \Drupal::service('language_manager')->getLanguageConfigOverride('fr', 'field.field.user.user.profile_biography');
    

    Add comment?

  6. +++ b/core/modules/config_translation/tests/src/Kernel/Migrate/d6/Migratei18nUserProfileFieldInstanceTest.php
    @@ -0,0 +1,68 @@
    +    $config_translation = \Drupal::service('language_manager')->getLanguageConfigOverride('fr', 'field.field.user.user.profile_sell_address');
    

    Add comment?

  7. +++ b/core/modules/config_translation/tests/src/Kernel/Migrate/d6/Migratei18nUserProfileFieldInstanceTest.php
    @@ -0,0 +1,68 @@
    +    $config_translation = \Drupal::service('language_manager')->getLanguageConfigOverride('fr', 'field.field.user.user.profile_sold_to');
    

    Add comment?

  8. +++ b/core/modules/config_translation/tests/src/Kernel/Migrate/d6/Migratei18nUserProfileFieldInstanceTest.php
    @@ -0,0 +1,68 @@
    +    $config_translation = \Drupal::service('language_manager')->getLanguageConfigOverride('fr', 'field.field.user.user.profile_bands');
    

    Add comment?

  9. +++ b/core/modules/config_translation/tests/src/Kernel/Migrate/d6/Migratei18nUserProfileFieldInstanceTest.php
    @@ -0,0 +1,68 @@
    +    $config_translation = \Drupal::service('language_manager')->getLanguageConfigOverride('fr', 'field.field.user.user.profile_birthdate');
    

    Add comment?

  10. +++ b/core/modules/config_translation/tests/src/Kernel/Migrate/d6/Migratei18nUserProfileFieldInstanceTest.php
    @@ -0,0 +1,68 @@
    +    $config_translation = \Drupal::service('language_manager')->getLanguageConfigOverride('fr', 'field.field.user.user.profile_blog');
    

    Add comment?

  11. +++ b/core/modules/config_translation/tests/src/Unit/Plugin/migrate/source/d6/i18nProfileFieldTest.php
    @@ -0,0 +1,88 @@
    + * Tests something
    

    Needs better comment using a full sentence.

quietone’s picture

Status: Needs work » Needs review
FileSize
16 KB

I thought I responded to this weeks ago, seems I didn't.

1) The Naming conventions states

If an acronym is used in a class or method name, make it CamelCase too (SampleXmlClass, not SampleXMLClass). [Note: this standard was adopted in March 2013, reversing the previous standard.]

So it seems the name should be MigrateI18nUserProfileFieldInstanceTest and I18nProfileFieldTest.php.

2) The reason there is a mix of French and 'fr - english text' is because I had a French speaking friend visiting while I was making these changes. It seems appropriate to have the correct French when available. In this case, there is no need to understand the language as we are simply migrating text.

3 - 10) Not sure what you are asking.

11) Fixed.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Thanks for all your work on these issues, reviewed this one:

  1. --- /dev/null
    +++ b/core/modules/config_translation/migration_templates/i18n_user_profile_field_instance.yml
    
    +++ b/core/modules/config_translation/migration_templates/i18n_user_profile_field_instance.yml
    @@ -0,0 +1,28 @@
    
    @@ -0,0 +1,28 @@
    +id: i18n_user_profile_field_instance
    

    Needs to be redone to not use migration templates given the move away from them.

    Also the other ones have d6 in their name, should this not? Hm, based on the code this does seem like both Drupal 6 and 7? Why not have both tags on it then?

  2. +++ b/core/modules/config_translation/src/Plugin/migrate/source/d6/I18nProfileField.php
    @@ -0,0 +1,79 @@
    +    if (empty($this->fieldTable) || empty($this->valueTable)) {
    +      if ($this->getModuleSchemaVersion('system') >= 7000) {
    +        $this->fieldTable = 'profile_field';
    +      }
    +      else {
    +        $this->fieldTable = 'profile_fields';
    +      }
    +    }
    

    ... Or given this is called d6_i18n..., should it not try to deal with Drupal 7? :)

  3. +++ b/core/modules/config_translation/src/Plugin/migrate/source/d6/I18nProfileField.php
    @@ -0,0 +1,79 @@
    +    $query = $this->select($this->fieldTable, 'pf')
    +      ->fields('pf', array('fid', 'name'))
    +      ->fields('i18n', array('property'))
    +      ->fields('lt', array('lid', 'translation', 'language'));
    +    $query->leftJoin('i18n_strings', 'i18n', 'i18n.objectid = pf.name');
    +    $query->leftJoin('locales_target', 'lt', 'lt.lid = i18n.lid');
    +    return $query;
    

    Should filter on i18n object type too no? Otherwise if some other object had the same name but a different object type, it would be included here.

  4. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
    @@ -70,10 +121,18 @@ public function getIds() {
    +    if ($row->hasDestinationProperty('langcode') && ($this->languageManager instanceof ConfigurableLanguageManagerInterface)) {
    +      // Then this is a row with a translation string.
    

    The original config may also have a langcode theoretically. So it would be better to check if this langcode is different from the original langcode of the entity.

Gábor Hojtsy’s picture

@quietone: are you still working on this?

quietone’s picture

Status: Needs work » Needs review
FileSize
15.64 KB
4.58 KB

1. Currently, all the migrations in core are in ./core/modules/*/migration_templates. I'm not sure this is the place to make a precedent. Maybe that needs an issue?

For now, this should be tagged for Drupal 6 only, because that is the priority. To add D7 the test data needs to be added to the dumps. There is already an issue for that, #2670846: Add i18n data to d7_dump.

2. Fixed.

3. True. But since this also has a join on locales.target.lid = i18nstrings.lid, where lid is a primary key, this shouldn't matter.

4. Added a test for the default language.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
    @@ -121,8 +121,12 @@
    -    if ($row->hasDestinationProperty('langcode') && ($this->languageManager instanceof ConfigurableLanguageManagerInterface)) {
    -      // Then this is a row with a translation string.
    +    // If this row has a language code and it is not the default language
    +    // and the language manager is configurable then this is a row with a
    +    // translation string.
    +    if ($row->hasDestinationProperty('langcode') &&
    +      ($row->getDestinationProperty('langcode') != $this->languageManager->getDefaultLanguage()->getId()) &&
    +      ($this->languageManager instanceof ConfigurableLanguageManagerInterface)) {
    

    This is only true if we make assumptions about the source data. In Drupal 6, the source data is indeed assumed to be in the site default language and all others are translations. Since this is a destination plugin, I am not sure Drupal 6 assumptions should be baked in here.

    Configuration in Drupal 8 may be created in whatever language and then translated to whatever other language. So what should be checked is the active config matches the language of this data or not. If it does not, then this is a translation (regardless of site default language).

  2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
    --- /dev/null
    +++ b/core/modules/config_translation/migration_templates/d6_i18n_user_profile_field_instance.yml
    
    +++ b/core/modules/config_translation/migration_templates/d6_i18n_user_profile_field_instance.yml
    +++ b/core/modules/config_translation/migration_templates/d6_i18n_user_profile_field_instance.yml
    @@ -0,0 +1,28 @@
    
    @@ -0,0 +1,28 @@
    +id: i18n_user_profile_field_instance
    

    The id should also start with d6, not?

quietone’s picture

Status: Needs work » Needs review
FileSize
15.65 KB
2.42 KB

1. Changed to getCurrentLanguage. Is that what you meant?
2. Yes, I missed that.

That's all for tonight.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

No, we need to check against

$config = $entity->getConfigDependencyName();
$langcode = \Drupal::config($config)->get('langcode');

And compare the langcode in the migrated data to that langcode there in the config.

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.61 KB
1.61 KB

Reworked EntityConfigBase.

Status: Needs review » Needs work

The last submitted patch, 29: 2225717-29.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
15.69 KB
836 bytes

There was trouble with my test setup so I couldn't run migrate tests before. The check on the language manager in EntityConfigBase is restored. And now that I fixed my setup, the tests run locally.

quietone’s picture

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

phenaproxima’s picture

Assigned: Unassigned » phenaproxima
phenaproxima’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/config_translation/migration_templates/d6_i18n_user_profile_field_instance.yml
    @@ -0,0 +1,28 @@
    +  plugin: entity:field_config
    

    Should be plugin: 'entity:field_config', since colons must be quoted in YAML.

  2. +++ b/core/modules/config_translation/src/Plugin/migrate/source/d6/I18nProfileField.php
    @@ -0,0 +1,51 @@
    + *   source_provider = "profile"
    

    Really? Core profile didn't provide any i18n support, to my recollection. Shouldn't this maybe be one of the i18n suite of modules?

  3. +++ b/core/modules/config_translation/src/Plugin/migrate/source/d6/I18nProfileField.php
    @@ -0,0 +1,51 @@
    +      ->fields('pf', array('fid', 'name'))
    +      ->fields('i18n', array('property'))
    +      ->fields('lt', array('lid', 'translation', 'language'));
    

    Nit: For readability, I'd prefer [] syntax instead of array() here.

  4. +++ b/core/modules/config_translation/src/Plugin/migrate/source/d6/I18nProfileField.php
    @@ -0,0 +1,51 @@
    +    $ids['translation']['type'] = 'string';
    

    Should the full translation really be a source identifier? That seems kind of weird to me.

  5. +++ b/core/modules/config_translation/tests/src/Kernel/Migrate/d6/MigrateI18nUserProfileFieldInstanceTest.php
    @@ -0,0 +1,62 @@
    +  public function testUserProfileFields() {
    

    Every instance of assertIdentical() in this function should be assertSame(). Can it also be broken up into "paragraphs" (maybe every time the language_manager service is called) for readability?

  6. +++ b/core/modules/config_translation/tests/src/Kernel/Migrate/d6/MigrateI18nUserProfileFieldInstanceTest.php
    @@ -0,0 +1,62 @@
    +    $config_translation = \Drupal::service('language_manager')->getLanguageConfigOverride('fr', 'field.field.user.user.profile_love_migrations');
    

    Use $this->container->get(), not \Drupal::service().

  7. +++ b/core/modules/config_translation/tests/src/Plugin/migrate/source/d6/I18nProfileFieldTest.php
    @@ -0,0 +1,82 @@
    +  protected $databaseContents = [
    

    Please move $databaseContents and $expectedResults into providerSource() to stay consistent with the other tests.

  8. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
    @@ -21,6 +26,52 @@
    +   * @param MigrationInterface $migration
    

    This and all other parameters should use the fully qualified class name.

  9. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
    @@ -70,10 +121,27 @@ public function getIds() {
    +    if ($row->hasDestinationProperty('langcode') && $this->languageManager instanceof ConfigurableLanguageManagerInterface)  {
    

    All of this definitely needs test coverage.

  10. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
    @@ -70,10 +121,27 @@ public function getIds() {
    +      $langcode = \Drupal::config($config)->get('langcode');
    

    There doesn't appear to be any reason why the config factory service cannot be injected into the constructor and used here.

  11. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/destination/EntityFieldStorageConfig.php
    @@ -39,11 +40,14 @@ class EntityFieldStorageConfig extends BaseEntityFieldStorageConfig {
    +   * @param LanguageManagerInterface $language_manager
    

    Should be using the FQCN.

quietone’s picture

Status: Needs work » Needs review
FileSize
16.57 KB
16.93 KB

Thanks for the review.

1. Except that we don't escape any of the other destination plugin strings.
2. Yes, that is wrong. Changed to 'i18n' but this also depends on locales_target.
3. Fixed.
4. Ouch, fixed.
5. Fixed.
6. Fixed, with a twist. The language manager is now a variable. I think it is easier to read the test.
7. Fixed.
8. Fixed.
9. Still needs tests. Not sure where to start on it.
10. Fixed.
11. Fixed.

quietone’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Just noticed this which can be added later with the test.

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
@@ -70,10 +133,27 @@ public function getIds() {
+    // This is a translation if the language in the active config matches the

Should be s/match/does not match/

Gábor Hojtsy’s picture

  1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
    @@ -21,6 +27,63 @@
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, EntityStorageInterface $storage, array $bundles, LanguageManagerInterface $language_manager, ConfigFactoryInterface $config_factory) {
    +    parent::__construct($configuration, $plugin_id, $plugin_definition, $migration, $storage, $bundles);
    
    +++ b/core/modules/migrate_drupal/src/Plugin/migrate/destination/EntityFieldStorageConfig.php
    @@ -39,11 +41,17 @@ class EntityFieldStorageConfig extends BaseEntityFieldStorageConfig {
    -  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, EntityStorageInterface $storage, array $bundles, FieldTypePluginManagerInterface $field_type_plugin_manager) {
    -    parent::__construct($configuration, $plugin_id, $plugin_definition, $migration, $storage, $bundles);
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, EntityStorageInterface $storage, array $bundles, LanguageManagerInterface $language_manager, ConfigFactoryInterface $config_factory,FieldTypePluginManagerInterface $field_type_plugin_manager) {
    +    parent::__construct($configuration, $plugin_id, $plugin_definition, $migration, $storage, $bundles, $language_manager,  $config_factory, $field_type_plugin_manager);
    

    I asked about these changes on #2810347-32: [policy, no patch] Mark migrate.module as beta stability to make sure we are allowed to do this in some form, and which form :)

  2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
    @@ -70,10 +133,27 @@ public function getIds() {
    +      $config = $entity->getConfigDependencyName();
    +      $langcode = $this->configFactory->get('langcode');
    +      if ($langcode != $row->getDestinationProperty('langcode')) {
    +        $translation = TRUE;
    +      }
    

    Not sure what is this doing :) We take $config but then don't use it and attempt to load a langcode top level config file (which does not exist). I proposed this logic in #27:

    $config = $entity->getConfigDependencyName();
    $langcode = \Drupal::config($config)->get('langcode');
    

    That would in fact take the langcode value of the config from the active config storage :)

    I think the tricky part is that config may not exist also. So if the langcode of that does not match the langcode of the row, that does not yet mean we need to save a translation, only if the original config existed.

    How do we ensure to only process config translations for config that exists already? (That would be the only scenario where this logic would work, otherwise I don't know if we could tell in this class if we are dealing with a translation or original config really).

quietone’s picture

Did some testing and found that rollback does not work as expected for the i18n migrations. The rollback will delete the config entity not the translation of the config entity.

quietone’s picture

How do we ensure to only process config translations for config that exists already?

This i18n migration has dependencies on the user_profile_field and user_profile_field_instance so the config *should* exist.

migration_dependencies:
  required:
    - user_profile_field
    - user_profile_field_instance

If it does not a FieldException is thrown.

v@dev2 {/opt/sites/d8} (tmp2)$ drush mi upgrade_d6_i18n_user_profile_field_instance
Attempt to create a field profile_color that does not exist on entity type user. (/opt/sites/d8/core/modules/field/src/Entity/FieldConfig.php:286)   
Gábor Hojtsy’s picture

@quietone:

Did some testing and found that rollback does not work as expected for the i18n migrations. The rollback will delete the config entity not the translation of the config entity.

Well, it goes in the field config entity destination, so if that is rolled back, then the original ones would be? Should it use its own ids or a different destination to make rollback possible?

This i18n migration has dependencies on the user_profile_field and user_profile_field_instance so the config *should* exist.

Superb, thanks! That also means that the config plugin in itself does not make sure this happens but it offloads that requirement to the migration itself. I guess that may be the case for other things as well, so not arguing the plugin should do something about it then :)

heddn’s picture

Issue tags: +Migrate BC break

Adding a tag. I did a quick review of the latest patch and it does seem to effect BC; and in a way that is fairly intrusive.

mikeryan’s picture

Issue tags: +Needs change record

BC break would definitely need a change record - that might help demonstrate how intrusive this is. Who in the contrib/custom space would be affected by this?

quietone’s picture

Should it use its own ids or a different destination to make rollback possible?

Similar to EntityContentBase, adding a language code to the destination is mostly working. The mostly is because the rollback count is incorrect. The rollback is deleting the translated field instance and not just the translated string from the field. But there is no information on the destination to indicate which string was translated. I guess that will have to be added.

Gábor Hojtsy’s picture

By "which string was translated" do you mean which field had a translation saved? I don't think we need to know the string specifically, its enough to know that a config translation was created. Or do you want to manage rollback on a per string basis, so specific strings are removed from the config translation rather than that translation of that field removed?

quietone’s picture

These string translation migrations do one string per migration. For example. the user profile fields have a label and description field, and those are migrated one at a time, resulting in 2 rows in the map table. The rollback now rolls back one translation at a time, either the label or the description, but not both. I have a working version, though it is very specific to this migration. I want to explore other migrations that use the destination entity:field_config to learn more.

Or, change the source plugin to get all translation strings at once.

Gábor Hojtsy’s picture

I think logically the migration looks at the source data as a unit, but I looked at the destination data as a unit :) Approaching from the source data makes more sense because there is no guesswork as to which items have translations, we don't need to check each of them one by one. Since a label may not have a corresponding description and a description does not necessarily have a corresponding label (although less likely), starting from the source data and coupling them up may be as hard...

So I think the per string approach may be fine and consistent with the rest of the system.

mikeryan’s picture

Priority: Normal » Major
Issue tags: +Migrate critical

Since this blocks beta stability for the Migrate API, setting Migrate critical.

quietone’s picture

Status: Needs work » Needs review
FileSize
27.05 KB
18.48 KB

Adding rollback, with a test.

Status: Needs review » Needs work

The last submitted patch, 48: 2225717-48.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
27.13 KB
12.42 KB

Renamed the test file and implode the destination_identifier in EntityConfgBase::rollback.

mikeryan’s picture

Issue tags: -Needs tests

Looks like we've got our tests - still needs a change record, though.

I haven't given a full review, but just want to clarify what BC break(s) we have here - if I'm not mistaken, the only use case that would be affected would be extending EntityConfigBase or a class extending it and overriding __construct() and create(), correct? Nothing in core does this, and it's hard to conceive of a real-world use case, so as a practical matter I think the actual impact will likely be nil.

Gábor Hojtsy’s picture

@mikeryan: my understanding was that rollback is still a problem with this approach(?) and that is the biggest outstanding problem.

Re whether we should consider this a BC break, I pointed out in #2225717-37: Add config translation support to migrations and implement for Drupal 6 user profile fields and asked in #2810347-32: [policy, no patch] Mark migrate.module as beta stability because it is not entirely clear-cut. I am not working on extending or providing services on top of migrate, so I don't know how severe/practically applicable this is. If we believe it is not, then we can remove the tag here and re-RTBC #2810347: [policy, no patch] Mark migrate.module as beta stability I guess.

mikeryan’s picture

Assigned: phenaproxima » mikeryan
mikeryan’s picture

Assigned: mikeryan » Unassigned
Status: Needs review » Needs work
Issue tags: -Migrate critical

Just some minor points, then RTBC as far as I'm concerned.

Since the BC impact is minimal, removing migrate-critical tag.

  1. +++ b/core/modules/config_translation/tests/src/Kernel/Migrate/d6/MigrateI18nUserProfileFieldInstanceTest.php
    @@ -0,0 +1,71 @@
    +  protected function setUp() {
    +    parent::setUp();
    +  }
    

    Looks like we can drop this.

  2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
    @@ -39,13 +102,26 @@ public function import(Row $row, array $old_destination_id_values = array()) {
    +          if (($id_key == 'langcode')) {
    

    Condition seems wrapped extra snugly.

  3. +++ b/core/modules/migrate/tests/src/Kernel/MigrateRollbackEntityConfigTest.php
    @@ -0,0 +1,171 @@
    +    // Conform the Original vocabulary still exists.
    

    s/Conform/confirm/
    s/Original/original/

Gábor Hojtsy’s picture

Re #52 we discussed on the migrate meeting that the rollback problem that was discussed earlier is now resolved in the patch. Will review again with multilingual features in mind.

quietone’s picture

FileSize
17 KB

Tested this by migrating the d6_dump to a D8 site that was installed in Spanish, a language not used in the d6_dump. Then I checked the translations of the user account settings. The english text has been stored as the original Spanish text instead of the English text. Hopefully, something simple like requiring the default_language migration.

quietone’s picture

Status: Needs work » Needs review
FileSize
27.03 KB
1.97 KB

Fixes for items from #54.

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

Patch itself is RTBC - should still get a change record here, though.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

I promised to review this but did not get around to it earlier, sorry. Found two hopefully easy things to fix.

  1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
    @@ -39,13 +102,26 @@ public function import(Row $row, array $old_destination_id_values = array()) {
    +        if ($this->isTranslationDestination()) {
    +          if ($id_key == 'langcode') {
    +            $return[] = $row->getDestinationProperty($id_key);
    +          }
    +          else {
    +            $return[] = $entity->get($id_key);
    +          }
    +        }
    +        else {
    +          $return[] = $entity->get($id_key);
    +        }
    

    Should this just be simpler as

    if ($this->isTranslationDestination() && $id_key == 'langcode') {
      $return[] = $row->getDestinationProperty($id_key);
    }
    else {
    ...
    }
    
    

    That would make this much readable. Also a little note as to what is this being done would be useful.

  2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
    @@ -70,11 +159,28 @@ public function getIds() {
    +    // This is a translation if the language in the active config matches the
    +    // language of this row.
    +    $translation = FALSE;
    +    if ($row->hasDestinationProperty('langcode') && $this->languageManager instanceof ConfigurableLanguageManager)  {
    +      $config = $entity->getConfigDependencyName();
    +      $langcode = $this->configFactory->get('langcode');
    +      if ($langcode != $row->getDestinationProperty('langcode')) {
    +        $translation = TRUE;
    +      }
    

    The logic is opposite of the two lines of comments? It is a translation if the two languages DONT match not if they match.

Gábor Hojtsy’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Updated issue summary significantly. Also added a change notice draft at https://www.drupal.org/node/2834360

mikeryan’s picture

Issue tags: -Needs change record

Change notice looks good to me. We should be back at RTBC with Gabor's comments applied.

quietone’s picture

Status: Needs work » Needs review
FileSize
27.1 KB
2.78 KB

Gábor Hojtsy, thanks for the review.

1. Oh yea, I meant to tidy that up. And changed a few related lines to use short array syntax.
2. Comment fixed.

Also, minor code standard fixes.

Status: Needs review » Needs work

The last submitted patch, 63: 2225717-63.patch, failed testing.

Jo Fitzgerald’s picture

Status: Needs work » Needs review

Re-test passed, setting back to NR.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

I don't see the fix for #59/2 in the interdiff. Otherwise changes look good.

Jo Fitzgerald’s picture

Assigned: Unassigned » Jo Fitzgerald

I'll take a look...

Jo Fitzgerald’s picture

Assigned: Jo Fitzgerald » Unassigned
Status: Needs work » Needs review
FileSize
937 bytes
27.11 KB

Correct the logic in a comment.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, that resolves the concerns I had. Since otherwise Mike marked it RTBC and approved the change notice I wrote, back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 68: 2225717-68.patch, failed testing.

quietone’s picture

Status: Needs work » Reviewed & tested by the community

No, it passed. Back to RTBC

Gábor Hojtsy’s picture

Issue tags: +blocker

This blocks all multilingual configuration migrations given its support code added for config translation migrations.

Gábor Hojtsy’s picture

Title: Migrate D6 i18n user profile field/category configuration » Add config translation support to migrations and implement for Drupal 6 user profile fields
Issue summary: View changes

Update title to be more honest about what is going on here :) Also opened the followup mentioned in the summary at #2845975: Migrate Drupal 6 user profile field value option translations and linked it in from there.

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

Is this just blocked by the number of hours in a day? :)

DamienMcKenna’s picture

I cross-linked the issues that this one was blocking, so we'd be able to find them more easily.

Gábor Hojtsy’s picture

@DamienMcKenna: yes, unfortunately Drupal is not quite advanced yet to expand the hours in a day.

xjm’s picture

@DamienMcKenna, it is not helpful to post comments on RTBC issues asking why they have not been committed. All RTBC issues will receive a response from committers as soon as they have the availability.

Edit: also, such comments are actually disruptive to getting something committed, since they make an RTBC newer than it is. It's unfortunate, but avoiding unnecessary comments on RTBCs will actually help committers get to them sooner.

xjm’s picture

Issue tags: +rc eligible

@catch, @alexpott, @cilefen, and I discussed this issue just now and agreed that it would be great to get this into 8.3.x still, and that we would be okay committing it to 8.3.x between the beta and RC for sure (unlike most things) since Migrate API itself is also beta.

Tagging rc deadline for now; we debated whether it would be okay during a patch release for a beta experimental module and saw cases for both. We don't really have a precedent yet.

But this is still definitely committable to 8.3.x until February 28.

Thanks everyone for your interest and contributions on this issue!

catch’s picture

I'd forgotten that we migrate 6/7 profile fields to the user entity. This doesn't seem right to me so I've opened #2851289: Revisit if/how to migrate profile module data to 8.x to discuss it again.

Doesn't affect this issue as such, I realise it unblocks other configuration translation migrations.

xjm’s picture

Issue tags: -rc eligible +rc deadline

(Screwed up the tag in a kind of important way; fixing.)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +8.3.0 release notes

Given we have the followup to discuss @catch's generic concern re profile fields I think we can proceed with this issue. The changes to migrating config entities where translations are involved all make sense and it is great to see good coverage of the rollback scenario. Nice work everyone.

Committed and pushed 6fc8b98 to 8.4.x and e9f78c5 to 8.3.x. Thanks!

diff --git a/core/modules/migrate/tests/src/Kernel/MigrateRollbackEntityConfigTest.php b/core/modules/migrate/tests/src/Kernel/MigrateRollbackEntityConfigTest.php
index b8e4811..76945a7 100644
--- a/core/modules/migrate/tests/src/Kernel/MigrateRollbackEntityConfigTest.php
+++ b/core/modules/migrate/tests/src/Kernel/MigrateRollbackEntityConfigTest.php
@@ -3,9 +3,6 @@
 namespace Drupal\Tests\migrate\Kernel;
 
 use Drupal\migrate\MigrateExecutable;
-use Drupal\migrate\Plugin\MigrateIdMapInterface;
-use Drupal\migrate\Row;
-use Drupal\taxonomy\Entity\Term;
 use Drupal\taxonomy\Entity\Vocabulary;
 
 /**
@@ -71,7 +68,7 @@ public function testConfigEntityRollback() {
     $vocabulary_executable = new MigrateExecutable($vocabulary_migration, $this);
     $vocabulary_executable->import();
     foreach ($vocabulary_data_rows as $row) {
-      /** @var Vocabulary $vocabulary */
+      /** @var \Drupal\taxonomy\Entity\Vocabulary $vocabulary */
       $vocabulary = Vocabulary::load($row['id']);
       $this->assertTrue($vocabulary);
       $map_row = $vocabulary_id_map->getRowBySource(['id' => $row['id']]);
@@ -159,7 +156,7 @@ public function testConfigEntityRollback() {
 
     // Confirm the original vocabulary still exists.
     foreach ($vocabulary_data_rows as $row) {
-      /** @var Vocabulary $vocabulary */
+      /** @var \Drupal\taxonomy\Entity\Vocabulary $vocabulary */
       $vocabulary = Vocabulary::load($row['id']);
       $this->assertTrue($vocabulary);
       $map_row = $vocabulary_id_map->getRowBySource(['id' => $row['id']]);
diff --git a/core/modules/migrate_drupal/src/Plugin/migrate/destination/EntityFieldStorageConfig.php b/core/modules/migrate_drupal/src/Plugin/migrate/destination/EntityFieldStorageConfig.php
index a92a771..43df170 100644
--- a/core/modules/migrate_drupal/src/Plugin/migrate/destination/EntityFieldStorageConfig.php
+++ b/core/modules/migrate_drupal/src/Plugin/migrate/destination/EntityFieldStorageConfig.php
@@ -54,8 +54,8 @@ class EntityFieldStorageConfig extends BaseEntityFieldStorageConfig {
    * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
    *   The configuration factory.
    */
-  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, EntityStorageInterface $storage, array $bundles, LanguageManagerInterface $language_manager, ConfigFactoryInterface $config_factory,FieldTypePluginManagerInterface $field_type_plugin_manager) {
-    parent::__construct($configuration, $plugin_id, $plugin_definition, $migration, $storage, $bundles, $language_manager,  $config_factory, $field_type_plugin_manager);
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, EntityStorageInterface $storage, array $bundles, LanguageManagerInterface $language_manager, ConfigFactoryInterface $config_factory, FieldTypePluginManagerInterface $field_type_plugin_manager) {
+    parent::__construct($configuration, $plugin_id, $plugin_definition, $migration, $storage, $bundles, $language_manager, $config_factory, $field_type_plugin_manager);
     $this->languageManager = $language_manager;
     $this->configFactory = $config_factory;
     $this->fieldTypePluginManager = $field_type_plugin_manager;

Coding standards fixed on commit.

  • alexpott committed 6fc8b98 on 8.4.x
    Issue #2225717 by quietone, Jo Fitzgerald, Gábor Hojtsy, mikeryan,...

  • alexpott committed e9f78c5 on 8.3.x
    Issue #2225717 by quietone, Jo Fitzgerald, Gábor Hojtsy, mikeryan,...
DamienMcKenna’s picture

@xjm, @gabor, others: I am sincerely sorry for my impatience in #75 :-(

Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks everyone, especially @quietone! This unblocks so many other issues also :)

Status: Fixed » Closed (fixed)

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