Proposed commit message:
Issue #2428563 by Wim Leers, Fabianx: Introduce parameter-dependent cache contexts
Problem/Motivation
- Cache Contexts define the state of the system / the viewer in how objects should be rendered.
In the case of the menu trail (which is in reality usually a subset of the page cache context), this is dependent on the menu being shown.
However cache contexts currently are global for one thing, a user, a role, a page, node grants, etc.
Proposed resolution
To avoid having to register a cache context service for every menu instead, cache contexts are given a parameter.
To distinguish the method from the normal CacheContextInterface, a CalculatedCacheContextInterface is introduced, whose getContext method takes a parameter.
Those cache contexts _should_ be container aware so that the getLabel() method can be called without getting all the dependencies needed for calculating things.
Remaining tasks
- Fix nits.
User interface changes
- None.
API changes
Introduction of CalculatedCacheContextInterface
, which allows developers to specify cache contexts that need a parameter.
See also:
See #2329101-19: CacheableInterface only has a getCacheKeys() method, no getCacheContexts(), leads to awkward implementations & #2329101-21: CacheableInterface only has a getCacheKeys() method, no getCacheContexts(), leads to awkward implementations.
Beta phase evaluation
Issue category | Bug because this is a gap in the API. |
---|---|
Issue priority | Major because it's important that D8 core sets the right example for contrib developers WRT cache contexts. Without this API addition, some cache contexts cannot be expressed as … cache contexts. That forces developers to use the semantically wrong cache keys, which is a big DX problem — if we continue to have HEAD's bizarre/confusing/frustrating splitbrain approach, developers will consider the recommended approach caching another "DrupalWTF" (rightfully). But it also has long-term negative effects for Drupal 8: it'd have prevented many Drupal 8 contrib modules to not set the correct caching metadata, hence causing incorrect caching, because cache contexts will soon bubble (see #2429257: Bubble cache contexts), and cache keys won't. In other words: if something actually is a cache context, but cannot be expressed as such because the API is lacking (well, missing even!), then it is hence impossible to write correct code! Part of the critical #2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance meta. |
Disruption | Zero disruption. Only a small bit of disruption in the 0.001% chance that somebody in already written D8 custom or contrib code is actually altering the menu block. |
Comment | File | Size | Author |
---|---|---|---|
#34 | cachecontext_parameter-2428563-33.patch | 22.72 KB | Wim Leers |
Comments
Comment #1
Fabianx CreditAttribution: Fabianx commentedFrom gut feeling I think this would muddy the waters of the very clear cache context system.
e.g. I would rather propose we use a #post_render_cache callback to render that, then to introduce parametrized cache contexts.
Of course there might be something I am missing. :)
Comment #2
Wim LeersComment #3
Wim LeersDone.
Introduces the concept of a "calculated cache context", which is different in only one way: it accepts a single (string) parameter. This can be encoded in the specified cache context as follows:
<context ID>:<parameter>
— e.g.:'menu.active_trail:tools'
, wheretools
is the name of a menu.This also allowed me to remove
MenuActiveTrail::getCacheKey()
, because it is now obsolete.Comment #5
Fabianx CreditAttribution: Fabianx commentedThis is fine now that we talked about it. The difference is:
cache_context.trail_tools_menu
vs.
cache_context.trail:tools_menu
e.g. a dedicated service configured with the parameter vs. a parametrized service.
This is still globally resolvable, which is important as some of those might need to be resolved at hook_boot() / very early kernel stage.
However for most practical purposes something rendered cache_context.trail can just be upgraded to cache_context.page.
Comment #6
Wim LeersWill still not yet apply, but is a cleaner patch already. Will reroll to make it apply.
Comment #7
Wim LeersComment #8
Fabianx CreditAttribution: Fabianx commentedI am not sure. I think making those container aware is better and loading things on demand.
Reason:
->label() which can be used in different contexts would lead to loading the menu trail service - even if it might not be needed. (and getService() gets called to check the interface).
And I think we should keep cache contexts as lightweight as possible?
---
Overall this looks great already!
Nice work!
Comment #9
Wim Leers+1 to #8.
I first thought I could work on this even while #2329101: CacheableInterface only has a getCacheKeys() method, no getCacheContexts(), leads to awkward implementations is not yet committed (that's why the patches above don't apply: they're rolled on top of that one). But I can't. It's impossible to detect which cache keys are actually cache contexts… because they're mixed together. And that's exactly what #2329101 solves.
Comment #10
Wim Leers#8 implemented.
Comment #11
Wim LeersAdded tests.
Comment #12
Wim Leers#2329101: CacheableInterface only has a getCacheKeys() method, no getCacheContexts(), leads to awkward implementations landed, now unblocked!
Comment #15
Wim LeersReroll against HEAD. Changes:
<context>:<parameter>
)I felt those additional changes were necessary to keep it clear what the
CacheContexts
service does and what the distinction is between these two types of cache contexts.Comment #16
Wim LeersThis is the actual interdiff for #15. Sorry.
Comment #18
Wim LeersForgot to update two assertions. Will be green.
Comment #19
Wim LeersThis blocks #2430341: Comment pager-dependent cache key should be a cache context.
Comment #20
Fabianx CreditAttribution: Fabianx commentedI don't know exactly what this functions return, but maybe this should be a hash?
Also again the question on coming in with a parameter and returning without that parameter and a '.'.
Maybe use:
'menu.active_trail:' . $menu_name . '.' . $trail_hash?
I love the simplicity of this!
It confuses me that I put in baz then ':' and receive baz and '.'.
Should we not standardize that?
e.g. it might be needed (not sure where) to calculate from a CID back to cache contexts.
nit - should it not be \Exception() to be consistent with the rest?
Overall:
I think maybe getCacheContext() should just return the value, and convertTokensTo... should then add the replaced cache_context in front and separate with a '.'?
Comment #21
Fabianx CreditAttribution: Fabianx commentedSpoke with Wim:
My proposed changes can be follow-up as the code is just moved.
Would still like to see the $menu_name in the corresponding CID, leaving at CNW for that as that parameter part is new.
I am fine with a follow-up for hash + what those cache contexts return.
Comment #22
Fabianx CreditAttribution: Fabianx commentedComment #23
Fabianx CreditAttribution: Fabianx commentedComment #24
Fabianx CreditAttribution: Fabianx commentedOpened #2430397: When mapping cache contexts to cache keys, include the cache context ID for easier debugging as potential follow-up.
Comment #25
Wim LeersComment #26
Fabianx CreditAttribution: Fabianx commentedRTBC, looks great now! Thanks!
Comment #27
Fabianx CreditAttribution: Fabianx commentedComment #28
alexpottThis issue is a major task. To evaluate whether we should make this change during the beta, we need to update the issue summary according to https://www.drupal.org/core/beta-changes to explain what improvements it makes and what the disruption of the change would be. Can someone add Drupal 8 beta phase evaluation template to the issue summary.
Comment #29
Wim LeersAdded beta evaluation, updated CR.
Comment #30
catchThe issue summary is out of date - I just committed the book issue that doesn't use this.
So is this just for #2430341: Comment pager-dependent cache key should be a cache context now?
Comment #31
Wim LeersThe book one (#2429261: Replace the hardcoded cache key on the book navigation block with a 'book navigation' cache context) does not need a parameter.
This issue for both
MenuActiveTrail
(converted as part of this patch) as well as indeed #2430341: Comment pager-dependent cache key should be a cache context, and the cases that contrib/custom modules will surely encounter.Comment #32
catchThis sentence doesn't parse for me. Should it be:
"An array with the parsed results, with the cache context IDs as keys, and the associated parameter as value (for a calculated cache context) or NULL if there is no parameter." or similar?
Just a general note there's a slight grey area here for something like a view mode with the terminology. I'd expect this to be part of the cache key, but it's more colloquially a 'variation/representation' of an object. Not to fix here of course.
The menu trail implementation in the patch makes sense. I'm not 100% on the comment pager patch as it stands - discussed with Wim in irc and we can continue in that issue.
Comment #33
Fabianx CreditAttribution: Fabianx commentedThe menu_trail is a cache_context as its a sub set of the cache_context.page - and as such usually implicitly bound to that.
This is not true for the view_mode.
Comment #34
Wim LeersComment #35
Wim LeersOpened #2432837: Make cache contexts hierarchical (e.g. 'user' is more specific than 'user.roles') for #33, since that has been brought up several times now. Better to have a dedicated issue, even if only for discussing it.
Comment #37
catchOK happy with this now.
Committed/pushed to 8.0.x, thanks!