Optimize menu link migration

Working on it.

Those graphs are very interesting!

The problem is that Uri::fromInternalUrl() calls \Drupal::pathValidator()->getUrlIfValidWithoutAccessCheck() which does route matching which does route enhancing which ends up loading the entity of the URL, i.e. node 1 for the internal:/node/1 URI. Thus, with ~2800 menu links this ends up loading ~2800 entities. Additionally painful is that this does not use loadMultiple() but loads the entities one by one.

In contrast Uri::fromEntityUrl() is not very expensive.

In theory we should probably try to optimize Uri::fromInternalUri() in some way, but in any case it is semantically more correct to store the proper entity URIs, so let's do that.

Will look into the issue now.

Needs tests, of course. But here's a first stab. What do you think?

Good point, fixed that.

Now also comes with a unit test. Hopefully some other tests fail, so that we do not need additional integration test coverage. Crossing my fingers...

Ahem.

This looks great, just a couple of comments.

1. +++ /dev/null
   @@ -1,35 +0,0 @@
   -class InternalUri extends ProcessPluginBase {
   

   Removing this would be an api break, we could leave it as deprecated? It could even proxy through to the new plugin?

2. +++ b/core/modules/menu_link_content/src/Plugin/migrate/process/d6/LinkUri.php
   @@ -0,0 +1,98 @@
   +        if (
   +          count($route_parts) === 3 &&
   +          $route_parts[0] === 'entity' &&
   +          $route_parts[2] === 'canonical'
   ...
   +          if (
   +            $this->entityTypeManager->hasDefinition($entity_type_id) &&
   +            isset($route_parameters[$entity_type_id])
   +          ) {
   

   I thought our coding standard had all these on one line?

Thanks for the review!

1. I thought that Migrate was exempt from the whole API stability scheme. I just thought that calling it "InternalUri" if it can produce both "internal:" and also "entity:" URIs doesn't make much sense. So sure, we could leave in a BC layer, or we could just add a "@todo D9 Rename to LinkUri" or something like that which would be a little less maintenance overhead. Don't feel strongly either way.

2. I thought we were allowed to split ifs by now, but not sure and don't feel strongly either. Sure, can be fixed.

Not sure I will be able to work on this any more, so unassigning for now.

Not sure about point 1, guess we name a product manager review?

Will ping alexpott for a product manager review. I'd rather leave the BC in place if this going into 8.0, don't mind removing it in 8.1 or 8.2 though.

Migrate is exempt and this sort of change that does not affect already migrated seems fine to go into any 8.x version to me.

Removing the tag - as this didn't need product manager review - I reviewed it as a framework manager - the actual question was a release management question. I answered because on previous discussions with the release managers (@xjm and @catch).

OK, RTBC for me.

This change does still need a change record. And given it is only an optimisation I think that committing to 8.1.x and 8.2.x is fine. 8.0.x will be history soon.

+++ b/core/modules/menu_link_content/src/Plugin/migrate/process/d6/LinkUri.php
@@ -0,0 +1,98 @@
+          list(, $entity_type_id, ) = $route_parts;

Coding standards says - There should be no white space before a closing ")" - I'm pretty sure the last comma is unnecessary.

Along the lines of alex's review,

+++ b/core/modules/menu_link_content/src/Plugin/migrate/process/d6/LinkUri.php
@@ -0,0 +1,98 @@
+          list(, $entity_type_id, ) = $route_parts;

Why not just $entity_type_id = $route_parts[1];? list() mostly helps with returns of methods/functions where you can't use [] or where you are assigning multiple variables. This is neither.

Modified the patch in accordance with #22 and #23.

thx, therealssj. Setting to Needs review for the testbot.

Thanks, the interdiff looks great. In general I prefer the list() syntax but in this particular case I think I went a bit far. Code looks better now.

Queued an 8.1.x test to see if this needs a reroll.

Rerolling the patch.

Can anyone explain why the rerolled patch failed?

There was conflict in file core/modules/menu_link_content/src/Plugin/migrate/process/d6/InternalUri.php since this file was deleted. I have just removed the deleted file from git.

@Sonal.Sangale, your patch is 1.32MB, much larger than the patch in #24, which is 9KB. That's a good sign that something is amiss. Maybe you were not working from HEAD when you made the patch?

Reroll of patch #24. Interdiff fails.

Patch looks good to me, thanks for the re-roll. This still needs a change record, though, before it can go in.

OK. I've not done a change record before but I started one, https://www.drupal.org/node/2761389.

Yay, thanks. Slightly expanded it to mention that internal: URIs are still sometimes returned.

I can't actually RTBC the patch itself, but it was RTBC before and basically only held up on a change notice, and I feel comfortable approving the latter, so -> back to RTBC.

@tstoeckler, thanks.

1. +++ b/core/modules/menu_link_content/src/Plugin/migrate/process/d6/LinkUri.php
   @@ -0,0 +1,93 @@
   +      $path = 'internal:/' . $path;
   

   Should this be ltrim($path, '/'), to remove any leading slashes?

2. +++ b/core/modules/menu_link_content/tests/src/Unit/Plugin/migrate/process/d6/LinkUriTest.php
   @@ -0,0 +1,134 @@
   +    $container = new Container();
   

   Maybe I'm out of date, but don't we use ContainerBuilder in unit tests?

1. I moved the ltrim so it is trimmed before it goes to parse_url.
2. Fixed

Love it.

Simplified it a little.

Great simplification, thanks!!

Committed and pushed 11904f8ea6000f3ccd93080d08886219137e3950 to 8.2.x and 310666e to 8.1.x. Thanks!

diff --git a/core/modules/menu_link_content/src/Plugin/migrate/process/d6/LinkUri.php b/core/modules/menu_link_content/src/Plugin/migrate/process/d6/LinkUri.php
index fcdd7b7..08c45d8 100644
--- a/core/modules/menu_link_content/src/Plugin/migrate/process/d6/LinkUri.php
+++ b/core/modules/menu_link_content/src/Plugin/migrate/process/d6/LinkUri.php
@@ -2,7 +2,6 @@
 
 namespace Drupal\menu_link_content\Plugin\migrate\process\d6;
 
-use Drupal\Core\Entity\EntityTypeInterface;
 use Drupal\Core\Entity\EntityTypeManagerInterface;
 use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
 use Drupal\Core\Url;
diff --git a/core/modules/menu_link_content/tests/src/Unit/Plugin/migrate/process/d6/LinkUriTest.php b/core/modules/menu_link_content/tests/src/Unit/Plugin/migrate/process/d6/LinkUriTest.php
index 656e2a5..93a7280 100644
--- a/core/modules/menu_link_content/tests/src/Unit/Plugin/migrate/process/d6/LinkUriTest.php
+++ b/core/modules/menu_link_content/tests/src/Unit/Plugin/migrate/process/d6/LinkUriTest.php
@@ -10,7 +10,6 @@
 use Drupal\Tests\UnitTestCase;
 use Drupal\Core\Entity\EntityTypeManagerInterface;
 use Drupal\Core\Path\PathValidator;
-use Prophecy\Argument;
 
 /**
  * Tests \Drupal\menu_link_content\Plugin\migrate\process\d6\LinkUri.

Unused uses removed on commit.