Problem/Motivation

The d7 menu translations need a migration

Proposed resolution

Migrations

  • Add menu migration, d7_menu_translation.yml
  • add langcode to d7_menu.yml

Source plugins

  • Update Menu.php and source plugin test
  • Add MenuTranslation.php

Tests

  • aAdd source plugin test MenuTranslationTest.php
  • update MigrateMenuTest
  • add MigrateMenuTranslationTest
  • update entity counts in Upgrade7Test

Update migration state in config_translation.migrate_drupal.yml

Fixture changes, drupal 7 - Add translations of menus

Remaining tasks

  • Re-roll
  • Release manager review for 9.0.x
CommentFileSizeAuthor
#58 3112249-58.patch18.29 KBmanish-31
#51 3112249-51.patch18.86 KBquietone
#51 interdiff-49-51.txt1.26 KBquietone
#49 3112249-48.patch18.86 KBquietone
#49 interdiff-46.48.txt929 bytesquietone
#46 3112249-46.patch17.95 KBquietone
#46 interdiff-43-46.txt1.81 KBquietone
#43 3112249-43.patch17.91 KBquietone
#43 diff-30-43.txt81.04 KBquietone
#38 3112249-38.patch90.85 KBravi.shankar
#37 adding_en.png11.06 KBrajiv.singh
#31 3112249-31-9.0.patch90.82 KBquietone
#30 3112249-30.patch90.87 KBquietone
#30 interdiff-28-30.txt697 bytesquietone
#28 3112249-28.patch90.02 KBquietone
#28 interdiff-19-28.txt78.34 KBquietone
#19 3112249-19.patch15.66 KBshaktik
#18 Screen Shot 2020-05-18 at 11.34.42.png162.36 KBshaktik
#13 3112249-13.patch16.17 KBquietone
#13 interdiff-12-13.txt2.82 KBquietone
#12 3112249-12.patch16.61 KBquietone
#12 interdiff-11-12.txt15.58 KBquietone
#11 interdiff-10-11.txt5.92 KBquietone
#11 3112249-11.patch16.78 KBquietone
#10 interdiff-8-10.txt555 bytesquietone
#10 3112249-10.patch12.13 KBquietone
#8 interdiff-7-8.txt75.17 KBquietone
#8 3112249-8.patch12.09 KBquietone
#7 3112249-7.patch86.76 KBquietone
#7 interdiff-6-7.txt29.29 KBquietone
#6 3112249-6.patch130.08 KBquietone
#6 interdiff-4-6.txt609 bytesquietone
#4 3112249-4.patch129.83 KBquietone
#4 interdiff-2-4.txt547 bytesquietone
#2 3112249-2.patch129.29 KBquietone
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
3112249-2.patch129.29 KB

And a first go at the patch. This adds translations of the main menu in both Icelandic and French for both the label and description and a French translation of the description for the test menu.

Status: Needs review » Needs work

The last submitted patch, 2: 3112249-2.patch, failed testing. View results

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff-2-4.txt547 bytes
3112249-4.patch129.83 KB

Forgot to add i18n_menu to the Upgrade7Test.

Status: Needs review » Needs work

The last submitted patch, 4: 3112249-4.patch, failed testing. View results

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff-4-6.txt609 bytes
3112249-6.patch130.08 KB

Add a menu entity to Upgrade7Test.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue tags: +migrate-d7-d8, +i18n-migrate
FileSize
interdiff-6-7.txt29.29 KB
3112249-7.patch86.76 KB

Trying to reduce the size of the patch by removing changes to the fixture.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
3112249-8.patch12.09 KB
interdiff-7-8.txt75.17 KB

Reroll because #3110669: Migrate d7 menu language content settings was committed.

Status: Needs review » Needs work

The last submitted patch, 8: 3112249-8.patch, failed testing. View results

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
3112249-10.patch12.13 KB
interdiff-8-10.txt555 bytes

Remove duplicate entry in getMissingPaths().

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
3112249-11.patch16.78 KB
interdiff-10-11.txt5.92 KB

Now it seems very odd to be adding translations of menus when the d7_menu migration doesn't have a process for the language. This adds such a process to d7_menu.yml and modifies the test to check the language. And MigrateMenuTranslationTest has a spelling correction and adds a test for the fixedlang menu.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
interdiff-11-12.txt15.58 KB
3112249-12.patch16.61 KB

Now move source plugin and tests from config_translation to system.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
interdiff-12-13.txt2.82 KB
3112249-13.patch16.17 KB

Self review and found some things to fix. The migration_tag property was missing from the migration. Changed the menu translation source plugin to extend from the menu source plugin and reworked the source plugin methods.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at Four Kitchens commented
Status: Needs review » Reviewed & tested by the community

This one looks good to me, I couldn't find any nits

shaktik’s picture

shaktik
he/him
Primary language Hindi
Location New Delhi
CreditAttribution: shaktik as a volunteer and at Material for Drupal India Association commented
Assigned: Unassigned » shaktik

Test case fails on D9.1, so I am working on it.

shaktik’s picture

shaktik
he/him
Primary language Hindi
Location New Delhi
CreditAttribution: shaktik as a volunteer and at Material for Drupal India Association commented
Status: Reviewed & tested by the community » Needs work
shaktik’s picture

shaktik
he/him
Primary language Hindi
Location New Delhi
CreditAttribution: shaktik as a volunteer and at Material for Drupal India Association commented
FileSize
Screen Shot 2020-05-18 at 11.34.42.png162.36 KB

patch fail#13

shaktik’s picture

shaktik
he/him
Primary language Hindi
Location New Delhi
CreditAttribution: shaktik as a volunteer and at Material for Drupal India Association commented
Status: Needs work » Needs review
FileSize
3112249-19.patch15.66 KB

Rerolled the patch #13 to made supportable 9.1.x-dev and solved test case issues.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

@shaktik, thanks for the reroll. Can you please provide a diff between the patches in #13 and #19?

shaktik’s picture

shaktik
he/him
Primary language Hindi
Location New Delhi
CreditAttribution: shaktik as a volunteer and at Material for Drupal India Association commented

Hi @quietone,

Please find the mention below diff between the patches in #13 and #19

diff --git a/core/modules/system/tests/src/Kernel/Migrate/d7/MigrateMenuTranslationTest.php b/core/modules/system/tests/src/Kernel/Migrate/d7/MigrateMenuTranslationTest.php
new file mode 100644
- protected function setUp() {
+ protected function setUp(): void {
diff --git a/core/modules/system/tests/src/Kernel/Migrate/d7/MigrateMenuTest.php b/core/modules/system/tests/src/Kernel/Migrate/d7/MigrateMenuTest.php  
-  protected function setUp() {
+ protected function setUp(): void {
shaktik’s picture

shaktik
he/him
Primary language Hindi
Location New Delhi
CreditAttribution: shaktik as a volunteer and at Material for Drupal India Association commented
Assigned: shaktik » Unassigned
quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue tags: +blocker

this is blocking #3008028: Migrate D7 i18n menu links.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at Four Kitchens commented
Status: Needs review » Reviewed & tested by the community

I'll put this back to RTBC for 9.1, but I wonder if it should be backported and how far. We may want to re-roll for 8.9 and 9.0, as these migrations are important for the ecosystem.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Issue tags: +Needs release manager review

fwiw I am +1 to backporting this to 9.0.x and 8.9.x, even in a patch release if that ended up being necessary. Tagging for a second opinion from @xjm though. Haven't reviewed the patch properly yet.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

Added tests for PostgreSQL and SQLlite for the 9.1.x.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Assigned: Unassigned » quietone
Status: Reviewed & tested by the community » Needs work

Looks like the fixture has changed.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff-19-28.txt78.34 KB
3112249-28.patch90.02 KB

And yet another reroll, this time to update the test fixture.

Status: Needs review » Needs work

The last submitted patch, 28: 3112249-28.patch, failed testing. View results

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff-28-30.txt697 bytes
3112249-30.patch90.87 KB
quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
3112249-31-9.0.patch90.82 KB

And a version for 8.9 and 9.0

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Needs work

Nice that all the tests are passing but I want to check the migration state file.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review

The migration state file is correct. This is ready for review again.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Assigned: quietone » Unassigned

Forgot to un-assign myself.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue tags: +Needs reroll
quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Needs work
rajiv.singh’s picture

rajiv.singh CreditAttribution: rajiv.singh as a volunteer and commented
FileSize
adding_en.png11.06 KB

#31 worked for D8.9 but adding "(en)" in the label of existing menu links during translation
Issue

ravi.shankar’s picture

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3112249-38.patch90.85 KB

Here I have added reroll of patch #31.

Status: Needs review » Needs work

The last submitted patch, 38: 3112249-38.patch, failed testing. View results

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

@ravi.shankar, You may not have noticed but there is a 9.1.x patch in #30. The patch in #31 really is just for 8.9.x and 9.0.x. To help reviewers please include an interdiff, or a diff if the interdiff fails, when rerolling patches.

ravi.shankar’s picture

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented

Thanks @quietone

Oh Yes, I didn't notice that there is a patch #30 for Drupal 9.1.x and patch #30 still applies on Drupal 9.1.x.

Please ignore patch #38.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue tags: +Needs reroll

The reroll here should use the fixture changes from #3110669: Migrate d7 menu language content settings.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
diff-30-43.txt81.04 KB
3112249-43.patch17.91 KB

Rerolling now that #3110669: Migrate d7 menu language content settings is in.

The diff is very large because the patch in #30 included the work from #3110669: Migrate d7 menu language content settings which is no longer needed in the current patch. The only conflicts during the reroll were to the fixture.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue tags: -Needs reroll

The reroll has been successfull, all tests passing so ready for review.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work

Patch looks good.

  1. +++ b/core/modules/system/src/Plugin/migrate/source/d7/MenuTranslation.php
@@ -0,0 +1,82 @@
+      ->fields('i18n', [
+        'lid',
...
+        'type',

    In core/modules/field/src/Plugin/migrate/source/d7/FieldOptionTranslation.php they removed those fields and added them with $query->addField(). Should we do here the same? 

  2. +++ b/core/modules/system/src/Plugin/migrate/source/d7/MenuTranslation.php
@@ -0,0 +1,82 @@
+      ->fields('lt', [
...
+        'language',

    Should this field not be in $query->addField()? 

  3. +++ b/core/modules/system/src/Plugin/migrate/source/d7/MenuTranslation.php
@@ -0,0 +1,82 @@
+    $query->addField('m', 'language', 'm_language');
...
+    $ids['language']['alias'] = 'lt';

    Which one is the correct language? It is a bit confusing to me. 

  4. +++ b/core/modules/system/tests/src/Kernel/Migrate/d7/MigrateMenuTranslationTest.php
@@ -0,0 +1,67 @@
+    $this->assertNull($config_translation->get('description'));
+    $this->assertNull($config_translation->get('description'));
...
+    $this->assertNull($config_translation->get('description'));
+    $this->assertNull($config_translation->get('description'));

    Why are we doing the same test twice? Maybe changing one of them to "label" instead of "description".

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
Parent issue: #3008028: Migrate D7 i18n menu links » #2208401: [META] Remaining multilingual migration paths
FileSize
interdiff-43-46.txt1.81 KB
3112249-46.patch17.95 KB

@daffie, thanks for the review.

1. I don't think so. In FieldOptionTranslation.php the alias is needed for 'type' because is exists on the field_config table as well as the 'i18n_string'. There is no 'type' in another table in the query. The alias was added for 'lid' so that the query could be sorted by the 'lid' field, that isn't being done here. A look at some of the other translation source plugins (FieldLabelDescriptionTranslation, BlockCustomTranslation, BlockTranslation) and it seems that FieldOptionTranslation is unique in making aliases for those fields.

2. It isn't there so that 'language' can be used in the migration and not an alias. Is there a better way to do that?

3. The language used for the source id is the language from the table with an alias of 'lt', which is locales_target. The 'alias' property is not uncommon in getIds() and is well documented at \Drupal\migrate\Plugin\MigrateSourceInterface::getIds. Because of that there is no additional comment.

4. Ah, copy/paste error. Fixed.

Added missing field 'language' and 'plural' to \Drupal\system\Plugin\migrate\source\d7\MenuTranslation::fields

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Reviewed & tested by the community

@quietone: Thanks for your explanation.

All my points are addresed.
All the required testing is in the patch.
All code changes now look good to me.
For me it is RTBC.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs review

Did a self review and noticed that the source plugin Menu.php has been changed but the source plugin test wasn't. This adds a test to the existing MenuTest.php.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
interdiff-46.48.txt929 bytes
3112249-48.patch18.86 KB

Forgot to upload the patch.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Reviewed & tested by the community

The extra testing looks good to me.
Back to RTBC.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Reviewed & tested by the community » Needs review
FileSize
interdiff-49-51.txt1.26 KB
3112249-51.patch18.86 KB

s/public static $modules/protected static $modules/

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Reviewed & tested by the community

Good change

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Title: Migrate d7 menu translation » [backport] Migrate d7 menu translation
Version: 9.1.x-dev » 9.0.x-dev

Committed 61ca2c2 and pushed to 9.1.x. Thanks!

Switching to 9.0.x to - this is tagged Needs release manager review for possible backport.

  • larowlan committed 61ca2c2 on 9.1.x 
    Issue #3112249 by quietone, shaktik, ravi.shankar, rajiv.singh, daffie:...
benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll, +Novice

The patch in #51 does not apply to 9.1.x (since it has already been applied) but it also does not apply to 9.0.x. I think we need a reroll, which is a Novice task.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue tags: -Novice

In order to reroll this the fixture changes from #3110669: Migrate d7 menu language content settings, which did not get committed to 9.0.x, needs to be incorporated. Should we commit that to 9.0.x first or not?

manish-31’s picture

manish-31 CreditAttribution: manish-31 at OpenSense Labs for DrupalFit commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3112249-58.patch18.29 KB

Added a patch for Drupal 9.0.x

Status: Needs review » Needs work

The last submitted patch, 58: 3112249-58.patch, failed testing. View results

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue summary: View changes
Status: Needs work » Postponed

Since this needs the fixture changes from #3110669: Migrate d7 menu language content settings and that migration should be ported as well, marking this as postponed.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

Adding clarification for the remaining i18n migrations. There are three of these issues that could be backported and they need to be done in this order. One reason is because of the fixture changes.

  1. #3110669: Migrate d7 menu language content settings
  2. #3112249: Migrate d7 menu translation
  3. #3008028: Migrate D7 i18n menu links

Version: 9.0.x-dev » 9.1.x-dev

Drupal 9.0.10 was released on December 3, 2020 and is the final full bugfix release for the Drupal 9.0.x series. Drupal 9.0.x will not receive any further development aside from security fixes. Sites should update to Drupal 9.1.0 to continue receiving regular bugfixes.

Drupal-9-only bug reports should be targeted for the 9.1.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.2.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher at FRUITION commented
Title: [backport] Migrate d7 menu translation » Migrate d7 menu translation
Issue summary: View changes
Status: Postponed » Fixed

We decided it is too late to backport #3110669: Migrate d7 menu language content settings, so we will not backport this issue either. Fixed for 9.1.x.

Status: Fixed » Closed (fixed)

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