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

tim.plunkett’s picture

Issue tags: +Page Manager

+1

Wim Leers’s picture

CacheContextInterface 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.

fago’s picture

We should look into unifying them.

Yep. 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.

Wim Leers’s picture

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.

Exactly!

EclipseGc’s picture

My 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

Wim Leers’s picture

Sounds great! :)

fago’s picture

@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.

EclipseGc’s picture

I 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

Wim Leers’s picture

Wim Leers’s picture

dsnopek’s picture

Issue summary: View changes

Updated issue summary a little to make the proposal here a little easier to understand.

tim.plunkett’s picture

Priority: Normal » Major
Issue tags: +Needs beta evaluation

This 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.

EclipseGc’s picture

Totally 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

dsnopek’s picture

Issue tags: +D8panels
yched’s picture

@Wim Leers in #2 :

CacheContextInterface already represents global contexts. We should look into unifying [CacheContextInterface and ContextInterface]

@fago in #3 :

Yep

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" ?)

Wim Leers’s picture

Wim Leers’s picture

Assigned: Fabianx » Unassigned
Status: Active » Fixed

Asked both Fabian & Berdir.

18:37:48 Fabianx-screen: WimLeers https://www.drupal.org/node/2349679 I think we can close down this one :) 
18:37:55 Fabianx-screen: with the context.repository
18:41:06 WimLeers: yched just commented on it: https://www.drupal.org/node/2349679#comment-10098350
18:41:11 WimLeers: and I'm not sure — can we?
18:41:16 WimLeers: Looking forward on your comment on that one :)
18:41:53 Fabianx-screen: WimLeers No, Contexts and CacheContexts are different things.
18:42:10 Fabianx-screen: They are related that in using one, you need to add where you got it from.
18:42:40 Fabianx-screen: But Cache-Contexts deal with variability more like HTTP Headers.
18:42:56 Fabianx-screen: While context are interchangeable.
18:43:01 Fabianx-screen: e.g. you want a 'node' as context.
18:43:07 Fabianx-screen: you don't care who provides it.
18:43:13 Fabianx-screen: So I don't think we can.
19:02:04 WimLeers: berdir: I think you might also be able to close https://www.drupal.org/node/2349679#comment-10098448
19:02:07 Druplicon: https://www.drupal.org/node/2349679 => Support registration of global context #2349679: Support registration of global context => 17 comments, 8 IRC mentions
19:02:22 WimLeers: berdir: your cacheability work on contexts effectively fixed the problem afaict
19:05:07 berdir: WimLeers: yeah I think we pretty much did exactly that :) I'm currently rerolling the block context UI patch, mainly to see how it looks after all the changes we made. fairly sure we resolved most of the cacheablity issues that EclipseGc was fighting with

Hence closing :)

yched’s picture

Thanks for the hints - I was having a hard time figuring out how we were going to unite at this point ;-)

Berdir’s picture

Issue tags: -Needs beta evaluation

To 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.

andypost’s picture

At least one global context should exists in core so related issue

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.