Add config translation support to migrations and implement for Drupal 6 user profile fields

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

Unblocked

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.

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?

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?

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.

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.

A migration template and source plugin for D6.

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

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?

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.

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

Since that passed tests, marking this as postponed on #2670170: Add i18n string & variable data to d6_dump

Reroll.

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.

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.

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.

@quietone: are you still working on this?

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.

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?

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

That's all for tonight.

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.

Reworked EntityConfigBase.

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.

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

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.

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.

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/

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

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.

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)   

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

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.

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?

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.

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?

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.

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.

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

Adding rollback, with a test.

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

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.

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

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/

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.

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.

Fixes for items from #54.

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

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.

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

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

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.

Re-test passed, setting back to NR.

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

I'll take a look...

Correct the logic in a comment.

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

No, it passed. Back to RTBC

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

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.

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

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

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

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

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

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.

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

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.

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

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

The implementation requires the source plugin to list a single property at a time using a hardcoded property and translation.

This is confusing from DX point of view, since the data structure for content entity translations does save multiple properties at time. I created #3175477: Migration destination for configuration entity translations is opinionated towards i18n and can only migrate a single property at a time to allow saving multiple properties in a single migration row.