Migrate D7 Menu Links

I have written a D7 menu links plugin based on (and extending) the D6 version.

Rather than converting all links to simply 'internal:/' . $path it also handle <front> and all node menu links (because the menu_ui module filters by uri starting with 'entity:node/' when displaying Menu Settings on the node edit page).

Checked in D8.1.1.

Separate D6/7 migration templates. Add D7 menu migration template mapping "main-menu" to "menu". Remove redundant source plugin.

Corrected test fails.

• Updated d7_shortcut migration template to be dependent on new d7_menu_links template (rather than menu_links).
• Added new d7_menu and d7_menu_links migration templates to $moduleUpgradePaths.
• Changed expected entity count for menu (from 10 to 6) now that 4 of the D7 menus are mapped to core D8 menus.

Corrections for test failures - in tests, replaced old migration executions (menu, menu_links) with new (d7_menu, d7_menu_links).

Looks really good.

1. +++ b/core/modules/menu_link_content/migration_templates/d7_menu_links.yml
   @@ -0,0 +1,54 @@
   +    -
   +      plugin: static_map
   +      map:
   +        management: admin
   +      bypass: true
   

   Is this necessary for D7? I'm not even sure what it's for in D6.

2. +++ b/core/modules/menu_link_content/src/Plugin/migrate/process/d7/InternalUri.php
   @@ -0,0 +1,45 @@
   +      $path_components = array_pad(explode('/', $path, 2), 2, NULL);
   +      // If $path is the node page (i.e. node/[nid]) then return entity path.
   +      if (($path_components[0] == 'node') && (is_numeric($path_components[1]))) {
   

   Please document why this is needed for nodes, and not other types.

1. I agree, I see no need in mapping menu_names in d7_menu_links, that will be done in d7_menu - code removed.
2. As stated in the newly added comment, "entity: URI"s enable the menu link to appear in the Menu Settings section on the node edit page. Other entities (e.g. taxonomy terms, users) do not have the Menu Settings section.

Sorry, I meant "please document in the *code* why this is needed for nodes and not other types". Your explanation makes sense, but it's not at all obvious from reading the code. I don't want someone to accidentally remove it later without realizing its importance.

Extended comment, as requested.

Re-roll.

I think this patch is going to need very extensive test coverage. Menu migration is quite tricky on a good day, and we need to be certain these migrations can handle anything that might be thrown at them.

1. +++ b/core/modules/menu_link_content/migration_templates/d7_menu_links.yml
   index b6bcdaf..1447e82 100644
   --- a/core/modules/menu_link_content/migration_templates/menu_links.yml
   
   --- a/core/modules/menu_link_content/migration_templates/menu_links.yml
   +++ b/core/modules/menu_link_content/migration_templates/menu_links.yml
   
   +++ b/core/modules/menu_link_content/migration_templates/menu_links.yml
   +++ b/core/modules/menu_link_content/migration_templates/menu_links.yml
   @@ -2,7 +2,6 @@ id: menu_links
   
   @@ -2,7 +2,6 @@ id: menu_links
    label: Menu links
    migration_tags:
      - Drupal 6
   -  - Drupal 7
   

   If we're untagging this migration as being Drupal 7-applicable, we need to rename menu_links.yml to d6_menu_links so it's not ambiguous.

2. +++ b/core/modules/menu_link_content/src/Plugin/migrate/process/d7/InternalUri.php
   @@ -0,0 +1,48 @@
   +/**
   + * @file
   + * Contains \Drupal\menu_link_content\Plugin\migrate\process\d7\InternalUri.
   + */
   

   This is no longer needed.

3. +++ b/core/modules/menu_link_content/src/Plugin/migrate/process/d7/InternalUri.php
   @@ -0,0 +1,48 @@
   +      $path_components = array_pad(explode('/', $path, 2), 2, NULL);
   +      // If $path is the node page (i.e. node/[nid]) then return entity path.
   +      if (($path_components[0] == 'node') && (is_numeric($path_components[1]))) {
   

   This seems complex. Can't we use a regex?

4. +++ b/core/modules/system/migration_templates/d7_menu.yml
   @@ -0,0 +1,20 @@
   +  plugin: entity:menu
   

   entity:menu needs to be single-quoted.

5. +++ b/core/modules/system/migration_templates/d7_menu.yml
   index 974a2d6..683beef 100644
   --- a/core/modules/system/migration_templates/menu.yml
   
   --- a/core/modules/system/migration_templates/menu.yml
   +++ b/core/modules/system/migration_templates/menu.yml
   
   +++ b/core/modules/system/migration_templates/menu.yml
   +++ b/core/modules/system/migration_templates/menu.yml
   @@ -3,7 +3,6 @@ id: menu
   
   @@ -3,7 +3,6 @@ id: menu
    label: Menus
    migration_tags:
      - Drupal 6
   -  - Drupal 7
   

   Again, I think we'll have to separate these migrations into d6_menu and d7_menu if they are not going to be generic.

1. Renamed menu_links.yml to d6_menu_links.yml.
2. Removed unnecessary comment.
3. Use regex to find node page.
4. I have not single-quoted entity:menu because none of the other 80 instances of plugin: entity:* are single-quoted.
5. Renamed menu.yml to d6_menu.yml.

Will add tests in a later patch.

Ensure the MenuLinkTests are calling the new d6/d7 migrations.

Ensure that the d6 MigrateMenuTest calls the d6_menu migration.

(and improve the readability of the D7 MigrateMenuLinkTest).

Increase the expected number of menu_link_content entities (having added one to the test fixture).

Add D7 MigrateMenuTest.

Add tests for all possibilities for d7_internal_uri process plugin.

Corrected expected test entity counts.

@phenaproxima you recommended very extensive testing. Do you think this requires any further tests adding to it? If so, can you suggest please. Thanks.

#28 needs re-roll.

Your branch is up-to-date with 'origin/8.3.x'.
$ curl https://www.drupal.org/files/issues/migrate_d7_menu_links-2669978-28.patch | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 23522  100 23522    0     0  37264      0 --:--:-- --:--:-- --:--:-- 37218
patching file core/modules/menu_link_content/migration_templates/menu_links.yml
patching file core/modules/menu_link_content/migration_templates/d7_menu_links.yml
patching file core/modules/menu_link_content/src/Plugin/migrate/process/d7/InternalUri.php
patching file core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTest.php
patching file core/modules/menu_link_content/tests/src/Kernel/Migrate/d7/MigrateMenuLinkTest.php
patching file core/modules/migrate_drupal/tests/fixtures/drupal7.php
patching file core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
patching file core/modules/migrate_drupal_ui/src/Tests/d7/MigrateUpgrade7Test.php
Hunk #1 FAILED at 57.
1 out of 1 hunk FAILED -- saving rejects to file core/modules/migrate_drupal_ui/src/Tests/d7/MigrateUpgrade7Test.php.rej
patching file core/modules/shortcut/migration_templates/d7_shortcut.yml
patching file core/modules/shortcut/tests/src/Kernel/Migrate/d7/MigrateShortcutSetTest.php
patching file core/modules/shortcut/tests/src/Kernel/Migrate/d7/MigrateShortcutSetUsersTest.php
patching file core/modules/shortcut/tests/src/Kernel/Migrate/d7/MigrateShortcutTest.php
patching file core/modules/system/migration_templates/menu.yml
patching file core/modules/system/migration_templates/d7_menu.yml
patching file core/modules/system/tests/src/Kernel/Migrate/MigrateMenuTest.php
patching file core/modules/system/tests/src/Kernel/Migrate/d7/MigrateMenuTest.php

Re-roll

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

   Let's add a skip_on_empty between migration and static_map in case the lookup fails.

2. +++ b/core/modules/menu_link_content/migration_templates/d7_menu_links.yml
   @@ -0,0 +1,48 @@
   +  menu_name:
   +    plugin: migration
   +    migration: d7_menu
   +    source: menu_name
   

   And let's do the same here.

3. +++ b/core/modules/menu_link_content/src/Plugin/migrate/process/d7/InternalUri.php
   @@ -0,0 +1,42 @@
   +    if (parse_url($path, PHP_URL_SCHEME) === NULL) {
   

   I'd rather use == here, since any empty value would indicate an internal URI.

4. +++ b/core/modules/menu_link_content/src/Plugin/migrate/process/d7/InternalUri.php
   @@ -0,0 +1,42 @@
   +      if (preg_match('/^node\/\d+$/', $path)) {
   +        // "entity: URI"s enable the menu link to appear in the Menu Settings
   +        // section on the node edit page. Other entities (e.g. taxonomy terms,
   +        // users) do not have the Menu Settings section.
   +        return 'entity:' . $path;
   +      }
   

   We could do a more robust job of detecting entity URIs here (anything matching \w+/\d+ could potentially be one), but I'm not sure if we should do that in this patch or pursue it in a follow-up issue.

5. +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d7/MigrateMenuLinkTest.php
   @@ -74,31 +76,36 @@ protected function assertEntity($id, $title, $menu, $description, $enabled, $exp
   +    $this->assertEquals(2, count($tree));
   

   Let's use assertCount() here.

The amount of testing we have will probably suffice for now.

Changes in response to @phenaproxima's code review:

• Added skip_on_empty to both menu_links migrations.
• Replaced === with ==.
• Used assertCount().

I recommend we create a follow-up issue to do a more robust job of detecting entity URIs - it could take a while and this ticket has taken a long time to get even this far.

Corrected entity counts that caused test failure.

Ugh, I should have mentioned this in my previous review (sorry!) -- but we should probably have a dedicated unit test of InternalUri as well.

Added a unit test of InternalUri.

+++ b/core/modules/menu_link_content/tests/src/Unit/Plugin/migrate/process/d7/InternalUriTest.php
@@ -0,0 +1,73 @@
+   * @inheritDoc

Needs to be {@inheritdoc}, but can be fixed on commit.

RTBC assuming tests pass.

Here's a tip: when RTBCing a patch, remember to RTBC the patch. #PhenIsAnIdiot

That'll teach me for trusting my IDE to auto-complete documentation correctly. PhpStorm--

Due to #39 comment, tagged for novice task. We'll work on that task today.

Fixed the inheritdoc. Iterdiff included.

This is the nit of nits, but it needs be {@inheritdoc}, all lowercase. Probably fixable on commit, though.

The patch file actually contains a correct value (lowercase). Sorry about the faulty interdiff.

A.

This looks great. I think it's going to be hard to improve on this without real world testing, so committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

The weak checking for entity URIs in this issue is potentially very problematic, see #2648996: Optimize menu link migration for more information. We should open a follow-up to align the two URI plugins for D6 and D7.

Doh I meant to ask for that follow-up. It seems better to have entity links going to non entities, than non-entity links going to entities, but we should definitely make that more robust. Opened here: #2822881: Improve Entity URI checking in menu link migration.

Awesome, thanks!

While the issue summary talks about language, the actual issue is migrating menu link and has no language behavior at all. So removing the tags / parent. The issue summary is totally unrelated to what the patch does, but I don't think its valuable to update now.