Menu link migrations should be defensive

migrate_upgrade has nothing specific to menu links, so I'm sure the root of that problem is in the core menu link migration support. Moving to the core issue queue.

The manifests are no longer used, I've now removed them, thanks for pointing that out.

To customize the migrations to import, run the migrate-upgrade command with --configure-only which creates all the migrations from the templates - then you can use migrate-import to selectively run migrations.

In terms of the d6_menu_links problem, if you could narrow it down by running only d6_menu and d6_menu_links, and maybe provide info on the links it's attempting to migrate, that would help.

Thanks.

We have been using this patch locally at APQC for about a month to solve this issue. We noticed that children were attempting to be migrated before the parent causing the error since the parent doesn't exist on the destination. By ordering on depth we solved this somewhat but would welcome some more test runs in different environments to see if this does in fact fix this issue.

In light of #7 lets go RTBC with this.

+++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
@@ -80,7 +80,11 @@ public function getTitle() {
+    if (!$link) {
+      throw new \RuntimeException('wtf');
+    }

Um. :)

Also, let's get some test coverage for this.

After discussing with @neclimdul and @lostkangaroo on IRC, here's a patch that may help. My suspicion is that paths are not being migrated in as expected, and certain menu links are therefore imported with invalid paths. It's a shot in the dark, though.

This patch makes d6_menu_links more defensive, validating link paths and skipping rows if they fail.

Forgot a semicolon.

Added patch #12 to our long run dataset lets see what we get.

This should pass the tests. #12 can be considered the fail patch.

Part of the issue here is that the d6_menu_links and d7_menu_links migrations don't bother to validate if the path being migrated exists at all. This here patch makes both migrations more defensive -- it beefs up the internal_uri process plugin so that it does a lookup of the link's path, allowing the row to be skipped if the path doesn't exist.

+++ b/core/modules/menu_link_content/migration_templates/d6_menu_links.yml
@@ -24,9 +24,13 @@ process:
-    plugin: internal_uri
-    source:
-      - link_path
+    -
+      plugin: internal_uri
+      source:
+        - link_path
+    -
+      plugin: skip_on_empty
+      method: row

+++ b/core/modules/menu_link_content/migration_templates/d7_menu_links.yml
@@ -24,9 +24,13 @@ process:
-    plugin: internal_uri
-    source:
+    -
+      plugin: internal_uri
+      source:
       - link_path
+    -
+      plugin: skip_on_empty
+      method: row

+++ b/core/modules/menu_link_content/src/Plugin/migrate/process/d6/InternalUri.php
@@ -18,18 +22,70 @@
-    list($path) = $value;
+    // If the routes have not yet been rebuilt, rebuild them.
+    if (static::$isRouterRebuilt == FALSE) {
+      $this->routerBuilder->rebuild();
+      static::$isRouterRebuilt = TRUE;
+    }
 
+    list($path) = $value;
     if (parse_url($path, PHP_URL_SCHEME) === NULL) {
-      return 'internal:/' . $path;
+      if ($this->pathValidator->getUrlIfValidWithoutAccessCheck($path)) {
+        return 'internal:/' . $path;
+      }
+    }
+    else {
+      return $path;
     }
-    return $path;

I don't like this approach because it leaves no reference for why the row was skipped. This leaves migrators to wallow through code looking for how they can fix their migration.

heh, pifr isn't comming back on that one. requeued in drupalci

what about this?

I don't really have RTBC privileges for this patch, but +1 for RTBC.

I'd RTBC as well but we didn't add any tests...

Why are we using an array of sources instead of just source: link_path...? Odd. Get certainly has $properties = is_string($source) ? array($source) : $source; so it doesn't care a lot.

Removed the array of sources as per #24. Added some tests.

There is a failure in D7, from a pre existing test, related to the link 'admin/content/book'. I suspect that the link is now found to be invalid, after all the book module is not installed. Perhaps, a different custom link should be created and added to the fixture.

1. +++ b/core/modules/menu_link_content/src/Tests/Migrate/d6/MigrateMenuLinkTest.php
   @@ -76,6 +78,27 @@ public function testMenuLinks() {
   +    foreach ($this->migration->getIdMap()->getMessageIterator() as $message_row) {
   +      $count++;
   
   +++ b/core/modules/menu_link_content/src/Tests/Migrate/d7/MigrateMenuLinkTest.php
   @@ -112,6 +112,24 @@ public function testMenuLinks() {
   +    foreach ($this->migration->getIdMap()->getMessageIterator() as $message_row) {
   +      if (preg_match('/The path \"(.*)\" failed validation and was dropped./', $message_row->message, $matches)) {
   

   How come only the D7 version needs the regex when iterating the map table?

2. +++ b/core/modules/menu_link_content/src/Tests/Migrate/d6/MigrateMenuLinkTest.php
   @@ -76,6 +78,27 @@ public function testMenuLinks() {
   \ No newline at end of file
   

   Missing new line.

3. +++ b/core/modules/menu_link_content/src/Tests/Migrate/d7/MigrateMenuLinkTest.php
   @@ -129,5 +147,4 @@ public function testUndefinedLinkTitle() {
   diff --git a/d6_menu_links.yml b/d6_menu_links.yml
   
   +++ b/d6_menu_links.yml
   @@ -0,0 +1,52 @@
   diff --git a/d7_menu_links.yml b/d7_menu_links.yml
   

   I think these were added by accident.

1. The D7 dump contains several invalid links and thus several messages are in the message table while the D6 dump doesn't have any invalid links. An invalid link needs to be inserted in the source. But I changed them to be the same. That way, the next person doesn't have to figure out why they are different. And it wouldn't hurt if the D6 test looped through the message table.
2. Fixed
3. Fixed

Also, changed the source for the D7 customized link test from 'admin/content/link' to 'admin/config/system/site-information'. This ensures the link is valid for testing in d7/MigrateMenuLinkTest.php.

OK, this looks good, i'd mark RTBC but do you think we should get #2648996: Optimize menu link migration in first which is going to conflict and is already RTBC?

Yes, agreed.

Currently waiting on #2648996: Optimize menu link migration.

#2648996: Optimize menu link migration is in so no longer postponed.

Rerolled.

+++ b/core/modules/menu_link_content/migration_templates/menu_links.yml
@@ -21,9 +21,12 @@ process:
-    plugin: link_uri
-    source:
-      - link_path
+    -
+      plugin: link_uri
+      source: link_path
+    -
+      plugin: skip_on_empty
+      method: row

+++ b/core/modules/menu_link_content/src/Plugin/migrate/process/d6/LinkUri.php
@@ -77,6 +110,10 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+      else {
+        $migrate_executable->saveMessage(SafeMarkup::format('The path "@path" failed validation and was dropped.', ['@path' => $value]));
+        return NULL;
+      }

Rather than reaching out to the executable to save a message (and add an extra step to the pipeline), it would be simpler (and consistent with how error handling is meant to work) to just throw a MigrateException for the invalid path.

Now throws MigrateException.

Changed unit test to handle the exceptions.

Ah, I'm all thumbs today.

Let's update the issue summary. I'd like to see the new test added in #42 converted to a kernel test so we can call an actual PathValidator service. Otherwise, we aren't really seeing what types of errors could get thrown from that thing when we use a mock.

Converted the plugin test to a Kernel test. Removed the changes to the test fixture that aren't needed anymore and the tests for map messages since an exception is being thrown. The interdiff is large because of all the deletions.

mlid 478 is failing to migrate because its path (admin/content/book) does not exist in D8. Avoid the complication of involving the book module by simplifying the path of mlid 478 to admin/content which we can guarantee is routed.

I disagree. I think the source data needs to have what Drupal7 really has, not fudge to suit our test. We should keep the 'real' D7 data and prove that link isn't migrated.

The menu link in question (mlid 478) is not part of 'real' D7 data; it was added as part of the testing for #2589237: Menu links parent migration is broken. It's path is not actually relevant, so a minor tweak to retain a test for internal:/abc/xyz links is justifiable.

OK, thanks for that. I should have checked first.

Assuming the 8.3.x/8.4.x tests I started come back clean, I think this is good to go.

The 8.4.x and 8.3.x tests on #48 have failed - and the reason looks patch related.

Migrate_drupal_ui.Drupal\migrate_drupal_ui\Tests\d7\MigrateUpgrade7Test
✓		- setUp
✗	
testMigrateUpgrade
fail: [Other] Line 149 of core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php:
Found 6 menu_link_content entities, expected 7.
Value 7 is equal to value 6.

fail: [Other] Line 170 of core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php:
Migration of a:1:{s:9:"sourceid1";s:3:"479";} to a:0:{} as part of the d7_menu_links migration. The source row status is 3

Just needed a minor change to the fixture because the route node/3 no longer exists. There should be no repercussions to this fixture edit because this entry was specifically added (by me!) as part of #2669978: Migrate D7 Menu Links.

Added 8.4.x test but it looks as though that should pass too. Re-setting to RTBC

1. Sorry this has taken so long to review.

2. +++ b/core/modules/menu_link_content/src/Plugin/migrate/process/LinkUri.php
   @@ -63,6 +83,11 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +      // If the routes have not yet been rebuilt, rebuild them.
   +      if (static::$isRouterRebuilt == FALSE) {
   

   I think we should document why we do this. Also why do we do this? What is happening that requires the route rebuild?

3. +++ b/core/modules/menu_link_content/src/Plugin/migrate/process/LinkUri.php
   @@ -80,6 +105,9 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +        throw new MigrateException(sprintf('The path "%s" failed validation and was dropped.', $path));
   

   Dropped? I'm not sure what that actually means with respect to a migration.

58.2 has been in the code for a while. I think since #17 2 years ago. I'd hazard a guess it isn't needed.
58.3 probably means we loose the data and do not import it. Let's try "The path "%s" failed validation" instead. Just drop the last part, pun intended.

1. Removed $isRouterRebuilt section and consequently all references to that and $routerBuilder.
2. Edited exception message.

@heddn-- for a dreadful pun!

That seems like a long time to have tests queued. 6 hrs? Otherwise, one small nit before tests come back.

+++ b/core/modules/menu_link_content/src/Plugin/migrate/process/LinkUri.php
@@ -4,7 +4,9 @@
+use Drupal\Core\Routing\RouteBuilderInterface;

Unused import.

Thanks, @heddn. I can't believe I missed that one - I'd removed most of the other changes in that file!

Marking RTBC based on #60 passing. #62 isn't any different, except for removal of an unused import.

+++ b/core/modules/menu_link_content/src/Plugin/migrate/process/LinkUri.php
@@ -80,6 +81,9 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+        throw new MigrateException(sprintf('The path "%s" failed validation.', $path));

+++ b/core/modules/menu_link_content/tests/src/Kernel/Plugin/migrate/process/LinkUriTest.php
@@ -0,0 +1,121 @@
+    $message = 'The path "%s" failed validation and was dropped.';

Doesn't look like the test is actually running. Or at least this is not being tested for.

Test fixed!

Assigning to myself to review this week.

I neglected to say how I fixed the test. Method providerTestNotRouted was providing 3 arguments to testNotRouted which was expecting 2 arguments. Method providerTestNotRouted is now providing 2 arguments. And the exception message now reflects what is actually i the process plugin.

The interdiff shows changes to drupal7.php even though I made no changes to the test fixture.

Test changes in #50 and #56 make sense. Interdiff does seem odd, but reviewing the actual patch, appropriate updates have been done to providerTestNotRouted and testNotRouted.

The defensiveness already is in. See #2816857: Process plugin link_uri shouldn't be in d6 namespace. This patch is just about adding more tests and throwing an exception on a bad link. It does that. Looks good to me.

This failed phpcs, could use a quick re-roll for that.

FILE: ..._content/tests/src/Kernel/Plugin/migrate/process/LinkUriTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 107 | ERROR | [x] Return type must not contain variable name
     |       |     "$actual"
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Could also do with an issue summary update to more clearly explain what the current bug is and how it's fixed. I think from reading the patch that while we were validing paths, we were still migrating them even if they fail - now we throw an exception (gets converted to a message, doesn't stop the migation) and discard the link if it fails validation - but the issue summary doesn't actually say this.

Nit fixed. Issue summary updated. I'm optimistic that the comment changes won't break anything. So back to rtbc.

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!