Existing menu links show validation issues on migration (and ALL menu links pointing to node translations are invalid)

Just have time for to comment about views.

views are not migrated

That is correct. More information can be found at Known issues when upgrading from Drupal 6 or 7 to Drupal 8.

I had to solve the taxonomy term related errors for my team.

Long story short: this happens because the d7_menu_link migration only depends on the d7_node migration (and only optionally). But nothing guarantees that your taxonomy terms are also migrated before menu links.

Also see #2975461-2: Convert query string to array for d6 menu_link migration.

The attached patch basically repeats the same pattern that I added at #3122649-12: [PP-2] Derive path alias migrations per entity type (and bundle).

Instead of having a single menu link migration (that Ideally should be executed only after every other, content-entity related migrations), I create standalone derivatives for nodes (per bundle), for terms (per vocabulary) and for users, and one more for everything else. And later, after that the migrations are collected and the migration plugin alter hooks are invoked, I collect every corresponding content entity migrations per entity type (and bundle) and add them to the related derived menu link migration in menu_link_content_migration_plugins_alter().

Looking great! 😊

+++ b/core/modules/menu_link_content/src/Plugin/migrate/source/D7MenuLinkDeriverTrait.php
@@ -0,0 +1,193 @@
+      ->condition('ml.menu_name', 'shortcut-set-%', 'NOT LIKE');

Ah and this means that this patch already includes #3152943-5: Remove migration of shortcuts from menu_link migration 🤓

With the derived menu link migrations, it's still possible that the parent menu link (B) of the menu link 'A' is in an another migration.

To solve this properly, we should be able to create stub menu links.

The patch attached here depends on #3156083-2: Route migrate process plugin shouldn't assume that the $options variable is always an array.

+++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
@@ -131,6 +157,40 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+    if (empty($lookup_result) || empty($links)) {

🤓 🤔 I'm surprised this if condition is necessary, because if we've reached this point then we already know for a fact that the MenuLinkContent entity could not be loaded, nor could $links be non-empty.

So … can we just remove this condition? If we can't, could you add a comment? 🙏

Other than that detail, this is a wonderfully simple interdiff! 😄

Hi @Wim Leers
Updated patch please review.

And below are un-used statements I removed that.
-use Drupal\Component\Plugin\Exception\PluginNotFoundException;
-use Drupal\migrate\MigrateStub;

Forget to upload interter-diff.

Hi @Wim Leers
Fixed failed test cases, please review.

Last patch failed to apply re-rolling for 9.1.x , kindly review.

Solving failed test cases, kindly review.

#14 says that the patch depended on another issue that hasn't been committed, but I don't quite see how. The tests pass, and the code from the other issue isn't included in this patch. I'm trying to see if this issue should be postponed on that one. If it really requires the other issue to work correctly, yet is still passing tests, then it sounds like it might need more tests? @hazooka?

Kicking this back to NW for a response to #24

I suspect it's thanks to #2845485: Refactor and document the MenuLinkParent process plugin that this may no longer be hard-blocked on #3156083: Route migrate process plugin shouldn't assume that the $options variable is always an array, specifically the menu link is route item test case that was added there.

But I'd like @huzooka to confirm.

Frankly, I am 90% confident that this just needs additional test coverage, for the case where the menu parent is a route with options not being an array.

Reviewing #17 (and the interdiff at #18):

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
   @@ -158,14 +158,17 @@
   -    $menu_link_migrations = $this->migrationPluginManager->createInstances($this->migration->getBaseId());
   +    $menu_link_migrations = $this->migrationPluginManager
   +    ->createInstances($this->migration->getBaseId());
   

   😅 Unfortunately badly reformatted code… This violates the coding standards.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
   @@ -158,14 +158,17 @@
   +    if (!empty($menu_link_migrations)) {
   

   👎 This change should not be necessary, the foreach won't do anything if the array is empty.

   EDIT: oh I see, this is to make tests "pass", but the newly created stub in the unit test just doesn't stub the ::createInstances() call. This !empty() test is precisely why the tests appear to be passing whereas in reality they do not.

3. +++ b/core/modules/migrate/tests/src/Unit/process/MenuLinkParentTest.php
   @@ -58,6 +74,8 @@ protected function setUp(): void {
   +    $this->migrationPluginManager = $this->prophesize(MigrationPluginManagerInterface::class);
   +    $this->migrateStub = $this->prophesize(MigrateStubInterface::class);
   

   Nice additions!

4. +++ b/core/modules/migrate/tests/src/Unit/process/MenuLinkParentTest.php
   @@ -88,8 +106,12 @@ public function testTransformExternal() {
   -    $this->menuLinkManager->createInstance('menu_link_content:fe151460-dfa2-4133-8864-c1746f28ab27')->willReturn($plugin->reveal());
   -    $plugin = new MenuLinkParent([], 'map', [], $this->migrateLookup->reveal(), $this->menuLinkManager->reveal(), $this->menuLinkStorage->reveal(), $this->migration->reveal());
   +    $this->menuLinkManager->createInstance('menu_link_content:fe151460-dfa2
   +    -4133-8864-c1746f28ab27')->willReturn($plugin->reveal());
   +    $plugin = new MenuLinkParent([], 'map', [], $this->migrateLookup->reveal(),
   +    $this->menuLinkManager->reveal(), $this->menuLinkStorage->reveal(), $this
   +    ->migration->reveal(), $this->migrationPluginManager->reveal(), $this
   +    ->migrateStub->reveal());
   

   Again badly formatted code…

Rebasing #14, because #14 does not apply to 9.1.x anymore, and the work in #17 and later unfortunately cannot be trusted — see my review in #28, and specifically point 2… 😢

Re-reviewing #14 too:

+++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
@@ -103,7 +129,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
       return '';

@@ -131,6 +157,40 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+        $default_values = [
+          'menu_name' => $menu_name,
+        ];

This seems to be a bug in @huzooka's #14 patch.

And resurrecting the correct test coverage changes made in #17 through #23, while adding todos to explain what needs to happen to make them pass.

@Deepak Goyal and @Hardik_Patel_12 — would you like to continue here? 🤓

NW for #16 and #29.

This patch also "fixes" the migration of menu links which are pointing to a node translation on the source Drupal 7 instance.

#14 still should be addressed.

I don't really see what actually is the issue highlighted in #29.

Tests are failing because we really need #3156083: Route migrate process plugin shouldn't assume that the $options variable is always an array. Without that fix, we cannot "stub" not-yet-migrated parent menu link content entities.

Queued a test. Updated issue title. The discovery in #32 IMHO makes this a major bug.

I don't really see what actually is the issue highlighted in #29.

$menu_name is an undefined variable!

1. +++ b/core/modules/content_translation/migrations/node_translation_menu_links.yml
   @@ -31,6 +32,27 @@ process:
   +  # If the path is of the form "node/<ID>" and is not routed, we will get
   +  # back an URI of the form "base:node/<ID>". If it is a valid node route,
   +  # this will return "entity:node/<ID>".
   +  source_nid:
   

   Wow this is one hell of a process pipeline.

   The issue that originally introduced this, #2912353: Handle menu_items related to Drupal 6 and 7 node translations with different IDs, even had this comment from @phenaproxima:

   This process pipeline is complex and opaque. I think we should add a big comment here to extensively document what is expected to happen at every stage of this transformation.

   The comments added here do help for sure. 👍

2. +++ b/core/modules/content_translation/migrations/node_translation_menu_links.yml
   @@ -53,9 +75,9 @@ process:
   -      # Skip row if node ID is empty.
   +      # Skip the process if node ID is empty.
          plugin: skip_on_empty
   -      method: row
   +      method: process
   

   Why?

3. +++ b/core/modules/menu_link_content/migrations/d6_menu_links.yml
   @@ -51,7 +54,6 @@ process:
   -  no_stub: true
   

   🤔 Why is this being removed here and in many other places?

#33.3: @huzooka just explained to me that we need the parent menu link items to be able to get stubbed, hence no_stub: true getting removed. 👍

Support derived migration of localized and also for translated menu links.

+++ b/core/modules/content_translation/migrations/d7_menu_links_localized.yml
@@ -4,6 +4,7 @@ migration_tags:
   - Multilingual
+deriver: Drupal\content_translation\Plugin\migrate\source\D7MenuLinkDeriver
 source:

+++ b/core/modules/content_translation/migrations/d7_menu_links_translation.yml
@@ -4,6 +4,7 @@ migration_tags:
   - Multilingual
+deriver: Drupal\content_translation\Plugin\migrate\source\D7MenuLinkDeriver
 source:

This should be Drupal\content_translation\Plugin\migrate\D7MenuLinkDeriver.

+++ b/core/modules/menu_link_content/src/Plugin/migrate/source/D7MenuLinkDeriver.php
@@ -189,7 +205,159 @@ class D7MenuLinkDeriver extends DeriverBase implements ContainerDeriverInterface
+        // The non-derived migration depends on "d*_menu_links" and
+        // "d*_node_translation" migrations, but also performs a migration
+        // lookup in the corresponding "d*_node_complete" migrations.
+        // Let's finalize the migration dependencies for first.
+        unset($derivative_definition['migration_dependencies']);
+        $node_migration_id_to_depend_on = $node_migration_base_id_to_depend_on . PluginBase::DERIVATIVE_SEPARATOR . $bundle;
+        $derivative_definition['migration_dependencies'] = [
+          'required' => [
+            // The migration should depend on the corresponding node migration.
+            $node_migration_id_to_depend_on,
+          ],
+        ];

This drops the d7_menu_links dependency! And that's okay, because it now allows stubbing.

This is good, because otherwise it is impossible to know which derivative of d7_menu_links this should depend on — it would have to depend on all of them. Stubbing is indeed the solution.

But that does mean that the indirect dependency on d7_menu is lost, which is why I created #3189476: If d7_menu_links depends on d7_menu, then node_translation_menu_links should too. This brings back that same lost dependency.

The queries generated here by the

+++ b/core/modules/menu_link_content/src/Plugin/migrate/source/D7MenuLinkDeriverTrait.php
@@ -0,0 +1,193 @@
+  protected function getBaseQuery(Connection $database) {
+    $options = ['fetch' => \PDO::FETCH_ASSOC];
+    $query = $database->select('menu_links', 'ml', $options)
+      ->fields('ml')
+      ->condition('ml.menu_name', 'shortcut-set-%', 'NOT LIKE');
+    $and = $query->andConditionGroup()
+      ->condition('ml.module', 'menu')
+      ->condition('ml.router_path', ['admin/build/menu-customize/%', 'admin/structure/menu/manage/%'], 'NOT IN');
+    $condition = $query->orConditionGroup()
+      ->condition('ml.customized', 1)
+      ->condition($and);
+    $query->condition($condition);
+    $query->leftJoin('menu_links', 'pl', 'ml.plid = pl.mlid');
+    $query->addField('pl', 'link_path', 'parent_link_path');
+    $query->orderBy('ml.depth');
+    $query->orderby('ml.mlid');
+
+    return $query;
+  }

+  /**
+   * Adds entity type specific exclude conditions a menu link query.
+   *
+   * @param \Drupal\Core\Database\Query\SelectInterface $query
+   *   A Select Query object we add the restrictions to.
+   * @param string[] $excluded_entity_type_ids
+   *   An array of the IDs of the entity types that should be excluded.
+   */
+  protected function addExcludedEntityTypeRestrictions(SelectInterface $query, array $excluded_entity_type_ids) {
+    foreach ($excluded_entity_type_ids as $entity_type_id) {
+      if (empty($link_templates = $this->getPreparedLinkTemplates($entity_type_id))) {
+        continue;
+      }
+
+      foreach ($link_templates as $link_template) {
+        $pattern = '/(' . $this->entityTypeParameter . '|' . $this->extraParameter . ')/';
+        $template_raw = preg_replace($pattern, '%', $query->escapeLike($link_template));
+        $query->condition('ml.router_path', $template_raw, '<>');
+      }
+    }
+  }
    

The resulting queries for this on a concrete site resulted in this query:

SET SESSION SQL_MODE=ANSI_QUOTES;

EXPLAIN
SELECT "ml".*, "pl"."link_path" AS "parent_link_path", "n"."type" AS "type", "nt"."name" AS "name"
FROM
"menu_links" "ml"
LEFT OUTER JOIN "menu_links" "pl" ON ml.plid = pl.mlid
INNER JOIN "node" "n" ON CONCAT('node/', n.nid, '') = ml.link_path OR CONCAT('node/', n.nid, '/delete') = ml.link_path OR CONCAT('node/', n.nid, '/edit') = ml.link_path OR CONCAT('node/', n.nid, '/revisions') = ml.link_path OR CONCAT('node/', n.nid, '/revisions/%/view') LIKE ml.link_path OR CONCAT('node/', n.nid, '/translations') = ml.link_path OR CONCAT('node/', n.nid, '/translations/add/%/%') LIKE ml.link_path OR CONCAT('node/', n.nid, '/translations/edit/%') LIKE ml.link_path OR CONCAT('node/', n.nid, '/translations/delete/%') LIKE ml.link_path
INNER JOIN "node_type" "nt" ON nt.type = n.type
WHERE ("ml"."menu_name" NOT LIKE 'shortcut-set-%' ESCAPE '\\') AND (("ml"."customized" = '1') OR (("ml"."module" = 'menu') AND ("ml"."router_path" NOT IN ('admin/build/menu-customize/%', 'admin/structure/menu/manage/%')))) AND (("ml"."link_path" LIKE 'node/%' ESCAPE '\\') OR ("ml"."link_path" LIKE 'node/%/delete' ESCAPE '\\') OR ("ml"."link_path" LIKE 'node/%/edit' ESCAPE '\\') OR ("ml"."link_path" LIKE 'node/%/revisions' ESCAPE '\\') OR ("ml"."link_path" LIKE 'node/%/revisions/%/view' ESCAPE '\\') OR ("ml"."link_path" LIKE 'node/%/translations' ESCAPE '\\') OR ("ml"."link_path" LIKE 'node/%/translations/add/%/%' ESCAPE '\\') OR ("ml"."link_path" LIKE 'node/%/translations/edit/%' ESCAPE '\\') OR ("ml"."link_path" LIKE 'node/%/translations/delete/%' ESCAPE '\\'))
ORDER BY "ml"."depth" ASC, "ml"."mlid" ASC;

which took 127 seconds to return 3604 rows…

Turns out that this patch never got the optimizations that I added to #3122649: [PP-2] Derive path alias migrations per entity type (and bundle) in #3122649-15: [PP-2] Derive path alias migrations per entity type (and bundle) through #3122649-32: [PP-2] Derive path alias migrations per entity type (and bundle).

I am still thoroughly testing this, but wanted to post my WIP interdiff already.

For #37.

In the end, the #39 WIP interdiff has become the final interdiff 👍

+++ b/core/modules/menu_link_content/src/Plugin/migrate/source/D7MenuLinkDeriver.php
@@ -97,47 +97,61 @@ public function getDerivativeDefinitions($base_plugin_definition) {
+            default:
+              // @todo logging
+              $x = 0;
+              break;

This is ugly, but a pre-existing missing capability.

Patch at #42 should be ignored.

#3189880: Use square brackets syntax in sql queries and #3207968: Replace @codingStandards comments with phpcs: comments broke this between 9.1.7 and 9.2.0-rc1. Fortunately, #44 already fixed it after #3189880 :) Thanks, @nikitagupta!

Huh, looks like #44 actually made many other changes in D7MenuLinkDeriverTrait for undocumented reasons, this caused many problems 😬😔

The issue summary here only has a description of the problem. Let's use the template and complete all the sections.

I skimmed the patch and see that there is a new deriver, why is a deriver necessary to fix a validation issue?

I presume it is know that tests are needed. The deriver will need test. And there are no changes to the existing MigrateMenuLinkTests which I would have expected. Am I wrong on that?

@nikitagupta, when adding a patch always add an interdiff and an explanation of the changes. That will help to avoid the problems faced in #49 where the undocumented changes 'caused many problems'. There are instructions for creating an interdiff in the handbook.

Because of the reason @huzooka gave in #5:

I had to solve the taxonomy term related errors for my team.

Long story short: this happens because the d7_menu_link migration only depends on the d7_node migration (and only optionally). But nothing guarantees that your taxonomy terms are also migrated before menu links.

So the solution is given in #7:

Instead of having a single menu link migration (that Ideally should be executed only after every other, content-entity related migrations), I create standalone derivatives for nodes (per bundle), for terms (per vocabulary) and for users, and one more for everything else.

The menu links no longer fail to import due to validation errors because the content they're pointing to has not yet been migrated :)

there are no changes to the existing MigrateMenuLinkTests which I would have expected. Am I wrong on that?

It is passing only because it contains hardcoded knowledge about the exact order in which migrations should be executed. Quoting \Drupal\Tests\menu_link_content\Kernel\Migrate\d7\MigrateMenuLinkTest::setUp():

    $this->executeMigrations([
      'language',
      'd7_language_content_settings',
      'd7_node',
      'd7_node_translation',
      'd7_menu',
      'd7_menu_links',
      'node_translation_menu_links',
    ]);

Who knows this? How can one know this? The answer is simple: this cannot be known. The solution: add the necessary migration dependencies.

But this then presents the next problem: maybe you want to focus on the migration of taxonomy terms first, but also the migration of menus targeting taxonomy terms. But blanket statements such as "d7_menu_links optionally depend on d7_node, d7_taxonomy_term, d7_node_translation, …" then fail to be helpful: you can only run the migration reliably if you've first migrated literally every content entity over, otherwise your menu links migration might fail!

That last paragraph is the answer to:

why is a deriver necessary to fix a validation issue?

Not to mention: you may want to migrate nodes, but not taxonomy terms, or vice versa. If that's the case, then the current all-or-nothing d7_menu_links migration is guaranteed to generate a lot of validation errors.

I'm wondering what the relationship is between this issue and #3253959: Menu links migration should run after terms are migrated

From comment #5 above:

I think the right fix for this issue is to add d6_taxonomy_term and d7_taxonomy_term as optional dependencies for the menu link migrations.

This is exactly what the new issue is about. Are these duplicates? Or is there something more in this issue that the related issue misses?

Setting to NR to get thoughts on whether or not these are duplicates.

It does look like this is working to solve the same problem as #3253959: Menu links migration should run after terms are migrated, where the dependency on the term migration is solved.

The issue title states that the 'ALL menu links pointing to node translations are invalid'. That should have been improved by #3008028: Migrate D7 i18n menu links, committed in Oct 2020, as it sets the correct language on the menu link. There is no information provided about why validation failed so we can't confirm if that change is sufficient.

The solution here, to add the dependency using derivers, is not needed in core migrate.

I am tempted to close this as works as designed but first I want to double check the menu links for translated nodes. Setting to NW while I do that.

Rerolled #52 to 9.3.x

#56 fixed my d7_menu_links migration error, but

drush migrate:import --tag="Drupal 7"

now failing on nearly identical d7_menu_links_other error messages.

drush migrate:messages upgrade_d7_menu_links_other shows:

1 515 upgrade_d7_menu_links_other:link/uri: The path "internal:/some/views/url" failed validation.

("some/views/url" my own above)

Thanks to Wim Leers for pointing out that the issue I ran into (#3253959) was a symptom of this larger issue.

Removed extra node_translation_menu_links.yml.orig file from patch and added https://www.drupal.org/node/3064016 code in D7MenuLinkDeriverTrait.php.
Again rerolled #52 to 9.3.x

Manually differed #48 with #59, and they're indeed identical except for two changes:

1. the *.orig file (which already was in #48 😅) is omitted 👍
2. it's updated to account for the changes that #3064016: Allow menu_link source plugin to filter menu links by menu name(s) introduced 👍

This is test coverage that quietone and danflanagan8 added to #3253959, so attribution should go to them.

Who knows this? How can one know this? The answer is simple: this cannot be known. The solution: add the necessary migration dependencies.

Agreed on fixing dependencies. We've played loose and fast in the past by bypassing validation and using stubs with the idea that at the end of the core suite of migrations everything will fall into place. Migrating menus linking to term ids before the terms are created is fine because at the end of the process the terms will exist and everything is good. As core hardens security and error checking we need to keep track of things where there are actual validation errors in the process and make sure we are adding dependencies as needed.

But this then presents the next problem: maybe you want to focus on the migration of taxonomy terms first, but also the migration of menus targeting taxonomy terms. But blanket statements such as "d7_menu_links optionally depend on d7_node, d7_taxonomy_term, d7_node_translation, …" then fail to be helpful: you can only run the migration reliably if you've first migrated literally every content entity over, otherwise your menu links migration might fail!

I've said this before, but the migrations provided by core are designed to be run together in the order specified by core and all at once. If you want to migrate taxonomy terms followed by taxonomy menus, that is a custom migration that would require a custom menu migration, and we provide the tools for you to write that.

Not to mention: you may want to migrate nodes, but not taxonomy terms, or vice versa. If that's the case, then the current all-or-nothing d7_menu_links migration is guaranteed to generate a lot of validation errors.

If you want to migrate nodes but not taxonomy or vice versa, you are also running custom migrations and not the core suite of migrations, and again you would need to customise your menu migration as well. This derivative falls into the same pattern as the other ones that have been suggested over the past year or two, as far as I can tell. It's not needed in core, but can be added to the contrib modules that export migrations for customization.

Without invoking custom migration flows, can someone give me a reason why we can't just make the menu link migration depend on all the content migrations in core instead of creating derivitives?

NW for cspell

I don't know of a single site that is able to migrate from 7 to 9 by using the migrate_drupal_ui module.

So this:

I've said this before, but the migrations provided by core are designed to be run together in the order specified by core and all at once.

is ignoring the fact that pretty much every site out there needs to execute specific migrations in a specific order.

I'm not ignoring it, what I'm saying is that is the litmus test we use when deciding if something goes into core vs contrib, vs something the core api supports via custom migrations. Changing this would be a much bigger discussion, but we provide the tools and places in contrib to put things that make the custom migrations easier. In addition to adding this derivative to the other ones in migrate_upgrade, there is nothing stopping anyone from putting out a module with a complete set of alternative core migrations designed to be split into lots of smaller derived migrations ready for customization, but It doesn't pass the test for going into core that we've currently adopted.

I'm not trying to be difficult. The needs for custom migrations are wide and varied and need to move faster than we can always support in core, I really think it's better for the migration community to have these tools in contrib where things can move faster and more easily support more customized complex workflows.

Still NW for cspell and passing tests.

If anyone wants not to loose their complex menu link structure during Drupal 7 -> Drupal 9 migration, try this: Migrate Magician Menu Link Migrate

Rerolled patch for 9.4.x.

Rerolling for 9.5.5. (#67 still applied fine to 9.5.2.)

#3152943: Remove migration of shortcuts from menu_link migration conflicted with this in \Drupal\menu_link_content\Plugin\migrate\source\MenuLink. This patch already had the logic that that added in D7MenuLinkDeriverTrait::getBaseQuery().

(See #11.)

The Migrate Drupal Module was approved for removal in #3371229: [Policy] Migrate Drupal and Migrate Drupal UI after Drupal 7 EOL.

This is Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

The deprecation work is in #3522602: [meta] Tasks to remove Migrate Drupal module and the removal work in #3522602: [meta] Tasks to remove Migrate Drupal module.

Migrate Drupal will not be moved to a contributed project. It will be removed from core after the Drupal 12.x branch is open.

just updating tags