Migrate D6 i18n menu links

A first attempt. This uses the EntityContentBase from the D6 i18n node issue.

Generally my feedback is that menus themselves have translatability settings as well. i18n supports different translation methodologies and menu items may have their own langcode (and be hidden/shown based on langcode) or be translated in-place like this. It is totally fine scope management to keep this issue for the in-object translation but it would be important to ensure only menus that had this setting take that data. Other menus may have had this setting before on and may have leftover translations in the DB that are not used. Looking at the code, one thing immediately stood out:

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -122,6 +130,20 @@ public function getIds() {
   protected function updateEntity(EntityInterface $entity, Row $row) {
+    if ($entity instanceof TranslatableInterface) {
+
+      $property = $this->storage->getEntityType()->getKey('langcode');
+
+      if ($row->hasDestinationProperty($property)) {
+        $language = $row->getDestinationProperty($property);
+
+        if (!$entity->hasTranslation($language)) {
+          $entity->addTranslation($language);
+        }
+        $entity = $entity->getTranslation($language);
+      }
+    }
+

#2225775: Migrate Drupal 6 core node translation to Drupal 8 just introduced a very similar change to this method, so this may not be needed anymore.

@quietone: are you still working on this?

I plan to resume working on the i18n issues this weekend.

Well, that was a long weekend. Rerolled and moved to menu_link_content.

Added the new migration to MigrateUpgradeForm.

Reroll to use new source test class.

Eek, ignore #14, where I lost the changes and didn't notice.

Yicks. And now fixing the namespace error.

Needed a reroll because of #2669978: Migrate D7 Menu Links.

Misplaced comma.

Hi,

Has any progress been made with this, when importing menu link items the translation is not been selected for me.

Thanks,
Chris

Corrected the dependencies in d6_i18n_menu_links.yml.

Add language to the dependencies in d6_i18n_menu_links.yml (and a couple of minor tweaks).

Assigning to myself for review.

Yes, language needs to be enabled and the other changes are good too. And I don't see any outstanding issues so probably RTBC, although I can't do that.

I spent some time reviewing this issue in the last few days.

If I understand correctly, this is for the use case where you have, on your D6 site, a menu with language neutral links and have their titles & descriptions translated as strings. The menu is then always displayed with the titles & descriptions in the right language.

But what about the case where the menu items have their own langcodes and they are only displayed according to the current language? In this case, the links are duplicated for each language, so there's no need for translating their titles & descriptions as strings.

I tested this second use case manually and the migration throws some exceptions. I think we should try to support all use cases, if it is only possible.

@maxocub: well, the thing is Drupal 8 is not going to hide/show menu items based on language, so while that *data* may be entirely possible to migrate (per menu item language information), the behavior will not be there. Unlike the translation scenario. I think migrating the data for menu item language itself would also be useful but we should not make people expect it works the way it worked with i18n in Drupal 7. Also I think that may not fit in the scope of this one, but possibly in another issue, since its a different problem space.

Taking for review.

Superb, thanks for working on this.

1. +++ b/core/modules/menu_link_content/migration_templates/d6_i18n_menu_links.yml
   @@ -0,0 +1,37 @@
   +      plugin: migration
   +      # The menu migration is in the system module.
   +      migration: menu
   +      source: menu_name
   

   What does this comment try to indicate? Not clear to me.

2. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d6/I18nMenuLink.php
   @@ -0,0 +1,116 @@
   +            if ($result[$i]['mlid'] == $result[$j]['mlid'] and
   

   and => &&

3. +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateI18nMenuLinkTest.php
   @@ -0,0 +1,74 @@
   +    'locale',
   

   Why do we need the locale module?

4. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -318,6 +318,10 @@ class MigrateUpgradeForm extends ConfirmFormBase {
   +    'd6_i18n_menu_links' => [
   +      'source_module' => 'menu',
   +      'destination_module' => 'menu_link_content',
   +    ],
   

   Should source be i18n?

One final thing that dawned on me here is that in #2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields and the other issues we did not ensure that the fields themselves are configured to be translatable. We are not doing it here either. Maybe we need a followup for that and incorporate in our current patches? I mean we are migrating the translation but are users able to edit and use those translations?

1. The menu migration is actually in the system module, which makes it harder to find. The comment was added in #2589805: Add documentation on how to find the menu migrations.
2. Fixed
3. Without explicitly adding locale the Migration test fails with

) Drupal\Tests\menu_link_content\Kernel\Migrate\d6\MigrateI18nMenuLinkTest::testMenuLinks
Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "locale.config_manager".

I haven't looked further than that.
4. Fixed.

Like the i18n user profile issue, these migrations and tests should be moved to config_translation. I didn't do that here.

Like the i18n user profile issue, these migrations and tests should be moved to config_translation. I didn't do that here.

Hm, no, config translation module is a UI module and is not needed for config translation overrides to exist. One may use a different UI module or not expose this UI on the site and still config translations will function equally well. If we need to move this to a "language place", that would be language module then.

+++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateI18nMenuLinkTest.php
@@ -0,0 +1,73 @@
+    'locale',
+    'config_translation',

See #2225781-41: Migrate D6 i18n taxonomy vocabularies also, that may resolve the bug with locale. Remove both locale and config translation from the test, neither module would be needed to test this. Both are UI/integration modules and actual config translation API is in language module.

@Gábor Hojtsy, thanks.

Removed the loading of those two modules in the test.

Looks good to me now for the scope it set out to do :)

1. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d6/I18nMenuLink.php
   @@ -0,0 +1,116 @@
   +    $result = $this->prepareQuery()->execute()->fetchAll();
   

   I'm concerned about how this might scale as this is getting all the data at once. I also think this might be incompatible with the batching approach in \Drupal\migrate\Plugin\migrate\source\SqlBase() which this inherits from. Not sure.

2. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d6/I18nMenuLink.php
   @@ -0,0 +1,116 @@
   +    return array(
   

   Array syntax - this has now been standardised on the short syntax in all of core...

Made changes as per the comment #39, Applying the patch, please review.

@Pavan B S, thanks for the updated patch. One small change to make sure that result is the results of the query.

The patch helps.

Setting back to "Needs work" to address @alexpott's point in #39 about fetchAll().

It may be worth noting that a similar method is also used in core/modules/migrate_drupal/src/Plugin/migrate/source/Variable.php and modules/migrate_drupal/src/Plugin/migrate/source/d6/i18nVariable.php. Either we bite the bullet and accept this as is or whatever solution we come up with here should also be applied there.

EDIT: It is also used in a similar manner in:

• core/modules/field/src/Plugin/migrate/source/d6/FieldInstancePerFormDisplay.php
• core/modules/field/src/Plugin/migrate/source/d6/FieldInstancePerViewMode.php
• core/modules/field/src/Plugin/migrate/source/d7/FieldInstancePerViewMode.php
• core/modules/field/src/Plugin/migrate/source/d7/ViewMode.php
• core/modules/node/src/Plugin/migrate/source/d6/ViewMode.php

i.e.

$result = $this->prepareQuery()->execute();
while ($row = $result->fetchAssoc()) {
  ...
}

This also needs renaming. The migrations be in the form basicmigration_variation. The same pattern should be used for the related files like migration plugins and tests. See #26 and #27

Let's start with the rename, and adding a missing colon, before addressing any other issues.

Here is a way to address the concerns #39. The source query makes rows with one property (title or description) per row. This is how the i18n_strings tables stores information, by property, so it makes the queries simpler. Then in prepareRow get the translation information for the other property as well as the untranslated values for each property.

There is also a process plugin that simply selects the translation of the untranslated version of the property. Could have done that in prepareRow but it seemed better to have the yml show that those values were being manipulated.

This causes menu links that have both a title and description translation to be migrated twice. Here is a way to prevent that.

Gábor Hojtsy asked about adding the language content settings to these i18n migration (#2225681: Migrate D6 i18n blocks translated strings) and it makes sense to do it here. I could be wrong.

Still I've added the migration of the language content settings for language, and the tests. And, the UI shows the custom menu links and the three fields are ticked as translatable.

Not sure why MigrateUpgrade6Test is failing. But there is time to think about because postponing this until the source plugin test is fixed. #2862006: MigrateSourceTestBase returns false positives for most plugin tests.

No longer blocked.

Fixed the class name on MenuLinkTranslationTest.

Yes, there is another language_content_settings, so increment the entity count.

Bumping priority while triaging the queue in the weekly migrate meeting. Getting this done is blocking completion of d6 migration path.

Reroll.

And a reminder that the migration needs to be renamed, See #2861383: Rename i18n migrations to _translation.

And a nevermind to the reminder, this has already been renamed.

I'll try to fix the test

+++ b/core/modules/language/migration_templates/d6_language_content_menu_settings.yml
@@ -0,0 +1,17 @@
+  third_party_settings/content_translation/enabled: enabled

I think the test is failing because of this line. If I remove it, it passes, at least locally.

What is the purpose of this third_party_settings here?

Needed a reroll

Removed the process plugin a MenuLinkTranslation.php and moved the logic to the process pipeline. This was suggested by phenaproxima in #2909444: Migrate D6 i18n custom blocks (boxes).

And the patch

Added source_module to source plugins. Two changes to d6_menu_links_translation.yml, change the name of the migration plugin to migration_lookup and the other to give it a valid input, 'd6_menu' instead of 'menu'.

The i18 submodules don't have an underscore in their name.

Changed variable name property2 to other_property in the MenuLinkTranslation source plugin.
Removed 'third party settings' from the migration. As maxocub pointed out in #65 removing this allows the test to pass. I compared this migration to d6_language_content_menu_settings, which is probably where I started this patch way back when. Well, that migration uses a Drupal 6 variable from the i18n modules, language_content_type_. Reading the D6 code I only see that being used in reference to nodes so it isn't needed here and is safe to remove.

Oops, forgot to remove reference to third party settings in the test.

Trying the patch from #79 I am getting this error:

Migration failed with source plugin exception: Extra unknown items in source IDs                                                                    [error]
Processed 0 items (0 created, 0 updated, 0 failed, 0 ignored) - done with 'ao2_it_d6_menu_links_translation'                                        [status]

The fact that there are 0 items to be processed makes sense because I have distinct per-language menus and not translated menus, but I still would not expect the error.

The solution for me is to just remove the migration since I don't need it, but I thought I'd report the issue anyway.

Thanks for reporting this, @ao2! Can you post the exact steps you used (or close to it) to get this error? Anything we can do to improve this patch, and its test coverage, is hugely appreciated.

Here's an improved error log/exception to help us know what is causing the issue. @ao2, can you re-test please?

With the new patch this message gets printed.

array(1) {
  ["language"]=>
  NULL
}
Migration failed with source plugin exception: Extra unknown items in source IDs:                                                                   [error]

As I said I have two distinct menus, one per language, that means that the properties from both are in i18n_strings but they are not correlated, so the left joins end up giving results with language and translation set to NULL. I tried to execute the query from core/modules/menu_link_content/src/Plugin/migrate/source/d6/MenuLinkTranslation.php manually and this seems to confirm the analysis.

I can post the resulting rows if necessary, or execute another query if you suggest one.

Maybe it's enough to add a condition to filter rows with NULL language? Or a different type of join could be used.

This quick and dirty change makes the error go away:

diff -pruN core/modules/menu_link_content/src/Plugin/migrate/source/d6/MenuLinkTranslation.php.orig core/modules/menu_link_content/src/Plugin/migrate/source/d6/MenuLinkTranslation.php
--- core/modules/menu_link_content/src/Plugin/migrate/source/d6/MenuLinkTranslation.php.orig	2017-11-27 23:11:56.247378796 +0100
+++ core/modules/menu_link_content/src/Plugin/migrate/source/d6/MenuLinkTranslation.php	2017-11-27 23:13:05.066904781 +0100
@@ -54,6 +54,7 @@ class MenuLinkTranslation extends Drupal
 
     // Add in the translation for the property.
     $query->leftJoin('locales_target', 'lt', 'i18n.lid = lt.lid');
+    $query->isNotNull('lt.language');
     $query->addField('lt', 'language');
     $query->addField('lt', 'translation');
     return $query;

Ciao,
Antonio

@heddn Was this more like what you intended? Or have I got the wrong end of the stick (and should keep out of this!)?

@Jo Fitzgerald that's more like it!

This is blocking #2918295: Move i18n query to a trait

The last patch from #85 fixes the error message but not the problem reported in #80.

As suggested in #84 a filter could be added or an INNER JOIN can be used here as well.

Thanks,
Antonio

--- /dev/null
+++ b/core/modules/language/migration_templates/d6_language_content_menu_settings.yml

This needs to move to migrations

Plus need to respond to #88

1. Moved migrations from migration_templates.
2. Handled problem from #88 (and #84 & #80):
   lookupDestinationIds() was not running unset() on array elements with a NULL value (in this example language) so added an array_key_exists() condition.

@ao2 Would you mind retesting with this patch, please?

I'm wondering if the changes to Drupal\migrate\Plugin\migrate\id_map\Sql (and associated tests) should be moved to a follow-up issue - it's beyond the scope of this ticket.

Looks like this needs a reroll...

Also, the last patch against 8.5.x appears to have failed testing. :(

Can anyone enlighten me on the test failures - I don't understand.

Correct the test failures.

Correct the test failures.

1. +++ b/core/modules/language/src/Plugin/migrate/source/d6/LanguageContentMenuSettings.php
   @@ -0,0 +1,60 @@
   + * The translation of menu links is enabled if the i18n_menu module is
   

   s/i18n_menu/i18nmenu

2. +++ b/core/modules/language/src/Plugin/migrate/source/d6/LanguageContentMenuSettings.php
   @@ -0,0 +1,60 @@
   +      ->fields('s', ['name', 'type', 'status'])
   

   I don't see the 'type' used anywhere, I don't think we need it.

3. +++ b/core/modules/language/src/Plugin/migrate/source/d6/LanguageContentMenuSettings.php
   @@ -0,0 +1,60 @@
   +    $fields = [
   +      'name' => $this->t('Name'),
   +    ];
   

   'name' is not the only field this plugin returns. We should document the other fields like status and/or language_alterable.

4. +++ b/core/modules/language/src/Plugin/migrate/source/d6/LanguageContentMenuSettings.php
   @@ -0,0 +1,60 @@
   +    $value = empty($row->getSourceProperty('status')) ? FALSE : TRUE;
   +    $row->setSourceProperty('language_alterable', $value);
   

   Could we not just return 'status' from the query? Is this prepareRow() really necessary?

5. +++ b/core/modules/language/src/Plugin/migrate/source/d6/LanguageContentMenuSettings.php
   @@ -0,0 +1,60 @@
   +    $row->setSourceProperty('enabled', $value);
   

   I don't see the 'enabled' property used anywhere, do we really need it?

6. +++ b/core/modules/menu_link_content/migrations/d6_menu_links_translation.yml
   @@ -0,0 +1,54 @@
   +  skip:
   +    - plugin: skip_on_empty
   

   There should be a new line after the '-'.

7. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d6/MenuLinkTranslation.php
   @@ -0,0 +1,116 @@
   +    // with the translations for with both the title and description or just the
   

   Extra 'with'

8. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d6/MenuLinkTranslation.php
   @@ -0,0 +1,116 @@
   +    // Therefore, build a query based on 18n_strings table where each row has
   

   s/i18n_strings/i18nstrings

9. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d6/MenuLinkTranslation.php
   @@ -0,0 +1,116 @@
   +    $query = $this->select('i18n_strings', 'i18n')
   +      ->fields('i18n', ['lid']);
   +    $query
   +      ->condition('i18n.property', $other_property)
   +      ->condition('i18n.objectid', $mlid);
   

   This could be chained.

10. +++ b/core/modules/menu_link_content/tests/src/Kernel/Plugin/migrate/source/d6/MenuLinkTranslationTest.php
    @@ -0,0 +1,256 @@
    +        'u18n_status' => 0,
    ...
    +        'u18n_status' => 0,
    ...
    +        'u18n_status' => 0,
    ...
    +        'u18n_status' => 0,
    

    s/u18n/i18n

11. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -562,9 +562,11 @@ public function lookupDestinationIds(array $source_id_values) {
    -        // Associative $source_id_values can have fields out of order.
    -        if (isset($source_id_values[$field_name])) {
    -          $conditions[$db_field] = $source_id_values[$field_name];
    +        if (array_key_exists($field_name, $source_id_values)) {
    +          // Associative $source_id_values can have fields out of order.
    +          if (isset($source_id_values[$field_name])) {
    +            $conditions[$db_field] = $source_id_values[$field_name];
    +          }
    
    +++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php
    @@ -517,6 +517,8 @@ public function testLookupDestinationIds() {
    +    $this->assertEquals([[101, 'en'], [101, 'fr'], [101, 'de']], $id_map->lookupDestinationIds(['nid' => 1, 'language' => NULL]));
    +    $this->assertEquals([[102, 'en']], $id_map->lookupDestinationIds(['nid' => 2, 'language' => NULL]));
    

    I removed the change in Sql and ran the MigrateSqlIdMapTest, and it still passed. I think we will need a failing patch to show that this change fix something. I would also agree that this could have it's own issue, but I don't want to slow this patch, so let's leave it here for now.

@maxocub, thanks for the review!

1 Fixed
2. I left 'type' since it is a condition on the query. I'd rather be sure that we only get the status of a module and not something else with the same name (yes, very very unlikely).
3. fields() updated
4. yes, just using status now
5, 6, 7, 8, 9, 10. Fixed
11. I did the same thing, removed the change in Sql.php and the test failed. I've made a fail patch and interdiff-101-fail.txt should show what I did.

interdiff 2225587-101-fail.patch 2225587-101.patch  > interdiff-101-fail.txt

Now, lets see if the patch passes tests.

Thanks @quietone for the failing patch, I don't know why it passes on my machine, but I think this failing patch shows that it works.

The code looks good now, except for an unused use statement in LanguageContentMenuSettings source plugin (Row is not used anymore).

But sadly, I'm still experiencing similar failures as @ao2 mentioned in #84.

The patch works very well if you have only one menu with items configured to be displayed for all languages and with their strings translated with the interface translation tool. But if like @ao2 you have one menu per language, or like what I tried, one menu with different items for each languages, then there's integrity constraint violattions:

SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'langcode' cannot be null: UPDATE @menu_link_content SET id=:db_update_placeholder_0, bundle=:db_update_placeholder_1, uuid=:db_update_placeholder_2, langcode=:db_update_placeholder_3 WHERE id = :db_condition_placeholder_0; Array ( [:db_update_placeholder_0] => 129 [:db_update_placeholder_1] => menu_link_content [:db_update_placeholder_2] => 832662fb-f294-4d29-97a1-8ad74eed3843 [:db_update_placeholder_3] => [:db_condition_placeholder_0] => 129 )

I think @ao2 is right about the inner join. The problem is that with his or my scenario, the strings are not in the locale_target table and the left join is getting untranslated menu items, and thus no langcode. We should definitely add tests for those cases. I think an easy way to reproduce this in a test would be to have an untranslated menu item, that is a menu item that is in the i18n_strings table but not in the locale_target table.

Hope to work on this this weekend.

@maxocub, thanks for the analysis, it makes more sense to me now.

I have added a translated link that does have an entry in i18n_strings but not in locales_target to the secondary links menu. The tests for translated menu links includes the new link. I also added tests to MigrateMenuLinkTest because it does, in fact, migrate these translated links. There is a fail patch too which uses the left join.

Updated the entity counts and extended the source plugin form MenuLink instead of DrupalSqlBase.

Thank you @quietone, I'm going to review & test this tomorrow at work, where I have a Drupal 6 installation.

The patch looks good but TBH, I don't quite grok it...

1. +++ b/core/modules/language/migrations/d6_language_content_menu_settings.yml
   @@ -0,0 +1,16 @@
   +id: d6_language_content_menu_settings
   +label: Drupal 6 language content settings
   

   The ID and label seem a bit mismatched.

2. +++ b/core/modules/language/src/Plugin/migrate/source/d6/LanguageContentMenuSettings.php
   @@ -0,0 +1,52 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function query() {
   +    $query = $this->select('system', 's')
   +      ->fields('s', ['name', 'type', 'status'])
   +      ->condition('name', 'i18nmenu')
   +      ->condition('type', 'module');
   +    return $query;
   +  }
   

   Is this needed? Won't Migrate Drupal filter this source (and its migration) out if i18nmenu is not enabled in the source?

@phenaproxima is right, I don't think we need the LanguageContentMenuSettings source plugin. We could use the Empty source plugin and move the 'status' into a constant, and this migration won't be detected if i18nmenu is not enabled on the source site. So this is one source plugin and one test we can get rid of.

Also, there's a few coding standards messages:

core/modules/language/src/Plugin/migrate/source/d6/LanguageContentMenuSettings.php
line 5	Unused use statement

core/modules/menu_link_content/src/Plugin/migrate/source/d6/MenuLinkTranslation.php
line 7	There must be a single space after the USE keyword

core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTest.php
line 39	Expected 1 space before "="; 0 found
line 86	Expected 1 space before "="; 0 found

core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTranslationTest.php
line 77	Expected 1 space before "="; 0 found

Unassigning

Hope to get to this on the weekend.

Here are fixes for #110.1 and all of #111.

For #110.2, I tried maxocub's suggestion of using the empty source plugin instead. That results in the 'source_module' not found error. I've attached a fail patch for that.

Unassigning

That's weird, the empty source plugin can't be use anymore? Isn't this a bug? The source_module is clearly provided. I'll take a look at this.

I don't see any uses of plugin: empty in core right now. And it doesn't seem that we merge in any of the plugin config/options when we call the source plugin manager's getDefinition(). So, this could be that we need to merge that in and we have us a legit feature/bug in the source_module checking logic.

Working on a problem with the empty source plugin isn't in scope here, in my opinion. Maybe part of #2937045: source plugin source_module testing seems incomplete?

Also, are you sure that using the empty source plugin will not prevent the migration from being discovered as suggested in #111. The logic to ignore a plugin if the source database doesn't have the source_module enabled is in DrupalSqlBase::checkRequirements and that doesn't look like it is called when using the empty source plugin.

We discussed this in the Migrate maintainer call and decided that, in the name of landing this damn patch, we can use the Embedded source plugin for now (which does define a source_module), rather than switch to the Empty source and block on #2937045: source plugin source_module testing seems incomplete. So let's do that; hopefully it works and we can get this in.

Oh fudge, I didn't document that I tried the embedded source plugin. That is still discovered even when i18n menu is not enabled on the source.

One idea is t to create a @internal Final subclass of embedded w/ annotation of source_module 'i18n_menu'.

Here's why we need to extend DrupalSqlBase! We need it so we can check if a module is enabled on the source d6/d7 database. There is no easy way to do that without extending that class. Because the info we need from d6 is if 'i18n_menu' is enabled. And if it is, then it will get discovered and we'll be happy again.

Solved.

This is shaping up wonderfully but I think we still need some tweaking, sadly.

1. +++ b/core/modules/language/migrations/d6_language_content_menu_settings.yml
   @@ -0,0 +1,16 @@
   +  target_bundle: 'constants/target_type'
   +  target_entity_type_id: 'constants/target_type'
   

   This was jarring when I first looked at it, but it made sense once I realized that menu_link_content is a content entity type without any bundle support (although it does have a 'bundle' key, for some reason). We should add a comment here explaining that, so that others don't stumble upon this and have a WTF moment.

2. +++ b/core/modules/language/migrations/d6_language_content_menu_settings.yml
   @@ -0,0 +1,16 @@
   +  default_langcode: 'constants/site_default'
   

   This looks like the wrong constant. Shouldn't it be 'constants/langcode'? If so, the tests apparently didn't catch this, so we should probably add an assertion or two somewhere. :)

   Although, looking in the ContentLanguageSettings entity, it appears that the default_langcode is set to site_default anyway. So maybe we can remove this line completely.

3. +++ b/core/modules/language/src/Plugin/migrate/source/d6/LanguageContentMenuSettings.php
   @@ -0,0 +1,51 @@
   +class LanguageContentMenuSettings extends DrupalSqlBase {
   

   I think we could/should make this a generic plugin (for both Drupal 6 and Drupal 7) which returns the system data of a module/theme (or several modules/themes) in the source database, rather than hard-coding i81nmenu and the name/type.

   We could call this plugin something like LegacyExtension, and it could key by name, type, and schema version.

4. +++ b/core/modules/language/tests/src/Kernel/Migrate/d6/MigrateLanguageContentMenuSettingsTest.php
   @@ -0,0 +1,49 @@
   +    $this->assertTrue($config instanceof ContentLanguageSettings);
   

   This should be assertInstanceOf().

5. +++ b/core/modules/language/tests/src/Kernel/Migrate/d6/MigrateLanguageContentMenuSettingsTest.php
   @@ -0,0 +1,49 @@
   +    $this->assertSame('site_default', $config->getDefaultLangcode());
   

   Let's use LanguageInterface::LANGCODE_SITE_DEFAULT instead of hardcoding 'site_default'.

6. +++ b/core/modules/language/tests/src/Kernel/Migrate/d6/MigrateLanguageContentMenuSettingsTest.php
   @@ -0,0 +1,49 @@
   +    $this->assertTrue($config->isLanguageAlterable());
   

   Can we add a comment explaining that this will only be true if i18nmenu is enabled? And can we add a test that proves it's false if i18nmenu is disabled in the source DB? (No need to alter the fixture; just run a query against the source database before executing the migration.)

7. +++ b/core/modules/menu_link_content/migrations/d6_menu_links_translation.yml
   @@ -0,0 +1,54 @@
   +  menu_name:
   +    -
   +      plugin: migration_lookup
   +      # The menu migration is in the system module.
   +      migration: d6_menu
   +      source: menu_name
   +    -
   +      plugin: static_map
   +      map:
   +        management: admin
   +      bypass: true
   

   Do we want to skip the row if migration_lookup returns no result?

8. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d6/MenuLinkTranslation.php
   @@ -0,0 +1,98 @@
   +    if (empty($this->idMap->lookupDestinationIds(['mlid' => $mlid, 'language' => $language]))) {
   +      $row->setSourceProperty('migrate', TRUE);
   +    }
   

   Do we want to bail out if the lookup fails?

9. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d6/MenuLinkTranslation.php
   @@ -0,0 +1,98 @@
   +    $row->setSourceProperty('description_untranslated', Unicode::truncate($row->getSourceProperty('options/attributes/title'), 255));
   

   I'm not sure we should be enforcing truncation here. The migration should probably do it in the process pipeline, if possible.

10. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d6/MenuLinkTranslation.php
    @@ -0,0 +1,98 @@
    +    $query = $this->select('i18n_strings', 'i18n')
    +      ->fields('i18n', ['lid'])
    +      ->condition('i18n.property', $other_property)
    +      ->condition('i18n.objectid', $mlid);
    +    $query->leftJoin('locales_target', 'lt', 'i18n.lid = lt.lid');
    +    $query->condition('lt.language', $language);
    +    $query->addField('lt', 'translation');
    

    This should probably be a helper method which works on the query object. We could call it from here and in query().

11. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d6/MenuLinkTranslation.php
    @@ -0,0 +1,98 @@
    +    $row->setSourceProperty($other_property . '_translated', $results['translation']);
    

    We need to handle the case where the query returns no result. Maybe just throw a MigrateException or something?

12. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d6/MenuLinkTranslation.php
    @@ -0,0 +1,98 @@
    +      'title' => $this->t('Menu link title translation.'),
    +      'description' => $this->t('Menu link description translation.'),
    

    Aren't these called title_translated and title_untranslated? (Plus description_translated and description_untranslated, respectively.)

13. +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTest.php
    @@ -70,6 +76,23 @@ public function testMenuLinks() {
    +    $menu_link = MenuLinkContent::load(459);
    

    Let's add an assertInstanceOf() to check that $menu_link is, in fact, an instance of MenuLinkContent. That way, if it isn't, the test will be able to fail gracefully instead of choking on a fatal error.

14. +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTest.php
    @@ -70,6 +76,23 @@ public function testMenuLinks() {
    +    $this->assertSame(TRUE, $menu_link->isEnabled());
    +    $this->assertSame(FALSE, $menu_link->isExpanded());
    

    Should be assertTrue() and assertFalse(), respectively.

15. +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTranslationTest.php
    @@ -0,0 +1,89 @@
    +    $menu_link = MenuLinkContent::load(139)->getTranslation('fr');
    

    We should assert that $menu_link is an instance of MenuLinkContent (assertInstanceOf()) before calling methods on it.

16. +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTranslationTest.php
    @@ -0,0 +1,89 @@
    +    $this->assertTrue(TRUE);
    

    What is this for?

17. +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTranslationTest.php
    @@ -0,0 +1,89 @@
    +    $this->assertSame(TRUE, $menu_link->isEnabled());
    +    $this->assertSame(TRUE, $menu_link->isExpanded());
    

    Should use assertTrue().

18. +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTranslationTest.php
    @@ -0,0 +1,89 @@
    +    $menu_link = MenuLinkContent::load(139)->getTranslation('zu');
    

    Should assert that $menu_link is an instance of MenuLinkContent.

19. +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTranslationTest.php
    @@ -0,0 +1,89 @@
    +    $this->assertSame(TRUE, $menu_link->isEnabled());
    +    $this->assertSame(TRUE, $menu_link->isExpanded());
    

    Same here.

20. +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTranslationTest.php
    @@ -0,0 +1,89 @@
    +    $menu_link = MenuLinkContent::load(140)->getTranslation('fr');
    

    Need to assert that $menu_link is an instance of MenuLinkContent.

21. +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTranslationTest.php
    @@ -0,0 +1,89 @@
    +    $this->assertSame(TRUE, $menu_link->isEnabled());
    +    $this->assertSame(FALSE, $menu_link->isExpanded());
    

    Should use assertTrue() and assertFalse() here.

22. +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTranslationTest.php
    @@ -0,0 +1,89 @@
    +    $menu_link = MenuLinkContent::load(459);
    

    assertInstanceOf() missing :)

23. +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTranslationTest.php
    @@ -0,0 +1,89 @@
    +    $this->assertSame(TRUE, $menu_link->isEnabled());
    +    $this->assertSame(FALSE, $menu_link->isExpanded());
    

    Should use assertTrue() and assertFalse().

24. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -562,9 +562,11 @@ public function lookupDestinationIds(array $source_id_values) {
    +        if (array_key_exists($field_name, $source_id_values)) {
    +          // Associative $source_id_values can have fields out of order.
    +          if (isset($source_id_values[$field_name])) {
    

    Why was this change made? Surely isset() should have both bases covered?

25. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
    @@ -579,7 +581,8 @@ public function lookupDestinationIds(array $source_id_values) {
    +      $var_dump = var_export($source_id_values, TRUE);
    +      throw new MigrateException(sprintf("Extra unknown items in source IDs: %s", $var_dump));
    

    NICE! Better error reporting is such a wonderful thing. IMHO, this should be its own exception (which would allow intrepid debuggers to inspect the extra source items, rather than just a dump of them), but that can be done in a minor follow-up issue.

Still plenty to do.

1. Comment added
2. fixed the constant. Not sure we should remove this process from the pipeline.
3. Good idea. New plugin, LegacyExtension added in system module. Should it return all possible fields or just the 4 given in the plugin?
4. Fixed
5. Fixed.
6. Comment added but in the migration yml file instead of the test. I reckon people are more likely to look at the migration than the test.
7-11. Todo
12-15. Fixed.
16. I have no idea! removed.
17-23. Fixed
24. todo
25. Credit to heddn on that. #82

Many of the test failures are not related. Just moving on with the other points in #124.

7. Fixed. Skip added
8. Removed this, not sure why it was added. Easy to add back if needed.
9. Instead of doing the truncation here, this now calls parent:prepareRow, which does the truncation.
10. I don't see how this can be added to the query() method.
11. If the query for the 'other property' returns no result it is still a valid case. It just mean that that 'other property' does not have a translation.
24. The isset keeps the addition of a condition on NULL in the query. It the field is NULL, nothing should be added to the query.

Add 'Configuration' tag to the d6_language_content_menu_settings migration.

The renaming of properties in the MenuLinkTranslation source plugin required changes to the source plugin test. Changed the source_module in the plugin from i18nmenu instead of i18nstrings. Removed unnecessary property from the d6_menu_links_translation.yml.

Curious that that error wasn't in the previous test. Also removed an unused use statement.

Silly mistake tagged it Content when it should be Configuration

The points raised in #124 were fixed and/or commented on in #126 and #127. Note that no change was made for #124.10 because the query in prepareRow isn't in query() because data from two rows from i18n and locales each are needed, one for property A and one for property B. I could only get the working with a union, which failed later since it wasn't joinable. It was also a bit complicated to read. That is why there are two similar queries.

Actually, originally there wasn't an extra query in prepareRow(). That version was kicked back by alexpott in #39 for scalability. That is why one property is retrieved in query() and the other in prepareRow().

This is really close. I could find very few objections, and nothing earth-shaking.

1. +++ b/core/modules/language/migrations/d6_language_content_menu_settings.yml
   @@ -0,0 +1,22 @@
   +  type: module
   

   Modules and themes cannot share names, so I don't think legacy_extension needs to be able to filter by type.

2. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d6/MenuLinkTranslation.php
   @@ -0,0 +1,92 @@
   +  public function getIds() {
   +    $ids['language']['type'] = 'string';
   +    return parent::getIds() + $ids;
   

   Should this also key by mlid?

3. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
   @@ -563,9 +563,13 @@ public function lookupDestinationIds(array $source_id_values) {
   +        if (array_key_exists($field_name, $source_id_values)) {
   +          // Associative $source_id_values can have fields out of order.
   +          if (isset($source_id_values[$field_name])) {
   

   About this, @quietone said:

   The isset keeps the addition of a condition on NULL in the query. It the field is NULL, nothing should be added to the query.

   Given this, I still don't think we need the array_key_exists() check. Just doing isset($source_id_values[$field_name]) oughta do the trick.

4. +++ b/core/modules/system/src/Plugin/migrate/source/LegacyExtension.php
   @@ -0,0 +1,55 @@
   +    $query = $this->select('system', 's')
   +      ->fields('s', ['name', 'type', 'schema_version', 'status']);
   

   Let's just select all fields in system. No need for a subset. And let's add a prepareRow() implementation to unserialize the saved extension info.

5. +++ b/core/modules/system/src/Plugin/migrate/source/LegacyExtension.php
   @@ -0,0 +1,55 @@
   +    $configuration_properties = ['name', 'type', 'schema_version'];
   +    foreach ($configuration_properties as $property) {
   +      if (isset($this->configuration[$property])) {
   +        $query->condition($property, $this->configuration[$property]);
   +      }
   +    }
   +    return $query;
   

   I don't think we should filter by type or schema version. Since extensions can't have name collisions, filtering by type is pointless. And schema_version is an integer, so it's not a very useful filtering criterion unless we also support comparison operators like !=, >, etc. Which we can add in a follow-up if needed.

   'name' is useful, but IMHO it should be an array (or a string that is cast to an array).

6. +++ b/core/modules/system/src/Plugin/migrate/source/LegacyExtension.php
   @@ -0,0 +1,55 @@
   +    $fields = [
   +      'name' => $this->t('Name'),
   +      'type' => $this->t('Type'),
   +      'status' => $this->t('Status'),
   +      'schema_version' => $this->t('Type'),
   +    ];
   +    return $fields;
   

   This will need to be changed.

7. +++ b/core/modules/system/src/Plugin/migrate/source/LegacyExtension.php
   @@ -0,0 +1,55 @@
   +  public function getIds() {
   +    $ids['name']['type'] = 'string';
   +    $ids['type']['type'] = 'string';
   +    return $ids;
   +  }
   

   I'd say we only need to key by name.

1. Fixed
2. parent::getIds() returns mlid, so it is included.
3. I don't seem to be explaining this well. I have made the change suggested but in a fail patch.
4. Fixed
5. Fixed
6. Fixed
7. Fixed

Heh, it tried to test the interdiff. :) Back to NR.

I think this looks legit. Raise your seat backs and lock your tray tables in preparation for landing.

This needs a re-roll.

Here's a re-roll.

So the error is:
Using the "Legacy" prefix to mark all tests of a class as legacy is deprecated since version 3.3 and will be removed in 4.0. Use the "@group legacy" notation instead to add the test to the legacy group.
which is really unfortunate. This is not a legacy test, it is a test of a source plugin accessing the system table of the Drupal 6 or Drupal 7 system.

Renamed LegacyExtension to just Extension, the source plugin from "legacy_extension" to "extension".

Test class renamed incorrectly.

Great, back to RTBC.

It was a testbot hiccup and I reran the tests. All green again.

1. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d6/MenuLinkTranslation.php
   @@ -0,0 +1,92 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function query() {
   +    // Ideally, the query would return rows for each language for each menu link
   +    // with the translations for both the title and description or just the
   +    // title translation or just the description translation. That query quickly
   +    // became complex and would be difficult to maintain.
   +    // Therefore, build a query based on i18nstrings table where each row has
   +    // the translation for only one property, either title or description. The
   +    // method prepareRow() is then used to obtain the translation for the other
   +    // property.
   +    // The query starts with the same query as menu_link.
   +    $query = parent::query();
   +
   +    // Add in the property, which is either title or description.
   +    $query->leftJoin('i18n_strings', 'i18n', 'ml.mlid = i18n.objectid');
   +    $query->isNotNull('i18n.lid');
   +    $query->addField('i18n', 'lid');
   +    $query->addField('i18n', 'property');
   +
   +    // Add in the translation for the property.
   +    $query->innerJoin('locales_target', 'lt', 'i18n.lid = lt.lid');
   +    $query->addField('lt', 'language');
   +    $query->addField('lt', 'translation');
   +    return $query;
   +  }
   +
   

   As far as I can see this is going to return 2 rows for every menu link one for title and one for description. Doesn't this mean we'll do unnecessary processing?

   And yeah it is a shame we have to use prepareRow to do something we advise people against.

2. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d6/MenuLinkTranslation.php
   @@ -0,0 +1,92 @@
   +    // Get the translation, if one exists, for the property not already in the
   +    // row.
   ...
   +    $row->setSourceProperty($other_property . '_translated', $results['translation']);
   

   As far as I can see we have no test case where either the title or description is not translated. In our test cases they always appear to be both translated. Is that always the case - the docs imply its not.

155.1 Yes, there is extra processing. This was pointed out in #48 where adding a skip process prevented the double processing. This was removed in #127 based on comment #124.8, which appears to be a mistake on my part. Setting to NW for that

155.2 No. Menu link 139 has a Zulu description but not a Zulu title. Menu link 140 has a French title and and no description translations. I see test for these cases in MigrateMenuLinkTranslationTest

Added a test in prepareRow to determine if this row has already been migrated and, if so, to skip this row. That will avoid processing the duplicate rows. Moved things a bit in prepareRow so that test is done as early as possible.

If this passes tests then both points in #155 are addressed and this will be ready for review.

I think #155 has been addressed with satisfaction, let's try to RTBC this again.

I'm not a huge fan of adding queries in ::prepareRow() I think it goes against good practices especially when we are querying the same tables as in ::query() but it is tricky to see how to do this differently - to many axes - language and translated property.

Credited people who reviewed and influenced the patch.

Committed and pushed 66480d7f40 to 8.6.x and 45fcc489b4 to 8.5.x. Thanks!

Backported to 8.5.x as this is a migration and migrate_drupal is still in beta.