Follow-up to #2256497: [meta] Menu Links - New Plan for the Homestretch
Problem/Motivation
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.
Proposed resolution
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#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 |
#31 | drupal-route_cache-2302139-31.patch | 7.74 KB | Rassoni |
#31 | interdiff-24-31.txt | 4.19 KB | Rassoni |
Issue fork drupal-2302139
Show commands
Start 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:
Comments
Comment #1
pwolanin CreditAttribution: 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->cache
would 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 CreditAttribution: nginex at Drupal Ukraine Community 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 CreditAttribution: sanjayk as a volunteer and 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
RassoniRerolling patch from comment 30
Fixed PHPCS and Deperactred code.
Comment #32
RassoniFixed Command Failed.
Comment #33
RassoniForgot 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).