Migrate D7 i18n taxonomy term language

This is using the test fixture from #3022137: Update migrate fixtures for remaining active issues which still needs some work but I am doing all the rest of the fixtures changes in one issue.

While this works it isn't clear what to do when both entity translations and i18n translation are available. So, this could be wrong.

That is better than it looks, most of the error are because of changes that need to be removed from the fixture. There is one error related to this issue, the one in Taxonomy.Drupal\Tests\taxonomy\Kernel\Migrate\d7\MigrateTaxonomyTermTest

This fixes the relevant test failure. The others will be fixed in the fixture issue.

I

Good, that fixed the relevant test. What this needs next is review and testing, yes, even with the failing tests. The tests will be fixed later, when the fixture has a few more additions. So setting to NR.

If you find yourself here the patch is #9 is ready for testing. If you want to review the code, look at 2979966-6-review-only.txt in comment #8.

Hi @quietone, I tried applying the patches in #6 and #9 to 8.6.12 and both broke taxonomy term migration. Do I need to be on 8.7.x?

Can we get a review only patch?

@PapGrande, thanks for trying to apply the patch. You will need to be on 8.8.x for the patch to apply.

Can we get a review only patch?

Sure, the review only patch is in #8. The change in #9 was a correction to the test, that is the vid number changed, and I'd rather not make a new patch just for that one line.

This looks fine. Based on the IS, we've got test cases for all the scenarios. We've got data fixture changes that covers all of it.

I'm not sure though about the fixture changes that come from #3022137: Update migrate fixtures for remaining active issues. Do we need to wait until that issue lands or just commit what we have here and it will shrink in scope?

Good point. I will double check what to do in relation to #3022137: Update migrate fixtures for remaining active issues. Back to NW for that point.

Really set to NW

@heddn, thanks for pointing this out.

The fixture in this patch is the same as the one over in #3022137: Update migrate fixtures for remaining active issues and it appears that the remaining data is in. An exception is #3008028: Migrate D7 i18n menu links which already has fixture changes and that one can be done later.

What should happen next is work on the fixture itself to reduce it size, it add about 250k to the fixture. Then commit that and then work on the remaining multilingual issue and hopefully not have to modify the fixture again. Therefore postponing this.

this patch removes the fixture and replaces it with the one from #3022137: Update migrate fixtures for remaining active issues. Lets see if anything breaks.

That's good that all tests pass. This was just an extra check that everything is OK with the new fixture. This is back to postponed.

When that is fixed the patch in #9 is to be used, or rerolled if needed. Do not use the patch in #19.

This rerolls the patch in #9. And now we have a patch without the fixture and much easier to focus on the work being done here.

No interdiff because the reroll is removing the fixture and it would all be noise. The patch is now down to a reasonable 8k.

This is ready for review. This was RTBCed in #15 and since then there hasn't been any code changes. What has changed is the fixture. It was committed on its own with changes from this patch, other patches and what is hoped the data for the remaining i18 patches.

Thanks, this looks great. Some notes for naming and scoping:

1. --- /dev/null
   +++ b/core/modules/content_translation/migrations/d7_taxonomy_term_translation.yml
   
   @@ -0,0 +1,46 @@
   +  translations: true
   ...
   +  destination_module: content_translation
   

   Since this is about individual language codes stored on taxonomy terms, why are we scoping this to the content translation module? The Drupal 8 language module already provides this functionality (if enabled on the vocabulary).

   Also, do we need the translations:true clause on the source for this case?

2. +++ b/core/modules/content_translation/migrations/d7_taxonomy_term_translation.yml
   @@ -0,0 +1,46 @@
   +id: d7_taxonomy_term_translation
   +label: Taxonomy terms
   
   +++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d7/MigrateTaxonomyTermTranslationTest.php
   @@ -0,0 +1,139 @@
   +  public function testTranslatedTaxonomyTerms() {
   +    $this->assertEntity(19, 'und', 'Jupiter Station', 'vocablocalized', 'Holographic research.', 'filtered_html', '0', []);
   +    $this->assertEntity(20, 'und', 'DS9', 'vocablocalized', 'Terok Nor', 'filtered_html', '0', []);
   +    $this->assertEntity(21, 'en', 'High council', 'vocabtranslate', NULL, NULL, '0', []);
   +    $this->assertEntity(22, 'fr', 'fr - High council', 'vocabtranslate', NULL, NULL, '0', []);
   +    $this->assertEntity(23, 'is', 'is - High council', 'vocabtranslate', NULL, NULL, '0', []);
   +    $this->assertEntity(24, 'fr', 'FR - Crewman', 'vocabfixed', NULL, NULL, '0', []);
   

   This shows well that the resulting taxonomy terms are not translations in the D8 sense. Should we name this migration "taxonomy language" instead (as the issue is scoped).

   And apply that then to the ID and the label.

Think you are right about that. This fixes the above #1 and #2.

+++ b/core/modules/taxonomy/tests/src/Kernel/Migrate/d7/MigrateTaxonomyTermLanguageTest.php
@@ -0,0 +1,136 @@
+    if (!isset($this->treeData[$vid])) {
+      $tree = \Drupal::entityTypeManager()->getStorage('taxonomy_term')->loadTree($vid);

Is this still needed? I think the parent now comes through on the entity in 8.6+. Leaving at NR if my question is wrong.

$row->get() is not supported yet in Drupal 8.6.x
Adapted that call to $row->getSourceProperty() to be able to use the patch on Drupal 8.6.x
Based on the patch from comment #24.
See attached.

Accidentally uploaded the patch with .txt extension. Corrected it to the .patch extension.

This will not be backported to 8.6.x, so that change is not helpful unfortunately.

25.1 - I don't know about the parent coming through in 8.6 but removing that code causes the tests to fail, as shown in the attached fail patch.

Re uploading the successful patch from #24 for convenience.

Obviously we don't get the right structure from parents. All feedback is now addressed.

Committed and pushed 81543e1755 to 8.8.x and 379763d5e5 to 8.7.x. Thanks!

This is causing failures in the UI tests of the complete upgrade in commerce migrate. It is because this is tagged Multilingual and in the taxonomy directory. Maybe the following changes are needed as well as rerolling #2936365: Migrate UI - allow modules to declare the state of their migrations.

1. +++ b/core/modules/taxonomy/migrations/d7_taxonomy_term_language.yml
   @@ -0,0 +1,45 @@
   +  - Multilingual
   

   Remove?

2. +++ b/core/modules/taxonomy/migrations/d7_taxonomy_term_language.yml
   @@ -0,0 +1,45 @@
   +  destination_module: content_translation
   

   Remove?

Hm, why would this not be a multilingual migration?!

Well, you pointed out that the resulting taxonomy terms are not translations in the D8 sense. So they are some other category. What are they?

The practical problem is that now if one has taxonomy installed then this new migration is found. And then if you want to migrate a source site that is not multilingual you will still have to install migrate_drupal_multilingual. That does not make sense and I should have caught that earlier.

When the Multilingual tag is found and migrate_drupal_ui is not installed an error is thrown and user gets a message the migrate_drupal_multilingual must be installed to continue. This is why it is convenient to have all the migrations tagged Multilingual in content_translation or config_translation. Though, as you point out, having this migration in content_translation isn't quite right either but it is a possible solution.

This could also go into language, which maybe should be the destination_module. But it is very late and I could be writing nonsense.

Thought about this a bit this morning from the perspective of using the UI. Say I am migrating a non-multilingual site and have taxonomy enabled. I will get an error stating that I need to install migrate_drupal_multilingual in order to run d7_taxonomy_term_language.yml. Since my site doesn't need that I just want to run all the other migrations. But in the UI there is no way to proceed without enabling migrate_drupal_multilingual. And changing that requires a new form to ask the question as well as changes to the logic that gets the migrations that are to run. Is that what needs to be done? What other ways to address this?

I think this needs to be reverted because folks need to install migrate_drupal_multilingual to migrate a site that is not multilingual.

Talked briefly with mikelutz in Slack about this.

In MigrateConfigurationTrait, we should be able to tell if the source database has a multilingual setup, shoudn’t we skip Multilingual migrations if it doesn’t instead of throwing an error?
quietone  [1 day ago]
Maybe, we don't check the source to determine multilingual. What if the source is multilingual and the user doesn't want to migrate translations?
mikelutz  [24 hours ago]
Well, that’s a whole new UI to let people pick and choose migrations to run.
quietone  [24 hours ago]
Not really, could just be a simple question  'do you want to upgrade translations' (edited)
mikelutz  [24 hours ago]
The core UI doesn’t really offer meaningful support for partial migrations atm, being able to skip migrations because the modules aren’t enabled is less a feature and more a technical need.
mikelutz  [24 hours ago]
Personally, I’d prefer the core UI was able to detect and enable the modules it needs on it’s own. :stuck_out_tongue:
mikelutz  [24 hours ago]
Purely from a UX perspective.

For me, the key point is that the "core UI doesn’t really offer meaningful support for partial migrations atm," There is an issue to add support for doing Content or Configuration migrations only and that has problems as well. However, it is the closest we have to what I think is needed here and there is a patch over there as well. I think it is worth looking at #2826742: Expose migration types in Migrate UI.

@quietone so this is a thing that got refined in Drupal 8. In Drupal 7 you need to enable locale module and i18n to assign language to taxonomy terms, so it is quite deep in being multilingual. In Drupal 8, you only need to enable the language module if the languages being assigned are other than English. That could be a monolingual site in Spanish let's say but it requires infrastructure for multilingual (language module in D8 and locale+i18n modules in D7). So to me it looks a bit closer to the borderline but still multilingual :) It is not a translation thing, but multilingual is not just translations :)

OK. I think the thing to do here is to move the move the migration to language, since language needs to be enabled, and remove the Multilingual tag.

No interdiff since this is a smallish patch.

Since d7_taxonomy_term.yml has the audit key I've added it to d7_taxonomy_term_translation.yml as well. And it seems the test has a list of migrations with audit set, so the new migration has to be added to that MigrateDrupal7AuditIdsTest.

Hi folks, this is ready for review.

Hm, this patch does a WHOLE LOT MORE than just transporting the language value. It seems to be doing entity translation support, title module support, field translatability support, etc. That is like 3 different ways to translate stuff on terms. The tests don't seem to reflect that complexity.

At the same time the title of the issue and this summary snippet says very different about the scope:

The scope of this issue is to migrate the language code of the terms from D7 to D8. The language code is stored in taxonomy_term_data.language database column.

IMHO it is great that we can support those migrations as well, but then we need to cover with tests and update the issue summary for clarity.

Thanks for the review. It got me to start over and what I saw is duplication. With many of the migration of translation it starts with building on the existing migration for the entity involved so that is what I did. The existing d7_taxonomy_term migration already includes the language property and the source plugin has logic to determine the language to use based on the default language and if entity translation is enabled. All this needs to do is add logic to use the i18n language, if it is available, to the source plugin. And MigrateTaxonomyTerm test has been modified to test the language field.

The patch is much smaller and the interdiff is full of deletions.

Because this is now reduced in scope I removed the Needs issue summary update tag.

Oops, accidentally removed the change to the Term source plugin.

This patch looks good! Thanks for focusing on scope!

Is entity translation support, title module support and field translatability support then a separate issue? I think its fine to focus this on term language, we can get it in nice and neat, but then we should have those translations represented somehow, because unless they somehow already got in in other issues, they will still be missing features. And the implementations looked fine, there was just missing tests :)

Is entity translation support, title module support and field translatability support then a separate issue?

The entity translation seems to be done already. There is a d7_taxonomy_term_entity_translation.yml file. This migration is run in the existing d7/MigrateTaxonomyTermTest.php and there is a test method testTaxonomyTermEntityTranslations. It was done in #2980996: Migrate Drupal 7 taxonomy term entity translations data to Drupal 8. That would have been maxocub who did the entity translation work.

The title module support is already in the prepareRow method of the Term.php source plugin.

Not sure about 'field translatability', so I'd like to move that to a follow up issue. Setting to NW to make this issue.

Follow up made, [#3073050,] and there is nothing else to do here so back to NR.

Thanks for adjusting the scope here, much simpler and indeed now fulfills the mission set out in the issue summary. Looks very straightforward. Thanks!

So, when the i18n_mode is localized we want to use the default language, not the language in the row.

This patch focusing on migrate the language code of the terms from D7 to D8. Thanks for above patch.

@quietone: can you upload an interdiff please?

Sorry, forgot to upload it.

And now that the migration is returning the correct language for the localized term, the test needs to change to use that language. Which makes sense that it would be the default language and not 'und'.

Makes total sense. The postgres test had 2 fails above, but one was from #3008029: Migrate D7 i18n field option translations where it got fixed since then. So that should not reappear randomly anymore. Thanks!

Committed 32f4d3e and pushed to 8.8.x. Thanks!

I think we should highlight a list of all the significant improvements to multilingual migrations in 8.8 (whether or not the module stabilizes in this release). Tagging accordingly.