Migrate D7 i18n menu links

Attached patch applies on D8.6.3 that has all the code changes required.

Still doesn't apply the language to the imported menus though, all links come out with the site default language.

@quietone I still haven't managed to get the patch to migrate the languages too :|

Attached patch applies against 8.6.x
There is no interdiff from #6 because there are only line offset and no code changes.

In any case, we couldn't get localized menus to migrate with the patch from this issue.
All our work is documented here: #3013625: Menu language not properly migrated from D7 localized menu links. where we made a patch that migrates localized menus.

As per #2208401-96: [META] Remaining multilingual migration paths, I reviewed this patch for BC breaks. As it turns out, it only adds new plugin implementations and migrations, and doesn't change any APIs at all.

So, rest easy, release and framework managers: there are no BC breaks or implications in this patch.

Not postponed anymore.

Some code in the fields list looks odd, like these:

+      'menu_name' => $this->t('The menu name . Primary key'),
+      'title' => $this->t('The human - readable name of the menu'),

Some docs are copy paste:

+  /**
+   * Tests migration of system (site) variables to system.site.yml.
+   */
+  public function testMenuTranslation() {

Also 90%+ of the patch is DB dump changes. Do we need them? A bunch of new translatable strings, i18n's tests listed, entity_translation's tests, etc. Especially after #3022137: Update migrate fixtures for remaining active issues landed.

Is there an opportunity for an 8.9 version similar to 3112249?

Attached is a backported 8.9.2 version.

@quietone Sure thing.

FYI my patch is pretty barebones and excludes both the tests and 3112249 work. I will try to upload something more useable once I test it further.

Also, I didn't mean to remove the "Needs reroll" tag (maybe it was automatic?).

First quick review. Will do a second one.

1. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php
   @@ -67,12 +76,48 @@ public function fields() {
   +          ->fields('ml')
   ...
   +        $source_mlid = $query->execute()->fetchField(1);
   

   Can we only select the field "mlid" instead of all fields. Can we then call the method fetchField() without any parameters.

2. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php
   @@ -67,12 +76,48 @@ public function fields() {
   +    if ($row->hasSourceProperty('i18n_tsid')) {
   +      if ($row->getSourceProperty('i18n_tsid') != 0) {
   

   Can we change this to using a single if-statement?

3. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php
   @@ -67,12 +76,48 @@ public function fields() {
   +        $default_language = $this->variableGet('language_default', (object) ['language' => 'en']);
   +        $default_language = $default_language->language;
   +        $row->setSourceProperty('language', $default_language);
   

   Why set the default language to "en" instead of "und"?

4. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php
   @@ -80,6 +125,29 @@ public function prepareRow(Row $row) {
   +      $query = $this->select('i18n_string', 'i18n')->fields('i18n');
   +      $query->condition('i18n.textgroup', 'menu');
   +      $query->condition('i18n.type', 'item');
   +      $query->condition('i18n.objectid', $mlid);
   +      $results = $query->execute()->fetchAll();
   

   Can we rewrite this query to:

         $results = $this->select('i18n_string', 'i18n')
           ->fields('i18n')
           ->condition('textgroup', 'menu')
           ->condition('type', 'item')
           ->condition('objectid', $mlid)
           ->execute()
           ->fetchAll();
   

5. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d7/MenuLinkLocalized.php
   @@ -0,0 +1,75 @@
   +      ->fields('ml')
   ...
   +    $source_mlid = $query->execute()->fetchField(1);
   

   Same as with MenuLink.

6. +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d7/MigrateMenuLinkLocalizedTest.php
   @@ -0,0 +1,97 @@
   +  protected function assertEntity($id, $langcode, $title, $menu, $description, $enabled, $expanded, array $attributes, $uri, $weight) {
   
   +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d7/MigrateMenuLinkTest.php
   @@ -80,11 +82,12 @@ protected function setUp(): void {
   +  protected function assertEntity($id, $langcode, $title, $menu, $description, $enabled, $expanded, array $attributes, $uri, $weight) {
   
   +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d7/MigrateMenuLinkTranslationTest.php
   @@ -0,0 +1,93 @@
   +  protected function assertEntity($id, $langcode, $title, $menu, $description, $enabled, $expanded, array $attributes, $uri, $weight) {
   

   As far as I can see we now have 3 times exactly the same method. Can we put the method in a trait or any other solution which result in having a single definition of the method.

7. +++ b/core/modules/menu_link_content/tests/src/Kernel/Plugin/migrate/source/MenuLinkTest.php
   @@ -54,7 +54,9 @@ public function providerSource() {
   -        'description' => '',
   
   @@ -83,7 +85,9 @@ public function providerSource() {
   -        'description' => '',
   
   @@ -112,7 +116,9 @@ public function providerSource() {
   -        'description' => '',
   
   @@ -140,7 +146,9 @@ public function providerSource() {
   -        'description' => 'Test menu link 1',
   
   @@ -168,7 +176,9 @@ public function providerSource() {
   -        'description' => 'Test menu link 2',
   
   @@ -196,7 +206,40 @@ public function providerSource() {
   -        'description' => '',
   

   Why is the description value being removed?

8. +++ b/core/modules/menu_link_content/tests/src/Kernel/Plugin/migrate/source/MenuLinkTest.php
   @@ -217,7 +260,16 @@ public function providerSource() {
   +    $tests[0]['expected_data'][] = $expected[0];
   +    $tests[0]['expected_data'][] = $expected[1];
   +    $tests[0]['expected_data'][] = $expected[2];
   +    $tests[0]['expected_data'][] = $expected[6];
   +    $tests[0]['expected_data'][] = $expected[3];
   +    $tests[0]['expected_data'][] = $expected[4];
   

   I am missing the value $expected[5]. Can you explain why? Maybe adding adding a comment to the patch with the explanation.

9. +++ b/core/modules/menu_link_content/tests/src/Kernel/Plugin/migrate/source/d7/MenuLinkLocalizedTest.php
   @@ -0,0 +1,222 @@
   +        'language' => 'unk',
   ...
   +        'language' => 'unk',
   

   Should it not be 'language' => 'und'?

I have tried to cover some of points from #82 comment, please review

@quitetone yeah, i have addressed point 1 to 5..
6 to 9 is pending.
I forgot to mention addressed points numbers.

Second review:

1. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php
   @@ -80,6 +123,30 @@ public function prepareRow(Row $row) {
   +      $results = $this->select('i18n_string', 'i18n')->fields('i18n')
   

   Please put the part ->fields('i18n') on a separate line.

2. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php
   @@ -80,6 +123,30 @@ public function prepareRow(Row $row) {
   +        ->condition('i18n.textgroup', 'menu')
   +        ->condition('i18n.type', 'item')
   +        ->condition('i18n.objectid', $mlid)
   

   The query is a single table query. The part "i18n." can be removed from each line.

3. +++ b/core/modules/content_translation/migrations/d7_menu_links_localized.yml
   @@ -0,0 +1,46 @@
   +  description: options/attributes/title
   

   Why has the "description" this value? Other yml files don not have such a value.

4. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php
   @@ -33,6 +33,15 @@ public function query() {
   +    if ($schema->fieldExists('menu_links', 'i18n_tsid')) {
   +      $query->addField('ml', 'i18n_tsid');
   +    }
   

   Why is this code added? All fields from the menu_links are already added to the query result.

5. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d7/MenuLinkLocalized.php
   @@ -0,0 +1,75 @@
   +    $query->orderBy('ml.mlid');
   

   The order by for 'ml.mlid' is already added in the paretn query.

6. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php
   @@ -67,12 +76,46 @@ public function fields() {
   +    if ($row->hasSourceProperty('ml_language')) {
   +      if (($row->getSourceProperty('ml_language') == 'und') && $this->hasTranslation($row->getSourceProperty('mlid'))) {
   

   Can we change this to a single if-statement. If not, then explain why not.

7. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d7/MenuLinkLocalized.php
   @@ -0,0 +1,75 @@
   +  /**
   +   * The join options between the node and the node_revisions table.
   +   */
   +  const OPERATOR = '<>';
   

   Why is this constant added. It is not being used!

8. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d7/MenuLinkTranslation.php
   @@ -0,0 +1,99 @@
   +  /**
   +   * Drupal 7 table name.
   +   */
   +  const I18N_STRING_TABLE = 'i18n_string';
   

   Why is this constant added. It is not being used!

1. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php
   @@ -67,12 +67,47 @@ public function fields() {
   +      $query = $this->select('menu_links', 'ml')
   +        ->fields('ml', ['mlid'])
   +        ->condition('i18n_tsid', $row->getSourceProperty('i18n_tsid'))
   +        ->orderBy('mlid')
   +        ->range(0, 1);
   +      $source_mlid = $query->execute()->fetchField();
   

   We can improve a little bit more by:

   +      $source_mlid = $this->select('menu_links', 'ml')
   +        ->fields('ml', ['mlid'])
   +        ->condition('i18n_tsid', $row->getSourceProperty('i18n_tsid'))
   +        ->orderBy('mlid')
   +        ->range(0, 1)
   +        ->execute()
   +        ->fetchField();
   

2. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d7/MenuLinkTranslation.php
   @@ -0,0 +1,88 @@
   +  use I18nQueryTrait;
   ...
   +  const I18N_STRING_TABLE = 'i18n_string';
   

   The I18nQueryTrait does not use the constant I18N_STRING_TABLE.

3. There are a number of coding standard violations that need to be fixed.

1. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d7/MenuLinkLocalized.php
   @@ -0,0 +1,69 @@
   +    $query = $this->select('menu_links', 'ml')
   +      ->fields('ml', ['mlid'])
   +      ->condition('i18n_tsid', $row->getSourceProperty('i18n_tsid'))
   +      ->orderBy('mlid')
   +      ->range(0, 1);
   +    $source_mlid = $query->execute()->fetchField();
   

   Here we can also make it a little more better:

   +    $source_mlid = $this->select('menu_links', 'ml')
   +      ->fields('ml', ['mlid'])
   +      ->condition('i18n_tsid', $row->getSourceProperty('i18n_tsid'))
   +      ->orderBy('mlid')
   +      ->range(0, 1);
   +      ->execute()
   +      ->fetchField();
   

Here I have addressed comments #92.1, #92.3 and #93

@ravi.shankar: Thank you for the updated patch!

Only the comment #92.2 is still open.

In the trait core/modules/menu_link_content/src/Plugin/migrate/source/d7/MenuLinkTranslation.php we have the variable $i18nStringTable. Why do we then create a new constant called I18N_STRING_TABLE? Can we not use the variable from the trait and remove the constant I18N_STRING_TABLE. If you do not like to do that, because in other migration code it is done the same way, then please say so. If @quietone does not want to change the constant as described above, then the it is RTBC for me.

It is for me now RTBC.

Back to RTBC.

As far as I can see this is a straight reroll. Back to RTBC.

Looks great, thanks! I am moving this to backport, but I would not hold my breath on that honestly. While there is potentially 2 more bugfix releases left of 9.0 that this could get into, #3110669: Migrate d7 menu language content settings needs to land in 9.0 first and then #3112249: Migrate d7 menu translation for this to land.

This introduced a spelling mistake into core - we automate spellchecking. Here's a patch to fix this.

This commit https://git.drupalcode.org/project/drupal/commit/ce767b8 in #107 above has caused the new coding standards message

1 coding standards message
core/modules/content_translation/migrations/d7_menu_links_localized.yml
line 45	Expected 1 newline at end of file; 2 found

It first appeared 15 Oct 2020 at 04:07 BST see https://www.drupal.org/pift-ci-job/1851506

This is such a minor fix, can this be done here? or does it need a separate issue?

I have raised #3178338: Fix coding standard fail committed to core 9.1 and 9.2 to fix the coding standard fault reported above.

9.1.0 was released nine days ago. I don't think backporting is still relevant?

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.