Migrate Drupal 7 Entity Translation settings to Drupal 8

Yes, the plan is to provide an upgrade path to D8, which will likely be the only functionality implemented in the 8.x-2.x branch. Both translation models will be supported.

This won't happen until we are done with the D8 entity storage at least.

...better title for our dashboards ;)

Parts of this might be addressed in #1952044: Migration path for legacy content translation module data.

One question: In entity translation D7 I think all languages are saved in every revision, while in D8 "we write to the storage only field values that actually changed": #2562759-2: Nodes revision cannot be saved per translation.
So will the upgrade path be able to pull out the relevant changed translation for each revision in D7 or will just all revisions for all languages be imported into D8?

That means that only changed values are re-written, but they are always stored at least once per revision.

Has there been any progress on this issue? Is there going to be an upgrade path in the 8.x-2.x branch?

I'm using entity translation with field translation, i.e. language_content_type_{type} == 4.

#2225271: Migrate content type language settings from Drupal 6 & 7 seems to add support only for the legacy translation settings and not for Entity Translation with "field translation".
#2669964: Migrate Drupal 7 core node translations to Drupal 8 also seems to deal only with the legacy translation.

Any pointers as how to approach this in a new migration project?

Should this issue be moved to the Drupal core queue? I think that many migrations where d7 contrib functionality has been moved to d8 core are now handled in the core issue queue.

Cheers,
Markus

Unpostponing and moving to the core queue. I think There is nothing stopping this from happening other than people working on it :)

My colleague Jigar wrote some example i18n migrations, including one for D7 entity translation. The particularly interesting part is the custom source he used.

It's not quite a general solution—it's only for nodes. However, it definitely shows the major pieces:

1. We need a way to determine whether a D7 bundle is entity-translated. This method works for nodes only, there's no general method for this—other migration sources will need different methods.
2. As with other translatable sources, we need an extra ID field in getIds(), for language.
3. We need to modify the query so it joins against the entity_translation table, and overrides some fields. Ideally this would go in a base class and/or trait for D7 migration sources, so they all could use it.
4. We need to modify getFieldValues() so it supports a $language parameter, and then have prepareRow() use that.

Jigar also blogged about it now at https://evolvingweb.ca/blog/migrating-content-translated-entity-translat... -- can we turn that somehow into a more general solution in this issue? That would be great :)

I wanted to experiment a bit, so I thought I'd spend some time to create a solution. I didn't know how to start the "right way", so I created a repo on https://github.com/jigarius/drupal/tree/dev-2073467 and did the following:

1. Added support for a $language parameter in the d7\FieldableEntity class.
2. Added a new method d7\FieldableEntity::loadFieldValues(Row $row, $entity_type). This will allow source plugins to load field data with a single method call with quite a simple set of arguments, instead of having to call the getFields() method and then iterating over every field, etc.
3. Added a D7EntityTranslationTrait in the migrate_drupal module.

Here's what a source plugin needs to do to implement entity translation support:

1. Source plugin uses the D7EntityTranslationTrait. The plugin will need to implement the d7\FieldableEntity interface and override the query() method to call D7EntityTranslationTrait::handleEntityTranslations() (which currently has some basic arguments I could think of).
2. The source plugin implements a getEntityKeyInfo() to provide entity key data.
3. The source plugin overrides the prepareRow() method to call the loadFieldValues() method to attach field values in the language concerned ($language parameter must be specified as and when applicable to get the field values in the correct language).
4. The migration definition (YML) sets the source parameter translation_method: entity_translation. The translations: true parameter should work as expected to get things working.

Comments and suggestions are welcome. Oh! And this is the first time I'm working on Drupal core, so if I do something which is not "standard procedure", please let me know.

We've used a mixture of approaches for our entity translation migration, but one thing that is important that no-one seems to have tackled, is to migrate the field config "translate" setting across for all instances. I can't find an issue for it already? I supposed it's tied to this issue really though, as you can't have one without the other.

Updated the tests and simplified the code.

Ooops, missed most of the test failures! This should handle them all now.

+++ b/core/modules/field/src/Plugin/migrate/source/d7/FieldInstance.php
@@ -33,6 +33,11 @@ public function query() {
+    // Enables entity translation to mark instances as translatable.
+    if ($this->getDatabase()->schema()->fieldExists('field_config', 'translatable')) {

Are there any circumstances under which field_config.translatable *wouldn't* exist? If so, we should probably document these cases in a comment. If the column always exists, we can remove the if statement.

Can we get an issue summary update, that includes specify all steps that need to be done. The current patch updates the field instance source plugin and with tests, thank you. What else is needed? Changes to the fixture, fully migration test ?

Tagging migrate-critical - I did a search the other day for remaining issues using the tag and this didn't show up.

This issue most probably also #2669984: Migrate Drupal 7 user entity translations data to Drupal 8.

I'm working on a reboot of this patch.

Right now the patch only migrate the translatability of fields, not the translated data.

Let's start with a re-roll before adding the migration of the translated data.

Helps if I include the patch!

That was supposed to be NR.

Looks great! However, we definitely need an issue summary update and re-titling here, because we're not actually migrating any translated data; just the flag to indicate that a field is, in fact, translatable. Additionally, I think we need end-to-end test coverage to assert that the flag is actually set on the resulting migrated field definition.

1. +++ b/core/modules/field/src/Plugin/migrate/source/d7/FieldInstance.php
   @@ -33,6 +33,11 @@ public function query() {
   +    if ($this->getDatabase()->schema()->fieldExists('field_config', 'translatable')) {
   

   Is there any circumstance under which this field would not exist? If so, can we explain that in a comment?

2. +++ b/core/modules/field/src/Plugin/migrate/source/d7/FieldInstance.php
   @@ -33,6 +33,11 @@ public function query() {
   +      $query->addField('fc', 'translatable', 'translatable');
   

   We don't need the alias (third argument).

3. +++ b/core/modules/field/src/Plugin/migrate/source/d7/FieldInstance.php
   @@ -102,12 +107,13 @@ public function prepareRow(Row $row) {
   +      if ($language_content_type_bundle == 2 || ($language_content_type_bundle == 4 && $row->getSourceProperty('translatable'))) {
   

   Let's cast $language_content_type_bundle to int and use === here.

Re #35. I understood that the scope of this issue is a) the migration of the field setting that tells that the field is translatable and b) the actual migration of the field translations.

I also understoid from #30-31that b) is being planned here but so far we only have a). In other words, this was a mid-review before we move on to b).

For b), note the earlier work in #12-14 by @jigarius

I'll update the issue summary in the next 2h on my way to work.

Cheers,
Markus

Issue summary updated.

Typos

@masipila: Thanks for the IS update! But, on yesterday's Migrate maintainers meeting, we suggested that since this patch is small, easy to review/commit, and close to being ready, we should reduce the scope to only migrate the field's translatability here and open a second issue to migrate the translated values.

I'll take a look at the situation and see if this is necessary.

@Jo Fitzgerald: Have you started to work on migrating translated data? Because I think this patch need a complete reboot.

Right now we do migrate translatability of node fields, but Entity Translation on D7 also allow to translate fields on comments, terms & users. I think we should have a unified migration for those configurationa, not one for nodes and three other different ways for the comments, terms & users. Since I have some time today, I started to work on a new patch. Tell me if I'm stepping on your feet.

No, I haven't done any further work on this.

Go for it, @maxocub! :)

Re #40: Ok. I'm perfectly fine with scoping this to the setting part only once we reboot this and handle the content part in a follow-up.

Re #35:

1. There's no circumstances where the 'translatable' column does not exists.
2. Outdated because of 1.
3. Done.

I was wrong, this patch didn't needed a complete reboot. The patch was migrating the translatability at the field level for any entity type. What was missing was the translatability settings at the bundle level. This new patch take care of that.

Since in D7 every entity type has a different way of storing it's bundle translatability settings, the new source plugin might seem overly complicated, but I havnen't found a better way. Suggestions are welcome if you think of a better way.

There may be a conflict here with #2975509: Migrate D6 vocabulary language settings which is also migrating taxonomy vocabulary content language settings. Are entity translation and i18n mutually exclusive?

Re #46: I don't think we'll have conflicts. You can have i18n & entity_translation enabled at the same time and configure some bundles to use i18n translations and other bundles to use entity translations, but never both at the same time. That is true for node types and vocabularies.

About the test failures of my previous patch, there's some things that need to be changed but I don't have time right now. For one thing, I should not have changed the translation settings of the Article node type in the fixtures. We'll need to use a different bundle and thus different fields.

@maxocub, thanks, that is good news.

There will probably still be some test failures, but at least I think I got the fixtures right this time.

This should fix the tests.

I read through the patch and discussed the topic with @maxocub on IRC. Here's a summary of the discussion and my first review.

The patch looks very good to me. We discussed the amount of logic we have in EntityTranslationSettings::initializeIterator() but there is no way around this since the information is all over the place in D7.

According to D7 Entity API documentation, the Entity types in Drupal core are as follows:

• Nodes
• Comments
• Files
• Taxonomy terms
• Taxonomy vocabularies
• Users

Based on the code review (I did not test this manually yet), patch #51 covers all other entity types except File entities. We have now Media in D8 core so if that provides translatable fields to Files in D8, then we should probably include handling for the File entity as well. I did not check that yet.

I'll go refresh my test environment and will then test this manually, hopefully I will have more results still today. Leaving this to Unassigned, all others are more than welcome to contribute on the review and testing of this.

Cheers,
Markus

Still trying to make those tests pass. This time might be the one.

#53: however files are not fieldable and even with media in core we still migrate files to files. So, until #2835825: Add d8 media migration lands, we don't need to worry about media, or files.

A fix & an improvement:

• Last patch was marking all node types with a language_content_type_% variable as translatable, now we make sure that it uses entity translation.
• Filter out non-translatable vocabularies earlier.

Still fixing some tests.

I think this code is pretty heroic, especially the source plugin. Nice job tracing through the absolute cf that is Entity Translation settings, and organizing the unholy variable salad. My comments are mostly questions, and wondering if we can clean the code up even more to increase its clarity a little.

1. +++ b/core/modules/content_translation/migrations/d7_entity_translation_settings.yml
   @@ -0,0 +1,39 @@
   +  third_party_settings/content_translation/enabled: content_translation_enabled
   

   Based on what I'm seeing in the source plugin, content_translation_enabled is always TRUE, so we can just use default_value here to set the value to TRUE.

2. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d7/EntityTranslationSettings.php
   @@ -0,0 +1,167 @@
   +    $condition = $query->orConditionGroup()
   +      ->condition('name', ['entity_translation_entity_types', 'entity_translation_taxonomy'], 'IN')
   +      ->condition('name', 'entity_translation_settings_%', 'LIKE')
   +      ->condition('name', 'language_content_type_%', 'LIKE');
   

   We should add a comment explaining what these conditions are doing.

3. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d7/EntityTranslationSettings.php
   @@ -0,0 +1,167 @@
   +      if (preg_match('/^language_content_type_(.+)/', $name, $matches) && (int) $value === 4) {
   

   Let's add a $ anchor at the end of the regex, and explicitly declare $matches before we call preg_match().

4. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d7/EntityTranslationSettings.php
   @@ -0,0 +1,167 @@
   +    foreach (array_filter($results['entity_translation_entity_types']) as $entity_type) {
   

   Why the call to array_filter()? Is there any chance of a falsy value in $results['entity_translation_entity_types']?

5. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d7/EntityTranslationSettings.php
   @@ -0,0 +1,167 @@
   +      switch ($entity_type) {
   

   This seems like an awkward place for a switch-case. Could it be a set of if checks -- if (in_array('node', $results['entity_translation_entity_types']), TRUE)) { ... }, and so forth?

6. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d7/EntityTranslationSettings.php
   @@ -0,0 +1,167 @@
   +          foreach (array_keys(array_filter($results['entity_translation_taxonomy'])) as $vocabulary) {
   

   Again, why the call to array_filter()? Can $results['entity_translation_taxonomy'] contain any falsy values?

+++ b/core/modules/content_translation/src/Plugin/migrate/source/d7/EntityTranslationSettings.php
@@ -0,0 +1,167 @@
+  public function count($refresh = FALSE) {
+    return $this->initializeIterator()->count();
+  }

Do we need to override this? This effectively invalidates the caching added by the parent class. If we do, we should comment why it is necessary.

Re #59:

1. Done.
2. Done.
3. Done.
4. Yes, array_filter() is needed because this variable always contains the 4 entity types as keys. For each entity type, if entity translation is enabled, the value is the entity type ID, otherwise it's false.
5. Good idea, I refactored the switch-case in favor of if statements with in_array() calls.
6. Same thing as with the entity types, this variable can contain a vocabulary that does not use entity translation, and in that case it's value is false.

Re #60:
Yes, we do need to override count() beacuse the the number of variables we fetch with query() does not match the actual number of rows generated by initializeIterator(). I added a comment saying so.
The count() that we're overriding is the one in SqlBase and it's counting the number of results from query(), which is not what we want. Also, is it really caching the results?

  /**
   * {@inheritdoc}
   */
  public function count($refresh = FALSE) {
    return (int) $this->query()->countQuery()->execute()->fetchField();
  }

Just a few self-review nits.

I manually tested the patch #62 with the following results.

1. Scope of the test

As mentioned in #53, according to D7 Entity API documentation, the Entity types in Drupal core are as follows:

• Nodes
• Comments
• Files
• Taxonomy terms
• Taxonomy vocabularies
• Users

My test covered Nodes, comments, taxonomy vocabularies / terms, users. Files were not covered as Files are not fieldable in D8 core as per #55.

2. Nodes

2.1 Content type Article with Entity Translation enabled in D7
- 'Enable translation' setting was migrated correctly to D8 on bundle / content type (translation was enabled)
- TEST OK

2.2. Content type Page with no translations enabled in D7
- 'Enable translation' setting was unchecked after migration as expected on bundle / content type level
- TEST OK

2.3 Field level setting on Article
- Article content type had fields in D7 that had Field Translation enabled.
-- 'Users may translate this field' settings was migrated correctly to all these fields (settings was enabled in D8).
- Article content type had fields in D7 that had Field Translation disabled
-- 'Users may translate this field' settings was migrated correctly to all these fields (settings was disabled in D8).
- BOTH TESTS OK

2.4 Observation on field storage / field instance settings
- The field translation setting is a property of Field Storage in D7
- This means that it is possible that the field level setting is contradicting the bundle level setting in D7
- Example:
-- We have content types Article and Page.
-- Article has Entity Translation enabled and Page disabled
-- We have field_text which appears on both Article and Page.
-- The field is set as translatable on Field Storage level because it is translatable on Article
-- This setting is applied everywhere the field is used in D7 even though Page is set as not-translatable on bundle level
- The data model is the same on D8 i.e. the field translation setting is applied everywhere where the field is used.
-- I tested this manually by creating a field on D8 and adding it to Article which is translatable.
-- I enabled the field translation for this field on Article
-- I added this same field to Page which is not translatable on bundle level in D8
-- The field 'User may translate this field' is enabled on Page but the field is grayed out i.e. it is not possible to unselect the checkbox. See screenshot below.
- We are migrating this correctly so this is just an observation and everything is OK.
-- This edge case is good to keep in mind when we write the actual field content migration.

2.5 Summary of node tests
All my tests passed.

3. User entity

3.1 User entity (admin/config/people/accounts) does not have a translation setting in D7

3.2 When I have translatable field in D7, the D8 user 'Enable translation' setting is enabled after the migration.
- TEST OK.

3.3 The field level setting on User entity is migrated correctly.
- The field that is set as translatable in D7 has the translation setting enabled in D8
- And fields that are not set as translatable in D7 have the translation setting disabled in D8
- TEST OK

4. Taxonomy vocabulary / taxonomy terms

4.1 Vocabulary Tags had 'Enable field translation' setting enabled in D7
- This settings was migrated to D8 correctly
- TEST OK

4.2 Vocabulary Forums had 'Enable field translation' disabled in D7
- This settings was disabled on D8
- TEST OK

4.3 Vocabulary Tags had a custom field which had field translation enabled on D7
- This was migrated correctly to D8
- TEST OK

4.4. Vocabulary Tags had fields that did not have field translation enabled on D7
- These were not marked as translatable on D8
- TEST OK

4.5 Summary
- All tests OK on taxonomy vocabulary / taxonomy terms

5. Comment type entity

5.1 Different bundles can have different fields on D7
- This is why we create different comment types when migrating to D8
- In other words, we will have comment_node_article and comment_node_page
- As we painfully remember, comment_forum is a special snowflake because it is hard coded in Forum module

5.2 comment_node_article
- In my D7 site, Article comments had fields where some fields are translatable and some not
- D8 comment_node_article has 'Enable translation' enabled as expected
- TEST OK

5.3. comment_node_page
- In my D7 site, Page comments have fields which are not translatable
- D8 comment_node_page has 'Enable translation' disabled as expected
- TEST OK

5.4 Field translation setting of comment_node_article
- Fields that were marked as translatable in D7 are so also on D8.
-- TEST OK
- Fields that were not marked as translatable in D7 are so also on D8.
-- TEST OK

5.5 comment_forum translation setting
- D7 Forum comment_body was set as translatable
- D8 comment_forum does not have translation enabled. See screenshot below
- TEST FAILED

5.6. comment forum field level setting
- D7 Forum comment_body was set as translatable
- This field level setting was migrated correctly to D8
- TEST OK

Conclusion

- Everything else except 5.5 was OK. Kicking back to NW for that.
- This issue had unexpectedly high amount of different permutations. It was a very good decision to handle the translation setting migration separately from the actual field content translation :)
- I'll leave it to others to consider if the different permutations I listed here are sufficiently covered in automated tests.
- Hats off to the max to @maxocub for this patch. \o/

Cheers,
Markus

Thanks for working on this!

The approach in general looks sane but, given that there's quite some duplicated logic, I'm wondering whether it would make more sense to collect it into a base class that individual entity types could use to migrate their own ET settings. I think this would make sense also because in contrib there are quite a few modules exposing entity type definitions and integrating with ET. The first that come to my mind are ECK, Media, Commerce Product. These could extend the same base class.

With this approach each core module (node, comment, taxonomy, users) would expose its own source, that would extend the base class provided by Content Translation.

+++ b/core/modules/content_translation/src/Plugin/migrate/source/d7/EntityTranslationSettings.php
@@ -0,0 +1,174 @@
+          'language_alterable' => isset($settings['hide_language_selector']) ? (bool) $settings['hide_language_selector'] : TRUE,

At first sight this looks weird: I'd expect language to be alterable if the selector is NOT hidden.

@plach: Thanks for the review. A base class seems like a good idea.

Discussed in the migrate maintainers meeting between @phenaproxima, @heddn, @maxocub about the idea of a base class. While on the surface it makes sense, in reality a base class needs to provide functionality that is re-usable by multiple implementing classes. We don't really have much that is re-usable here in that source class. Except maybe the count method. Let's noodle on this a little and see if we get any benefit or not; but right now it just might not make sense.

Replying to myself in #63 / 5.5 which I reported as a failing test case.

The logic we have in the patch is as follows:

+      // A comment type uses entity translation if the associated node type
+      // uses it. 

This logic is ok and the point 5.5 I made in #63 is not valid.

I'll test a bit more over the weekend.

I'll leave this to NW for #66. If it turns out that the base class can't provide much that can be re-used, then the remaining work in my eyes is to double check test coverage.

Cheers,
Markus

Markus

I will also update the issue summary over the weekend to help other reviewers and committers so that the IS clearly describes all the different settings that we are migrating.

Issue summary should now be up-to-date on what this patch is conceptually doing.

Did we already reach a conclusion on the base class topic? If the conclusion is that the base class does not bring any additional value, then please change the status to Needs review. As mentioned above, I will do a second round of manual testing to ensure that all possible permutations are covered. I'm quite sure they are but I will re-test this once more and cover the permutations that were not covered in #63..

Cheers,
Markus

Thanks @masipila for the IS update.
I did some manual testing too and I was not able to reproduce the problem mentioned in #63.
The idea of the base class, while generally a good idea, would not be very useful here since not a lot of code could be reused.
So back to NR.

I just read the patch once more on my way to work and all aspects seem to be covered. I should be able to do the final test today Friday or latest tomorrow and RTBC this.

Cheers,
Markus

I will address the last point in #64 later today.

I just re-tested this and all aspects that are listed in the Issue Summary were OK.

I discussed about @plach's comment in #64 with @maxocub, he promised to address the language selector topic. @maxocub also mentioned that the default for hide_language_selector is different in D7 for different entities. I would appreciate inline comments about this gotcha...

Markus

Here's addressing the language_alterable concern from #64.

I'll test this today.

I tested manually the default language and 'Show language selector on create and edit pages' settings and they were all migrated as expected. All feedback has been addressed, there is test coverage, patch has received good feedback from multiple reviewers. RTBC.

Great work maxocub!

Cheers,
Markus

Committed/pushed to 8.6.x, thanks!

This doesn't apply to 8.5.x, marking fixed but since it's a migrate issue we could backport it to 8.5.x still - feel free to re-open this issue against 8.5.x if you re-roll.

Rerolled against 8.5.x.

Thanks @catch for committing this to 8.6.x!

Regarding the backport to 8.5.x. The only diff in the 8.5.x backport seems to be the context in core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7ReviewPageTest.php

< @@ -57,6 +57,7 @@ protected function getAvailablePaths() {
---
> @@ -41,6 +41,7 @@ protected function getAvailablePaths() {

Since this diff is in the tests and the 8.5.x testbot liked the backport, I'm moving this back to RTBC.

Markus

Committed 3a35e13 and pushed to 8.5.x. Thanks!

With the backport patch for 8.5 from #80 applied there are no translation settings migrated for the node types. Perhaps I'm missing some steps?

Patch in #75 works for me. On a site with 3 different languages each node had the right language setting.

Since this issue is closed, please open a new one with steps to reproduce.

@Nchase: and please link in the issue here :)