Follow-up to #2256497: [meta] Menu Links - New Plan for the Homestretch
In the menu link system and other places we may want to associate data with a specific route name AND route parameters e.g. data associated with 'node.view' and node => 1
In Drupal 7, this would be a certain system path, but in Drupal 8, we want to adhere to the priciple that the system path may be altered by altering the route path without changing the relationship to the route name and parameters.
So, the system path is NOT the right answer. Instead one could use the route name and some serialization of the route parameters in a ingle text field.
| Comment | File | Size | Author |
|---|---|---|---|
| #37 | 2302139-route-cache.patch | 2.86 KB | djdevin |
| #36 | query.png | 175.38 KB | djdevin |
| #33 | interdiff-31-32.patch | 0 bytes | rassoni |
| #32 | drupal-route_cache-2302139-32.patch | 7.74 KB | rassoni |
| #32 | drupal-route_cache-2302139-32.patch | 7.74 KB | rassoni |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #1
pwolanin commentedShouldn't be worked on until after #2301319: MenuLinkNG part5: Remove dead code; and party!
Comment #2
mgiffordComment #9
grimreaperHello,
Here is a patch that add a static cache on Drupal\Core\Menu\MenuTreeStorage::loadByRoute().
Thanks for the review.
Comment #11
jhedstromIt's sort of surprising this wasn't already statically cached. A few nits below:
Rather than use
drupal_static(which is in the process of being deprecated and removed), a class variable$this->cachewould make more sense here I think.I'm not certain if a cache is enough to consider this standardized upon, but part of the final patch here needs to remove this todo comment.
Comment #13
nginex commentedTagging for Drupal Global Contribution Weekend
Comment #14
nickolajComment #15
nickolajReused a cache from the class instead of
&drupal_static. Please review.Comment #16
nickolajComment #17
andypostComment #21
sanjayk commentedRe-roll patch for 9.2 and also fixed failed test case.
Comment #22
abhisekmazumdarChanging the status to Needs review as there is a patch to be reviewed.
Comment #26
grimreaperRerolling patch from comment 21.
I also created two MR, one against 9.2.x and one against 9.3.x for easier rebase.
I am trying to apply patch on 9.2.4
Comment #31
rassoni commentedRerolling patch from comment 30
Fixed PHPCS and Deperactred code.
Comment #32
rassoni commentedFixed Command Failed.
Comment #33
rassoni commentedForgot to attach interdiff
Comment #34
catchDon't think we can use a persistent cache here, or at least not without adding the relevant cache tags to it.
The original approach here was to add a static cache, #3047289: Standardize how we implement in-memory caches has a way to do that without using drupal_static().
Could use an issue summary update. This is also likely cached at a higher level now like the render cache, so we'd want to check if this data is actually requested more than once on the same request (for example after a cache clear).
Comment #36
djdevinSeems to still be called a lot, even with a warm cache. Looking at RDS insights it's still a top query under load.
General log is full of the same query, even when trying the page as anon (no page cache, just dynamic cache).
(346 is the current node being viewed)
Could be something with our site but still investigating.
Comment #37
djdevinPatch worked once rerolled for 10.3.x, not flooding the query log anymore.
There's a tag set on config:system.menu.[name], not sure if that gets flushed if a link changes.
Comment #40
vasikeI think this could be related https://www.drupal.org/project/drupal/issues/3523497
for the issue, results, implementation.