Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Motivation
#2339151: Conditions / context system does not allow for multiple configurable contexts, eg. language types is going to introduce an event for fetching the context for blocks to render. Parts of that actually is global context and always available. Rules is going to need a way to fetch this context, such as page manager and possibly more context-aware systems.
Proposed resolution
Provide a dedicated event to allow the subscriber to add to global context, which blocks can merge in also.
Remaining tasks
Do it.
User interface changes
-
API changes
Just additions, no changes.
Comments
Comment #1
tim.plunkett+1
Comment #2
Wim LeersCacheContextInterface
already represents global contexts, but only provides a label and the generation of a corresponding cache key.We should look into unifying them. Due to #2344045: ContextInterface needs documentation still not being solved, it's very hard to start doing that.
Comment #3
fagoYep. As the context is already there then, we'd have to make it possible to add a way to derive cache key for a certain kind of context. However, there is currently no class which is provided with the context so probably add a class for that somewhere - or - add it directly into contextinterface + make it possibly for data types to control cache key generation.
I'd assume it's generally important to have this for context such that you can properly can generate a cache key for a context-aware block based on its context.
Comment #4
Wim LeersExactly!
Comment #5
EclipseGc CreditAttribution: EclipseGc commentedMy current patch in #2339151: Conditions / context system does not allow for multiple configurable contexts, eg. language types should greatly simplify this issue. I've streamlined the gathering of contexts, made it happen at one point (in the Variant) so it should be pretty obvious where to put this when we decide to do it. Likewise, that patch adds an administrative context event. That issue currently focusses on blocks/conditions specifically, so we may decide to rename some stuff once it lands, but it should have a generally positive impact on this issue.
Eclipse
Comment #6
Wim LeersSounds great! :)
Comment #7
fago@EclipseGc: What do you think could need renaming later on? If possible, we should probably try to figure out right names in the beginning to avoid later API changes due to the renames.
Comment #8
EclipseGc CreditAttribution: EclipseGc commentedI specifically meant creating a couple of new global context events, and reworking some of the existing context classes to return on those events instead of block specific ones.
Eclipse
Comment #9
Wim Leers#2339151: Conditions / context system does not allow for multiple configurable contexts, eg. language types landed, this can now move forward. Is this really only 'normal'?
Comment #10
Wim LeersComment #11
dsnopekUpdated issue summary a little to make the proposal here a little easier to understand.
Comment #12
tim.plunkettThis is absolutely major. Many contrib modules that want context will be confused why they don't have access to the context set by seemingly-generic things like \Drupal\block\EventSubscriber\CurrentUserContext.
Comment #13
EclipseGc CreditAttribution: EclipseGc at Acquia commentedTotally agree. Page Manager and Rules, both "super contrib" in their own right, will both want this. Otherwise they'll have to invent it for themselves with diverging systems.
Eclipse
Comment #14
dsnopekComment #15
Fabianx CreditAttribution: Fabianx for Drupal Association commentedComment #16
yched CreditAttribution: yched commented@Wim Leers in #2 :
@fago in #3 :
Was the idea to somehow merge the notion of "cache contexts" with the generic "(blocks / conditions / rules / page manager / ...) Context API" ? Is that still relevant ?
(I guess so, since this just got added as a child issue of the #2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance meta for the "cache context API" ?)
Comment #17
Wim LeersAFAICT this has effectively been fixed by #2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed.
Assigning to Fabian for further feedback.
Comment #18
Wim LeersAsked both Fabian & Berdir.
Hence closing :)
Comment #19
yched CreditAttribution: yched commentedThanks for the hints - I was having a hard time figuring out how we were going to unite at this point ;-)
Comment #20
BerdirTo clarify, #17 has the wrong issue referenced. That has little to with this.
The actual issue that did this was #2354889: Make block context faster by removing onBlock event and replace it with loading from a ContextManager.
Cache contexts vs contexts from the initial comments is misleading although it is what that other issue is about. But that wasn't about merging them, it was about (plugin/block) contexts *using* cache contexts.
Also note that the solution that is now in HEAD can not fully replace the context handling that page manager needs. It is *only* global contexts, while page manager and likely also others still need an API to collect page manager specific cache contexts. But I could allow page manager or rules to use those global contexts and all the others that contrib can now add in a useful way instead of having to duplicate that.
Comment #21
andypostAt least one global context should exists in core so related issue