Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The menu link migration creates all menu link as "internal:/..." menu links. This leads to bad performance during cache rebuild.
I did some profiling on this.
Before (internal:/node/1): https://blackfire.io/profiles/ef057540-857b-45cf-a40d-7a46e53dc141/graph
After (entity:node/1); https://blackfire.io/profiles/0360f0cf-c6cf-4d7d-be47-ff84912c7159/graph
Compared: https://blackfire.io/profiles/compare/837c9e5b-4374-42e1-88a8-1979e38381...
Proposed resolution
Menu link migration should prefer entity-based urls (entity:node/1
instead of internal:/node/1
).
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#43 | interdiff.txt | 2.57 KB | chx |
#43 | 2648996_43.patch | 9.25 KB | chx |
#41 | interdiff-2648996-35-41.txt | 1.97 KB | quietone |
#41 | 2648996-41.patch | 9.52 KB | quietone |
#35 | 2648996-35.patch | 9.37 KB | quietone |
Comments
Comment #2
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #3
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #4
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #5
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #6
tstoecklerWorking on it.
Comment #7
tstoecklerThose 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 theinternal:/node/1
URI. Thus, with ~2800 menu links this ends up loading ~2800 entities. Additionally painful is that this does not useloadMultiple()
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.
Comment #8
tstoecklerNeeds tests, of course. But here's a first stab. What do you think?
Comment #10
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedLooks great. Just a small nitpick, we could move
$url->getRouteParameters()
into the first if condition.Comment #11
tstoecklerGood 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...
Comment #13
tstoecklerAhem.
Comment #14
benjy CreditAttribution: benjy at PreviousNext commentedThis looks great, just a couple of comments.
Removing this would be an api break, we could leave it as deprecated? It could even proxy through to the new plugin?
I thought our coding standard had all these on one line?
Comment #15
tstoecklerThanks 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.
Comment #16
benjy CreditAttribution: benjy at PreviousNext commentedNot sure about point 1, guess we name a product manager review?
Comment #17
quietone CreditAttribution: quietone as a volunteer commentedComment #18
benjy CreditAttribution: benjy at PreviousNext commentedWill 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.
Comment #19
alexpottMigrate is exempt and this sort of change that does not affect already migrated seems fine to go into any 8.x version to me.
Comment #20
alexpottRemoving 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).
Comment #21
benjy CreditAttribution: benjy at PreviousNext commentedOK, RTBC for me.
Comment #22
alexpottThis 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.
Coding standards says - There should be no white space before a closing ")" - I'm pretty sure the last comma is unnecessary.
Comment #23
neclimdulAlong the lines of alex's review,
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.
Comment #24
therealssj CreditAttribution: therealssj commentedModified the patch in accordance with #22 and #23.
Comment #25
quietone CreditAttribution: quietone as a volunteer commentedthx, therealssj. Setting to Needs review for the testbot.
Comment #26
tstoecklerThanks, 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.
Comment #28
mikeryanQueued an 8.1.x test to see if this needs a reroll.
Comment #30
mikeryanComment #31
Sonal.Sangale CreditAttribution: Sonal.Sangale at Blisstering Solutions commentedRerolling the patch.
Comment #32
Sonal.Sangale CreditAttribution: Sonal.Sangale at Blisstering Solutions commentedComment #34
Sonal.Sangale CreditAttribution: Sonal.Sangale at Blisstering Solutions commentedCan 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.
Comment #35
quietone CreditAttribution: quietone as a volunteer commented@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.
Comment #36
tstoecklerPatch looks good to me, thanks for the re-roll. This still needs a change record, though, before it can go in.
Comment #37
quietone CreditAttribution: quietone as a volunteer commentedOK. I've not done a change record before but I started one, https://www.drupal.org/node/2761389.
Comment #38
tstoecklerYay, 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.
Comment #39
quietone CreditAttribution: quietone as a volunteer commented@tstoeckler, thanks.
Comment #40
phenaproximaShould this be ltrim($path, '/'), to remove any leading slashes?
Maybe I'm out of date, but don't we use ContainerBuilder in unit tests?
Comment #41
quietone CreditAttribution: quietone as a volunteer commented1. I moved the ltrim so it is trimmed before it goes to parse_url.
2. Fixed
Comment #42
phenaproximaLove it.
Comment #43
chx CreditAttribution: chx at Migrate Rocks commentedSimplified it a little.
Comment #45
tstoecklerGreat simplification, thanks!!
Comment #46
chx CreditAttribution: chx commentedComment #47
benjy CreditAttribution: benjy at PreviousNext commentedComment #48
alexpottCommitted and pushed 11904f8ea6000f3ccd93080d08886219137e3950 to 8.2.x and 310666e to 8.1.x. Thanks!
Unused uses removed on commit.