Migrate d7 menu language content settings

Yay thanks for doing this one. Do we really need this much test data updates? :)

Generally looks great, thanks! I noticed some leftover things from the base term migration code :) These are all minor.

1. +++ b/core/modules/language/migrations/d7_language_content_menu_settings.yml
   @@ -0,0 +1,54 @@
   +  # State is the value in the i18n_mode column of taxonomy_vocabulary table
   

   Hm, its not the taxonomy_vocabulary table though? menu_custom right?

2. +++ b/core/modules/language/migrations/state/language.migrate_drupal.yml
   @@ -8,6 +8,9 @@ finished:
   +    i18n_manu:
   +      - language
   

   manu?

3. +++ b/core/modules/language/tests/src/Kernel/Migrate/d7/MigrateLanguageContentMenuSettingsTest.php
   @@ -0,0 +1,77 @@
   +    // No multilingual options for terms, i18n_mode = 0.
   

   not terms

Looks great now, thanks!

Here I have made changes as suggested in comment #19.

Re-rtbcing from #18. The requested changes have landed.

Superb, thanks! Next up is #3112249: Migrate d7 menu translation.

Just tried to revert this, but the revert is no longer clean against 9.1.x, so this could use a revert patch.

Spot-checked various points in this patch and it does seem to be rolling back the same things it added indeed.

Meant to RTBC it, sorry.

"Rollback" also means a migrate feature, so retitling for clarity.

I reverted the original 8.9.x commit, then committed #31 to 9.0.x and cherry-picked it forward to 9.1.x.

I guess this is then NW for #26?

It seems #31 didn't apply to 9.1.x and somehow I didn't notice that the cherry-pick failed. Sorry! So we need a different revert patch for that?

Thanks, looks good to me.

Re-roll is green.

Committed/pushed to 9.1.x, thanks!

I think that means we go back to 'needs work' here.

Just on suggestion.

+++ b/core/modules/language/migrations/d7_language_content_menu_settings.yml
@@ -0,0 +1,25 @@
+  target_entity_type_id: constants/target_type
...
+  target_bundle: constants/target_type
+  default_langcode: constants/langcode
...
+  third_party_settings/content_translation/enabled: constants/enabled
+  third_party_settings/content_translation/bundle_settings/untranslatable_fields_hide: constants/hide

Why are not using default value plugin here? tracking down constants is difficult to debug imo.

The patch looks good to me. Only I do have some questions:

1. +++ b/core/modules/language/tests/src/Kernel/Migrate/d7/MigrateLanguageContentMenuSettingsTest.php
   @@ -0,0 +1,59 @@
   +    $target_entity = 'menu_link_content';
   ...
   +    $this->assertSame($target_entity, $config->getTargetEntityTypeId());
   

   Can we just remove the variable $target_entity.

2. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
   @@ -20982,31 +20995,43 @@
   +  'i18n_mode' => '5',
   

   Why do you set the value to '5' in this case, where all other values are set to '0'? When I do a code base search, I find possible values of 0, 1, 2 and 4, but not 5.

3. +++ b/core/modules/language/migrations/d7_language_content_menu_settings.yml
   @@ -0,0 +1,25 @@
   +    target_type: menu_link_content
   +    langcode: site_default
   ...
   +  target_entity_type_id: constants/target_type
   ...
   +  target_bundle: constants/target_type
   

   Is there a standard for adding quoting values. In this yml file and others is it not done and in others it is done. Not sure what to do or are both allowed?

4. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
   @@ -24616,7 +24908,7 @@
   -  'hidden' => '0',
   +  'hidden' => '-1',
   

   Can you explain why this change is needed?

5. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
   @@ -24775,14 +25079,14 @@
   -  'link_title' => 'Configure block',
   +  'link_title' => 'Configure',
   ...
   -  'weight' => '0',
   +  'weight' => '-100',
   

   Can you explain why these changes are needed?

6. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
   @@ -25065,33 +25389,8 @@
   -->values(array(
   -  'menu_name' => 'navigation',
   -  'mlid' => '174',
   -  'plid' => '172',
   -  'link_path' => 'search/user',
   -  'router_path' => 'search/user',
   -  'link_title' => 'Users',
   -  'options' => 'a:0:{}',
   -  'module' => 'system',
   -  'hidden' => '-1',
   -  'external' => '0',
   -  'has_children' => '0',
   -  'expanded' => '0',
   -  'weight' => '0',
   -  'depth' => '2',
   -  'customized' => '0',
   -  'p1' => '172',
   -  'p2' => '174',
   -  'p3' => '0',
   -  'p4' => '0',
   -  'p5' => '0',
   -  'p6' => '0',
   -  'p7' => '0',
   -  'p8' => '0',
   -  'p9' => '0',
   -  'updated' => '0',
   
   @@ -25281,33 +25592,8 @@
   -->values(array(
   -  'menu_name' => 'navigation',
   -  'mlid' => '182',
   -  'plid' => '174',
   -  'link_path' => 'search/user/%',
   -  'router_path' => 'search/user/%',
   -  'link_title' => 'Users',
   -  'options' => 'a:0:{}',
   -  'module' => 'system',
   -  'hidden' => '-1',
   -  'external' => '0',
   -  'has_children' => '0',
   -  'expanded' => '0',
   -  'weight' => '0',
   -  'depth' => '3',
   -  'customized' => '0',
   -  'p1' => '172',
   -  'p2' => '174',
   -  'p3' => '182',
   -  'p4' => '0',
   -  'p5' => '0',
   -  'p6' => '0',
   -  'p7' => '0',
   -  'p8' => '0',
   -  'p9' => '0',
   -  'updated' => '0',
   

   Why is these menu items removed?

7. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
   @@ -31491,13 +32233,15 @@
   -  'router_path' => 'node/2',
   +  'router_path' => 'node/%',
   

   The same for this change: why is this done?

@quietone: Thank you for the explanations.

All the code changes now look good to me.
It is for me RTBC.

Committed e72c88f and pushed to 9.1.x. Thanks!

The commit in #70 introduced a regression - it uses public static $modules in the test instead of protected static $modules.

It's a quick fix - can this be fixed here, or should I open a new issue?

@TR: I think that it is better to open a new issue and link it to this issue as a followup.

Mmm, OK, I posted the fixes in #3164721: More uses of public static $modules. Maybe you can review that @daffie?

Leaving this for RM review on whether we can back-port to 9.0

Should the status be "Patch (to be ported)"?

I am changing the target branch to 9.0.x.

Thank you, @quietone! To recap for the RMs: This patch contains a new migration for the menu language content settings, a bunch of tests, and updates to the fixture. While not technically a bug fix, and would ordinarily not be eligible for a backport to 9.0, the new migration enhances support for D7 migrations, which we've traditionally considered backports for as its good for the ecosystem to make it as easy as possible to get people off of D7 into modern Drupal. Also, the fixture changes are needed to backport #3112249: Migrate d7 menu translation, which contains another needed D7 i18n migration, which is being considered for backport. None of this is runtime code, so from that perspective, it is safe to backport, but this and the other issue will help make it easier for people to run migrations from D7 to D9.0 right now, so I'm in favor of the backport to 9.0 and 8.9, but leaving it to @xjm and @catch to approve or deny. If this is approved and committed, We would then put the other issue up for backport as well, so a up/down on both would be great.

Originally this was committed to 9.0 (When that was the active dev branch) and backported to 8.9 (again while that was still in -dev). It had to be reverted due to a regression that has been fixed, so we need a new determination from the RMs as to whether it still qualifies for backport and how far back it should be backported.

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

Yes moving this back to 'fixed' on 9.1.x

Then let's also remove "[backport]" from the title.