Proposed commit message:
Issue #2354889 by larowlan, dawehner, lauriii, Berdir, catch, martin107, pfrenssen, EclipseGc, Fabianx, Wim Leers, dsnopek, tim.plunkett, jibran, andypost: Make block context faster by removing onBlock event and replace it with loading from a ContextManager
Problem/Motivation
During the usage of the webprofiler on a stock Drupal (no content needed etc., just standard install) I realized that the following events needed not 0 time as I would have expected it: 'block.condition_context'
What is this event doing. There are by default three subscribers for determining the context of a block:
NodeRouteContext
CurrentUserContext
CurrentLanguageContext
None of them actually differ from block to block, they are all "global" contexts.
Also even if no blocks are configured to use them, they all run on every HTML page request. In core the most expensive (particularly on anonymous page requests) is #2479529: \Drupal\user\ContextProvider\CurrentUserContext::onBlockActiveContext() loads the anonymous user, but contrib is expected to add more contexts.
Proposed resolution
- Have the block visibility configuration store a reference to the service which provided the context.
- When checking block visibility, only initialize the contexts required by blocks placed in the active theme
- Maintain the optimization where we only initialize global block contexts once per request (previously they were running for every block)
- Make this logic and pattern re-usable for contrib
Remaining tasks
Reviews
User interface changes
None
API changes
BlockEvent
and BlockEvents
are removed
BlockSubscriberBase
is removed
Context providing services must be tagged as context_provider and implement Drupal\Core\Plugin\Context\ContextProviderInterface
BlockRepositoryInterface
changes thus:
- public function getVisibleBlocksPerRegion(array $contexts, array &$cacheable_metadata = []);
+ public function getVisibleBlocksPerRegion(array &$cacheable_metadata = []);
Data model changes
Visibility conditions now store a reference to the service ID as well as the context slot name in their mapping.
Before
visibility:
node_type:
id: node_type
bundles:
article: article
negate: false
context_mapping:
node: node.node
After
visibility:
node_type:
id: node_type
bundles:
article: article
negate: false
context_mapping:
node: @node.node_route_context:node
Profiling (from #209
### FINAL REPORT
=== 8.0.x..8.0.x compared (559b0b0e56140..559b0c03d41d6):
ct : 27,504|27,504|0|0.0%
wt : 64,377|64,306|-71|-0.1%
mu : 14,889,272|14,889,272|0|0.0%
pmu : 14,956,152|14,956,152|0|0.0%
=== 8.0.x..2354889 compared (559b0b0e56140..559b0c10cc4dc):
ct : 27,504|26,892|-612|-2.2%
wt : 64,377|61,858|-2,519|-3.9%
mu : 14,889,272|14,157,288|-731,984|-4.9%
pmu : 14,956,152|14,218,696|-737,456|-4.9%
---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage
### XHPROF-LIB REPORT
+-------------------+------------+------------+------------+------------+------------+
| namespace | min | max | mean | median | 95th |
+-------------------+------------+------------+------------+------------+------------+
| Calls | | | | | |
| | | | | | |
| 2354889 | 26,892 | 28,579 | 26,909 | 26,892 | 26,892 |
| 8_0_x | 27,504 | 29,334 | 27,522 | 27,504 | 27,504 |
| | | | | | |
| Wall time | | | | | |
| | | | | | |
| 2354889 | 61,858 | 76,623 | 64,582 | 64,135 | 68,569 |
| 8_0_x | 64,306 | 93,565 | 68,014 | 67,082 | 74,414 |
| | | | | | |
| Memory usage | | | | | |
| | | | | | |
| 2354889 | 14,157,120 | 14,557,944 | 14,189,594 | 14,157,288 | 14,509,872 |
| 8_0_x | 14,889,232 | 15,299,768 | 14,932,275 | 14,889,272 | 15,242,024 |
| | | | | | |
| Peak memory usage | | | | | |
| | | | | | |
| 2354889 | 14,218,696 | 14,685,672 | 14,251,873 | 14,218,696 | 14,570,544 |
| 8_0_x | 14,956,112 | 15,429,384 | 14,999,913 | 14,956,152 | 15,307,784 |
| | | | | | |
+-------------------+------------+------------+------------+------------+------------+
Comment | File | Size | Author |
---|---|---|---|
#242 | 2354889-242.patch | 60.33 KB | dawehner |
Comments
Comment #1
dawehnerNo content at all
Using the !wonderful!
xhprof-kit
(hacked it to get it working withuprofiler
) I got to the following result using no content (yes I know this is kind of cheating as the view/render is less important with that,but let's assume we would have a cache HIT on the main content ...
Maybe this is indeed worth to have a look at. In general I am not sure whether the approach is the actual proper one. The context system should be aware of global contexts and just inject them as fast as possible.
Otherwise we could also have a base context class for global ones which allows you to get rid of the requirement to do the one-time loading for your own.
50 frontpage articles
Comment #2
dawehnerComment #3
dawehnerComment #4
Fabianx CreditAttribution: Fabianx commentedI must say I am at a loss how event subscribers work, how this keeps state and how this->addContext() differs from the other internal class state.
The benchmark result looks great though.
Comment #5
dawehner@eclipseGC asked me to move things into the base class but given that you have to know the name of the context I decided to not do that. We can do that always later but should move a bit more forward and not discuss things for the sake of it forever.
Comment #6
EclipseGc CreditAttribution: EclipseGc commentedSo, as part of #2339151: Conditions / context system does not allow for multiple configurable contexts, eg. language types I've had to touch this topic. That issue is moving context gathering to FullPageVariant and it happens once and is injected in the block plugins & entities for blocks/conditions to leverage as necessary. How about we postpone this issue on that one and if that lands we can close this, and if, for whatever reason I can't get that issue in, we can revisit this one.
Eclipse
Comment #7
BerdirI noticed similar issues in page_manager.
Comment #8
dawehnerThe other issue fixed that problem.
Comment #9
Wim LeersIndeed! :)
Comment #11
catchRe-opening - I see this taking 7-10ms on the front page, minimal profile.
- we get the global contexts even if no visibility is configured - this takes 4ms in TypedData, also #2479529: \Drupal\user\ContextProvider\CurrentUserContext::onBlockActiveContext() loads the anonymous user and then other bits here and there.
Comment #12
catchHere's a very rough patch, works enough for profiling, interesting to see how much it explodes.
1. Don't get the contexts up front.
2. Do get the contexts when they're actually needed.
#2 should not work like this - we really want to get the contexts needed by the visiblity plugins, not all of them at once, and not use a nasty hack like the one in here.
Comment #13
dawehnerThat is certainly some progress but I guess we should really apply the proper strategy:
Given that those contexts also matter for rules, I guess changing the API and moving away from events would have an impact, but especially
if its a positive one performance wise we should consider doing it.
Comment #15
catchEclipseGc asked in irc if I could profile a node page. The node context takes about 0.8ms on a node page so not much different.
I think this comes down to:
~4ms for TypedData stuff.
~1ms for each context
Works out as ~8ms with minimal profile.
Bumping to major, but this is borderline critical if it requires an API change per #1744302: [meta] Resolve known performance regressions in Drupal 8. Even with SmartCache we'd still need to run this code on every cache miss below SmartCache.
Comment #16
andypostSo maybe better to keep global contexts cached somewhere to not dispatch event on each region?
If condition knows about needed contexts so it could be $entity->getNeededContexts($condition) somehow
probably that disatcher needs injection
Comment #17
Wim LeersQuoting IRC:
See #2318377: Determine whether a view is cacheable and its required contexts, store this i/t config entity for the history around that, and hopefully for inspiration.
Comment #18
EclipseGc CreditAttribution: EclipseGc at Acquia commentedI'm curious how cheap that would be. With #2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed it would be very feasible to calculate the cacheability of a block on save, however that data is still serialized into the config table, and requires entity loading every block in the system to first determine which blocks are used for this theme and then extract the cacheability data. Unless we want to introduce new tables to query that specific information, which would make Block a very different sort of config entity from the rest of core. That's fine, I'm just pointing it out.
Most of the damage here seems to be initializing typed data... no?
Eclipse
Comment #19
catchWe already do that every request (give or take a config entity query to get blocks for the theme) - see #2479459: BlockRepository::getVisibleBlocksPerRegion() does an uncached entity query on every request which tries to wrap the block querying and loading in a cache. So having the required contexts stored in the block entities would add very little if anything given we already do this.
Comment #20
Wim LeersExactly!
See:
So we receive all the contexts (that were so expensive to retrieve), to then NOT use them at all during the loading of block config entities. It's only AFTER we have loaded all block config entities that we start using contexts.
That's why I like that approach: small set of changes, yet still big impact.
Comment #21
EclipseGc CreditAttribution: EclipseGc at Acquia commentedMy point is that that would all happen before we ever do any of getVisibleBlocksPerRegion() because that would be required to know which contexts will actually be used. That's fine I suppose, but that's the layer at which it should happen.
Eclipse
Comment #22
Fabianx CreditAttribution: Fabianx commentedYes I agree.
If we think of this as a 'page' object (which we simulate) we have:
- Block IDs
- Contexts those blocks use
- Conditions those blocks use
If I was to design that I would use a very simple object to store that data per theme, e.g.:
However why not simulate that by caching that information in this exact fashion after building it once? Might take a little moment longer (but could be deferred to build after saving as write-through cache) but then we have all information available to efficiently do the same:
- Get contexts => we already know which contexts those blocks expect, so no need to load more of them.
- Load blocks by region (we already know that information, no need to query)
- Load all condition plugins that are used at once (saves some time, too - possibly less discovery)
- Visibility checks
- Then one block_load_multiple
- Then building the individual regions
That is similar to what catch tries to do in that other issue (but only for the blocks), but caching in this fashion seems the most clean to me.
Am I missing something?
Comment #23
dawehnerI agree that introducing some form of abstraction for an internal optimization is kinda odd, especially given that the way how panels will solve that problem is probably 100% different,
so that information could not be shared.
Comment #24
Fabianx CreditAttribution: Fabianx as a volunteer commentedPer #2375695-83: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed this is really important to do. An architecture that always loads all condition and other plugins via an event does per design not scale.
To quote myself:
And a service and using "don't call us, we'll call you" approach seems to be the sanest.
Removing onBlock however is something we cannot do after 8.0.0 ...
Comment #25
catchYes I have been thinking about making this critical for a while, but held off because the saving with standard profile is not quite 10ms (although it is not shabby, just #2479529: \Drupal\user\ContextProvider\CurrentUserContext::onBlockActiveContext() loads the anonymous user would be an improvement of over 3ms.
However the fact that we'll get more block contexts added by contrib, and they'll also always run on every request, does make this a 'performance critical' - see note about contrib modules here:https://www.drupal.org/core/issue-priority
Also as a context author, I'd never expect my context to be called unless actually in use, so just like the user context, it doesn't look like it should be in the critical path, when currently it is.
Comment #26
catchComment #27
larowlanre-roll
Comment #28
catchComment #31
larowlanUpdating issue summary to include notes from the weekly criticals meeting.
Comment #32
Wim Leers+1 to #24, #25 and #31.
Comment #33
larowlanTaking a look
Comment #34
larowlanLooks like these are already being saved in the config - note the context mapping:
The on block event also fires in an administrative context - see for example
CurrentUserContext::onBlockAdministrativeContext
And then
ConditionPluginBase::buildConfigurationForm()
also callsaddContextAssignmentElement
which adds this elementSo it looks like this is already being saved and we already show the form element - we just need to utilize it?
To test this, I added a 'user from route' context and here's the screenshot of it in action.
Thoughts?
Comment #35
dawehnerGreat research @larowlan, its great that we have that part already!
To be clear, these contexts are added via subclasses of
\Drupal\block\EventSubscriber\BlockContextSubscriberBase
,see
so much like for cache contexts we need a mapping between these machine names and their corresponding context provider, so we can ask them directly.
Current flow
What are we doing at the moment:
Suggested flow
(Note this tries to be as minimal as possible)
Given how similar contexts here are to cache contexts, we should try to build a similar model. Maybe one day we could even partially unify them, but I guess this is tricky,
given its typed data requirement.
\Drupal\block\EventSubscriber\BlockContextSubscriberBase
to a tagged service, similar to cache contexts,Comment #36
larowlanYeah they're already service (event subscribers) but the service ID doesn't match the context mapping, so we'd need to align those first.
Will start there
Comment #37
BerdirKeep in mind that a single event subscriber can add expose multiple (dynamic) contexts and we must support that. So we can't just 1:1 map the service names and the context names.
For example, page manager has a subscriber that exposes route parameters as context and there's a patch to expose configurable contexts.. with any possible name.
We can't statically map that.
Comment #38
dawehnerIs there no way to at least know the list of possible once in advance? It sounds architecturally wrong to require to execute all of them all the time, even you just want one.
Well route parameters are context are still one context, if you think about how cache contexts work, it could be statically mappable, in the sense that there would be an additional resolve step in there.
Comment #39
larowlanAny reason why we can't store the service ID as well as part of the context mapping?
That way we continue to execute all events in the admin context, but ensure the config contains the service IDs we need for non-admin runtime.
Comment #40
larowlanPossibly upgrade path blocker for config changes
Comment #41
larowlanAll I have time for today, WIP
Comment #42
EclipseGc CreditAttribution: EclipseGc commented@larowlan,
Your research into block context & form elements for mapping might benefit from checking out #2377757: Expose Block Context mapping in the UI. This is the issue where I've been working to get configurable block contexts in core exposed the same way they are for conditions (which is why it was built generically and re-useably.
@dawhener
Berdir is correct, and no the route context providing capabilities are not limited 1 to 1. A single route could define multiple contexts across the url parameters and (I think) even in the route defaults if properly approached. (Not horribly relevant just saying I think it's technically possible). The problem here isn't that it's impossible to get a list of available contexts, it's that core makes this incredibly difficult by not actually having any notion of block placement on a per page basis. Core just configures blocks for all pages simultaneously and that just simply insane. The caching layer we've built expects to functionally calculate all of this information, but the second we allow stuff from the url (which is sort of a sane expectation) you end up needing to calculate all of that across all pages because Drupal has no notion of page management.
In a true page_manager type scenario, this would be much easier cause we'd just ask the page object what contexts are relevant for this page variant. Even calculating all variants at once would be pretty easy in 99% of the use cases and some sane expectation can be force down on that other 1%.
Eclipse
Comment #43
larowlankicking some more
Comment #44
larowlanMore WIP, out of time - implements most of 35 but haven't looked at tests manual or otherwise
Comment #45
larowlanAdding API changes
Comment #46
BerdirAs an alternative idea that would possibly have a smaller API change, what if we'd just pass the list of contexts IDs that we need to the services that provide them? It would mean they are responsible for checking that and only add them if necessary but it would probably help quite a bit already, assuming that calling those services isn't too expensive but creating the contexts is.
Might be worth looking into if we run into troubles with this approach?
Comment #48
martin107 CreditAttribution: martin107 commentedThis minor nudge, makes BlockHtmlTest pass.
Comment #50
tim.plunkettThat looks broken
Comment #51
dsnopekComment #52
larowlanThis should be ready for tests, page renders ok, only needed contexts are loaded.
Comment #53
BerdirCrossposting my comment in #2375695-86: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed.
Wondering if someone here, especially @larowlan, has any ideas/suggestions on the other issue and if this could simplify the other one. The primary conflicting point over there seems to be that the services need to return context objects without a value even if there is no context, so that we can transport the cacheability metadata. *Maybe* this could support that in a more direct way, e.g. by exposing the cacheability metadata directly with a separate method or so. But we could have multiple contexts with different metadata and we still need a way to transport it...
Comment #54
jibranWhy not make
Comment #55
dawehnerI think that would be not a bad idea, especially because this could make that information 100% cacheable as this kind of special interface would ensure that things aren't dynamic on runtime ...
Comment #56
larowlan@Berdir will try and have a look over there, but my bandwidth is uber-low until at least the 24th so it might take a while
Comment #58
Fabianx CreditAttribution: Fabianx as a volunteer commentedOverall: Fantastic work!
Some comments:
I think it would be better to do it like cache_context_manager, to only collect service tag IDs and make that service container aware.
Injecting all possible block_context_provider's feels wrong ...
I agree with Jibran, that we should store the result.
I think it would be better to have a more specific interface that runs the onBlockActiveContext() event, so that this information is hidden from users of this service.
Unless there is a need to get a specific context provider, but if you go to that length, why not get it from the container itself?
Should this not try to use the ID the service currently has?
Or if we want to hardcode it, we should also add it to the docs of the class.
e.g. currently $this->_serviceId.
Not sure we have a method for that ...
Is this not missing the new '@' notation?
Imho this is way too much information, especially with using the BlockContextEvent().
I would imagine the API to be like:
// Collect all conditions
$context_plugins = [];
if ($condition instanceof ContextAwarePluginInterface) {
$context_plugins[] = $condition;
}
$contexts = $this->contextManager->getContexts($context_plugins);
That nicely decouples implementation from Interface, it also removes the hardcoding for conditions.
Comment #59
larowlanComment #60
larowlanFixes unit tests, doesn't touch #58 or functional tests.
Out of time for today.
Time poor.
Comment #62
EclipseGc CreditAttribution: EclipseGc commentedWhy? This should return empty array if there are no conditions, and foreach empty array still won't perform any of the subsequent actions. ??
can we get a better type hint return here?
This is going to invoke the same active context method multiple times if its mapped into multiple blocks. Is that what we want to do?
All in all this is a little weird. I get where we're trying to go, and I think why. I'll think on this a bit more and be back soon.
Eclipse
Comment #63
larowlanMore test fixes.
Out of time again.
I agree with points in 58 and 62, just haven't had time to implement yet.
Still to-do list:
Comment #65
EclipseGc CreditAttribution: EclipseGc commentedThis is not really a competitive patch but some of the slow down that's been central to this problem has to do with (I think) when the typed data manager stuff gets called. Looking at this further, I was troubled by the implications of the public setters on the classes, so I attempted to remove them and rework Context classes as dumb data objects. The tagged context services that are ALSO event subscribers are just weird to me. I see what we're attempting to do with the @service_id, but it still seems odd to me. Just want to make sure we don't have other options. I don't expect this will pass, I have no clue about the performance.
Eclipse
Comment #67
Wim Leers#65:
+1
This would definitely be a big improvement — significantly less weird.
… but… isn't this out of scope for this particular issue? It seems an orthogonal problem to fix? (Then again, I don't understand the Context system well enough to say this with authority.)
Comment #68
dawehner@EclipseGc
Opened up a dedicated issue for you, where you can improve the general speed: #2508884: Make contexts immutable
This issue is also about scalability
Comment #69
Wim Leers#68: good call.
Comment #70
dawehnerjust some normal review ... more later
meh I'd expected it as part of user to be honest
I'm a bit confused, $contexts seems to be never actually used here?
So far it doesn't manage anything, but rather just collects
Comment #71
dawehnermore nitpicking
you can use
\Drupal\block\BlockInterface[]
Comment #72
larowlanmore test fixes
still to do
Comment #74
larowlanRemoves the 'user from route' context I'd only added for local testing/research sake in #34 (didn't mean for that to make it into the patch).
This should fix the last fail - comment_entity_storage_load is being during rebuild container (in DrupalKernel::initializeContainer we do a lot of work to carry through the current user, but it results in comment_entity_storage_load running before the container is fixed).
Comment #75
larowlanstill to do
Comment #77
larowlanwhoops
Comment #78
larowlanFixes #70 and #71 (parts of 70 were fixed in #74)
Still to do:
Comment #81
andyposthow comment manager could be not registered if comment module enabled?
please elaborate
Comment #82
catchDo we need to fix #2495087: comment_entity_storage_load() is too expensive on cold caches and postpone this issue on that?
Comment #83
xjm@catch, @alexpott, @effulgentsia, and I discussed this issue today. We decided to defer triage of this critical because the performance gains for core alone are not that significant, and it's unclear whether they will be that significant with contrib either, but the change does require a BC break that would potentially affect a number of significant contrib modules. We'll revisit this issue again in a week or two.
Comment #84
EclipseGc CreditAttribution: EclipseGc commentedThat seems super sane to me, in the mean time could we look at #2508884: Make contexts immutable and see if it provides any performance gains since I was explicitly trying to move the most expensive operations in the existing code flow past the caching point? It'd be great to see if it succeeded at all and might put some of the changes proposed in this issue in a different light.
Thanks!
Eclipse
Comment #85
Wim LeersI'm very surprised about #83. What about #24 and #25?
EDIT: Just providing a bit more explanation, showing some of the reasoning — would be appreciated.
Comment #86
dawehner#83 maybe is fair in terms of performance, but from my point of view, this issue is mainly about scalability, which for example #2508884: Make contexts immutable doesn't solve.
Once contrib adds like kind of for contexts, like one for every entity type, things will blow up pretty quickly.
Comment #87
larowlanComment #88
larowlanFixes #62
Still to do:
More tomorrow
Comment #89
larowlan#67 was a different approach, so just #58 to go, working on that now
Comment #90
larowlanStarts on #58
Looking for feedback from bot and humans alike
The event and the base subscriber still remains, as I'm sure that is part of a bigger picture.
Figure there will be fails, will tackle them tomorrow
Comment #92
larowlanmissed an easy one
Comment #94
larowlanThis should be last of feedback from bot, now for human feedback :)
Comment #95
Fabianx CreditAttribution: Fabianx for Drupal Association commentedOverall looks really great! Just some questions and remarks:
Unused
Is that change necessary?
Does it help performance?
Would that not call getContexts() for each condition?
I think this should explain clearer when it returns what.
However my suggestion is have two separate methods:
- getContexts()
- getAdministrativeContexts()
Would then be:
->getAdministrativeContexts()
That would feel good.
Necessary API change, but I think its the best way and there should not be that many users of that function in contrib.
@todo Check in beta eval and IS.
Are we using that event still? (It did seem like BlockContextManager now took over that task).
We should document the service ID in the class doc block, as it means it cannot be changed - which I am fine with.
Or we need to use $this->_serviceId or have an official API to ask the container for a services name. (follow-up though)
While this issue is specifically about the block context conditions, there is also another block context - IIRC.
I think the blocks would have been able to use that one with the global event.
So I _think_ we need to fix that case too to avoid any regressions.
nit - simple hardening explode('@', $context_provider_id, 2);
Comment #96
larowlanComment #97
larowlanComment #98
dawehnerhonestly i'm not sure whether collecting all the services in a parameter is something we should do. This is kind of a new concept compared to just pushing in the service IDs using a pass directly. Any reason why you went this route?
Both new lines aren't needed.
It would be great to explain a bit what are global block contexts and @var string could be added.
This is honestly really really confusing code ... at least some documentation why specifying them externally are admin contexts and passing them in actual runtime contexts.
Gets is not really the perfect description. What about Allows to determine context for the ... event
Yeah I guess we need a CR about that telling peopl ehow to write block contexts now.
It would be also great to have somewhere, maybe the BlockContextProviderInterface the idea of having @ in name of the contexts.
This change feels a little bit optional to be honest. Do you mind quickly explaining why this is needed as part of this patch?
Comment #99
BerdirDidn't really read any comments yet and not a full review, so just ignore what has been discussed already...
Maybe it wasn't before, but this really seems like an unnecessary change now? You'll also avoid a conflict with #2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed here.
contexts vs contextProviderIds.. seems like that should be a bit more consistent?
Don't we usually use the container aware thing for injection of the container?
Wouldn't it be easier to have two methods for this and one where $context_ids is rquired?
order of the methods in the interface and the class is different, diff looks very weird, maybe move the new method below so it's reasier to review and cosistent with the interface?
seems like we can remove this completely now?
Comment #100
Fabianx CreditAttribution: Fabianx for Drupal Association commented#98: Cache Contexts Manager uses that part of collecting things. It allows to avoid having to use proxies for all and allows knowing the service_ids, which as with cache contexts become important here now.
#99: I had all of that all in #95, so we are on the same page.
Comment #101
dawehnerWell right, but I don't get why we not just inject that into the service and be done. Storing the parameter, as it it would be some form of configuation, seems a bit redundant.
Comment #102
lauriiiThere was quite a lot of conflicts with the latest head because of #2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed so just a reroll to see how many web/kernel tests this breaks
Comment #104
lauriiiI think I've addressed comments from #95, #98 and #99 and should have fixed the tests too.
Comment #106
Fabianx CreditAttribution: Fabianx for Drupal Association commentedThis is wrong.
This needs to call onBlockAdministrativeContext() and can't use getContexts().
I don't think that will work as the block does not know its contexts.
I _think_ (not sure will need a test) that this should parse the context IDs in the same way, then get them from the contextManager.
Comment #107
larowlanpicking this back up
Comment #108
larowlanThis:
* Moves everything to \Drupal\Core\Plugin\Context namespace so it is generic enough for page manager to use too
* Moved BlockContextManager to ContextStackManager
* Introduces ContextStack which is an amalgamation of ContextCollection objects
* Moves the block event to a ContextCollection - this is passed around for services to add contexts too
* The context stack manager takes care of setting the service ID on the context collection objects (so the individual providers no longer need to use the somecontext@service_id notation
* The context stack object takes care of fetching contexts from the context collection objects and converting their keys to use the @notation
* Gets rid of the event completely
This feels a lot less clunky in my book
Comment #109
dsnopekOnly skimmed through the patch quick on my cell phone, not a real review but this looks awesome! This will be great for Panels and probably Rules too.Thanks for hacking on this!
Comment #110
larowlanThe only clunky bit left here is BlockPageVariant needing to gather the service IDs with explode
Comment #111
larowlanRemoves the clunkiness of BlockPageVariant needing to know that the mappings contain @
Comment #112
larowlanUn-assigning so others can work on it during sprint
Comment #113
larowlanWe still need to profile against OP
And need to list API changes in OP
Comment #114
Fabianx CreditAttribution: Fabianx for Drupal Association commented#111 looks truly fantastic, the interdiff also makes a lot of sense.
Comment #115
larowlanNow that I think about it, I think ContextStackManager could be a plugin manager, with each of the context providers as plugins. Then the tagged services goes away in favour of Context Plugins and the service IDs become plugin IDs.
This is not dissimilar to context plugins in D7 ctools.
Should be simple from where we are now.
Thoughts?
Comment #117
larowlanFixes some tests
Comment #118
Fabianx CreditAttribution: Fabianx for Drupal Association commenteds/context_stack/collection/g
ContextStackManager
Love it!
Very nice!
This is missing the other contexts that can be on blocks, to e.g. variate by a given node.
As the ACTIVE_CONTEXT had taken care of that, I think this would be a regression.
=> CNW
Looks fantastic, needs some tests and needs to deal with the non-condition contexts that can be on block plugins.
Comment #119
jibranThis is a good idea but if this gives us the better performance then the current patch then we can switch to the plugin approach because performance is a major concern here.
Comment #122
larowlanmore test fixes
Comment #123
benjy CreditAttribution: benjy at CodeDrop commentedIt's confusing to me, but this class doesn't actually have the semantics of a stack?
Comment #125
larowlanBenjy, agree that's why I think plugin manager makes sense
Comment #126
dawehnerI like where we are heading towards to.
I still think just injecting it would be fine.
Can we use \Drupal\Core\Plugin\Context\ContextInterface here?
On an abstract level a context collection does not have direct dependencies, right? This could be a component and handle the CacheableDependencyInterface inside the core implementation.
+10 to have an interface instead of the thrown event!
Nitpick: Missing new line, wrong @file
I think we should explain here better, what the purposes are.
Should we default to an empty array? I'd say so.
I was wondering whether defining such an idenfier could live on the ContextStackInterface and ContextCollectionInterface. So instead of having to hardcode that we have a service container go with something like $context_stack->getContextIdentifier($context_id)
Also a context stack seems to be a more generic concept
do you mind moving this into its own protected method?
Comment #127
dawehnerWell, what would be the gain here really. Its just the different discovery you have here, and that is all. It though would be a bigger API change, as people would need to be move it to plugins itself. On top of that letting cache contexts and block contexts be similar at least in some way is not a bad thing developer experience wise.
It would be great if you could ellaborate the advantages of doing that.
For me personally this caused me a fried brain.
Comment #128
larowlanFor me the main advantage is if say contrib provides another 20 context providers, we don't end up with another 20 services in the container, instead just the plugin manager.
Then there's the DX and consistency with other plugins. Simpler to copy a file than add a service.
But yeah at the end of the day, its just a different discovery method.
Comment #129
larowlan@FabianX
Can you elaborate? Are you saying I should be
$contexts += $block->getContexts();
?Comment #130
BerdirNot exactly sure what he means but $block->set/getContexts() is weird. HEAD is using that to transport the contexts around, BlockRepository currently sets them all and then BlockAccessControlHandler fetches them all again to apply them to the conditions. Note that this is the block entity and that has no concept of context awareness. That's something else entirely.
I'm fairly sure that this is a left-over of when visibility condition checking was part of BlockBase::access() since we needed them inside there then.
It would probably be very easy to clean up and just inject the new context manager into the BlockAccessControlHandler and fetch the contexts you need from there. You want to check that with @EclipseGc and how he is applying contexts to blocks exactly in his block context issue. Or check the code there.
Comment #131
dawehnerWell, I still think its a separate discussion not needed to fix the critical issue.
Comment #132
dsnopekA look at this again this morning. It's still awesome! :-)
One quick comment - can we move these out of the 'block' module:
I could definitely see these contexts being used by Rules, which has nothing to do with blocks.
Comment #133
xjmCould we add a section to the issue summary here about the data model changes and the update that would be needed? Hopefully the service changes would no longer be a problem following #2507509: Service changes should not result in fatal errors between patch or minor releases.
Since this is a critical, the update hook and tests can be moved to a critical followup issue, but it would be good to know what they are before it goes in. Thanks!
Comment #134
Fabianx CreditAttribution: Fabianx for Drupal Association commented#131: I agree, while I think plugin managers might make sense, because unlike cache contexts, I don't think contexts need to work on a low bootstrap level, this is fine as follow-up. Also the context parameter is fine for now - maybe something else needs it.
#130:
What my concern is:
I have a block that gets the node from the [route] to display an image field of said node. ( I think we have at least unit tests doing that.)
What I _think_ has happened before.
The onBlockActiveEvent was fired unconditionally, the Node Context was injected into the block via setContexts(), now the block is rendered with the node correctly.
Because the current code only checks for conditions on the block, so any other 'contexts' are not fulfilled anymore.
However it might be that a) this is already broken or b) this is not a concern or c) we should just put the whole discussion to a follow-up.
I am mainly the voice of caution here, though I personally won't mind a regression (if any) in this space, because if what we have now does not break tests, it is clearly lacking test coverage and things without test coverage we can IMHO temporarily break for a critical as thats like the network cord that does not have a label, so it must be pulled ...
Comment #135
dsnopek+1 to this for all the reasons given above. I think the amount of contrib this breaks would be super minimal because contrib has in many cases been forced to duplicate the context providers (this patch even with services would make that no longer necessary).
Comment #136
Fabianx CreditAttribution: Fabianx for Drupal Association commentedDiscussed #134 in IRC, there is a method called getContextMapping(), but currently nothing in Core calls it, so likely context mapping for anything else than conditions is broken anyway (in core) ...
So lets get this in.
I think ContextStackManager could be renamed to a context plugin manager even if the discovery is hardcoded.
I use a ContainerAwarePluginManager in D7 with service_container module a lot and it works pretty great.
Then we would still have tagged services, but already a plugin manager, which would ease transition to real plugins later.
I would however also be okay to mark ContextStack as @internal for now and just follow-up on the critical.
Comment #137
Wim Leers+1
+1 — but… we should very very carefully evaluate whether plugins truly make more sense than services.
(We really need good documentation explaining the pros/cons of plugins vs. services, and the recommended criteria to choose either, because many contrib modules will have to choose either too!)
I apologize in advance for what will likely be an annoying review to address. But I find many parts here very confusing. Hopefully you'll find my suggestions/questions helpful to make this patch more understandable.
I think an explanation in the IS of the proposed architecture would massively help in understanding this patch. But the "Needs issue summary update" tag is already present.
This seems like a contradiction? (service vs. plugin)
What does "global active" mean?
"given service" -> what kind of service?
s/array/set/ ?
"[…] collected from all context provider services"
s/stack/collection/
"run-time" vs. "active".
I think "run-time" is much clearer. Can't we name the method accordingly?
Same here with "configuration-time" versus "administrative". Configuration-time is much clearer.
This doesn't seem to be a stack data structure at all?
So context stacks contain context collections, and those collections apparently only contain active (== run-time) context? Then shouldn't
ContextCollection
be calledActiveContextCollection
?Confusingly inconsistent.
These all look wonderful :)
Comment #138
Fabianx CreditAttribution: Fabianx for Drupal Association commented#137: A plugin manager is just an interface for discovering services / plugins.
The underlying architecture does not matter as long as there are possibilities to get instances of the service / plugin.
The main difference between services and plugins is:
- Services are instantiated once and remain for the life time of the Container. (unless the deprecated "scope: prototype" is used - yes, that exists ...)
- Plugins are instantiated newly whenever instances of them are created.
When one wants to expose services as plugins, one can use:
or explicitly define that this plugin manager always returns the same instances (singleton plugin manager), which is fine as long as there is not any configuration that is needed to be injected.
I personally like the plugin manager interface concept and given the ContextStackManager is newly introduced, that could be fine to use the correct interface for future proof-ness.
However l checked and changing the interface of getContexts() and getAdministrativeContexts() would not make sense.
The main thing changing would be that instead of $contexts being injected as a string a ContextPluginManager would be injected and
getAdministrativeContexts() would then do:
and
getContexts() would then do:
So I don't think at this stage we would gain anything, but more complexity and as the container->get() is internally to the ContextStackManager that should definitely be follow-up.
Also: In any case - any move away from services is certainly follow-up.
Comment #139
BerdirThere are a number of things here that don't really make sense to me yet.
* This still gets the *all* the contexts of a given service if any is requested. We should pass the/a list of slot_names (is that an official name now?) Everywhere else we seem to use context_id.
* I think we should also really statically cache the fetched contexts. They're not going to change during the request and it seems like unnecessary overhead to ask the same service multiple times for the same context.
* I don't really get why we need a stack *and* collections especially since both are just used internally. When combining with the things above, it will also get a lot more complex, because you need to add lots of methods to check if a collection exists in a the stack and get it, or alternatively get a specific context based on service_id and context_id. Seems at least the stack could be left out for now and we'd just use two arrays: $this->loadedContexts and $contexts = [] to return a list of contexts?
* We also need better names for whatever we're going to keep using. Both stack and collection are very generic concepts, but are used in a specific way here. Especially stack is confusing as others said, since I don't see how it is a stack implementation :)
Comment #140
pfrenssenFamiliarizing myself with the issue as part of the D8 Accelerate sprint, cleaned up the documentation and use statements a bit while reading through the patch.
Comment #141
Fabianx CreditAttribution: Fabianx for Drupal Association commentedComment #143
jibranCreated #2525914: Covert ContextStackManager to plugin manager for plugin manager discussion.
Comment #144
larowlanprodding some more
Comment #145
larowlanStill need an issue summary update and profiling.
Tries to address some of the reviews and clarify the naming.
Stack => Collection
Collection => Result
StackManager => Manager
Adds back the comment.module change, but I think we'll need to make the services lazy in order to revert that.
Comment #147
larowlanmissed some spots
Comment #148
dawehnerI like the new naming much better!
Do we want to change the service ID as well?
I'll just stop complaining about this.
API question, is there a result why getRuntimeContexts cannot just return the $result?
I'm curious whether there would be an advantage of return the collection instead of an array, we have things like RouteCollection in core
<3
Do you mind removing service out of that sentence? It let's my brain go into circles
Yeah let's look at the function signature again ... it does not get, it rather allows you to set it.
That documentation is a little bit confusing, to be honest. To be honest I don't get why we can't just have a collection and put everything on there directly. The code calling out to all providers could deal with getServiceId() couldn't it? So provideRuntimeContexts() and maybe an array?
As mentioned in earlier reviews, I think getServiceId() is a wrong coupling
yeah that var name is wrong now
Comment #149
dawehnerAlso did some quick profiling: Anonymous frontpage without page cache
Comment #150
larowlanThanks @dawehner - great feedback
Unassigning in case someone wants to work on it at the sprint
If not, will pick it up in the morning
Comment #151
lauriii#148.2: I made it collect the context providers inside ContextManager since context providers are not needed else where.
#148.3-4: I don't really have answers to there but it might help to hear of the use cases where the object would be needed.
#148.8: Can you open up that a little bit more?
Comment #152
Fabianx CreditAttribution: Fabianx for Drupal Association commentedThis is not the right way and should not be done on run time anyway.
It would theoretically work now, because tests are usually run with a ContainerBuilder, but findTaggedServiceIds() is not part of the IntrospectableContainerInterface.
The proper way is to leave the compiler pass, but to remove the %context_ids and instead use:
$definition = $container->getDefinition('context.manager');
$definition->setArguments(array_merge($definition->getArguments()), [$context_provider_ids]);
or something like that.
I personally don't think the parameter is wrong, but that is the proper way.
Comment #154
lauriiiDidn't know that is possible, thanks!
Comment #156
Fabianx CreditAttribution: Fabianx for Drupal Association commentedUhm,
That did not work ...
But this should (see RegisterLazyRouteEnhancers):
Comment #157
dawehnerWorking on it ...
Comment #158
dawehnerUploading a less failing one for now.
Comment #159
dawehnerMaking good simplification progress, stay tuned!
Comment #160
fagoCan you elaborate on the new concepts introduced? What's a ContextResult? Sounds like a general class for Contexts, or is it tailored to some specific use?
Same for ContextStackManager. Is its purpose registration of global context in general or is it tied to blocks?
Comment #161
Fabianx CreditAttribution: Fabianx for Drupal Association commented#160: Registration of global contexts in general.
The main idea besides all the rest is that contexts are allowed to be transformed to:
- context_name@[serviceId]
so that the provider is saved explicitly and such can be called explicitly - as it was chosen by the user explictly on the UI. (or implictly if there is only one).
So:
On UI:
$this->context_manager->getAdministrativeContexts() returns all the contexts as the event before, but does encapsulate that in a manager instead of in a generic event.
Then the context to be used is stored in that format, so context_name@[serviceId].
On block loading time, the exact known context can be asked to set the context then instead of asking who can fulfill that, so all we need to do is call:
$this->context_manager->getContexts($context_ids);
and because the format is context_name@[serviceId], the context can be gotten from the container directly.
So it is absolutely generic to plugins, but blocks use it in this specific way.
The main reason for the added complexity is that contexts by definition are not bound to a service (but can be fulfilled by anything), so they need be transformed out and back and that is also what dawehner probably is simplifying.
The main reason (what took you so long?) is that the context system is very hard to understand for anyone not Tim or Eclipse or those working with panels or rules directly (likely).
@fago: So if you have experiences and can share documentation on how it works from your understanding (or how you use it in rules) that would help, too.
Comment #162
larowlanReady to take the baton back if you're about for a handover
Comment #163
dawehnerLet's try.
Comment #164
larowlantag
Comment #166
larowlanMore cleanup
Another test
Removes ContextCollection as we don't need it anymore either
Comment #167
larowlanforgot to merge
Comment #170
larowlanSo if BlockPageVariant fatals, turns out lots of things break :)
Comment #172
Wim LeersThis is looking much better already. It's now possible to get a sense of how all this works without already being very experienced with the context system — that's a great sign :)
In this review, a whole bunch of nitpicks, but also another batch of questions/unclarities that can be addressed by better consistency, more/better documentation and potentially by better naming.
Nit: s/into/to/ ?
Nit: extraneous
\n
.Nit: we usually just say "The things" not "The array of things". Alternatively: "The set of […]" or "The list of […]". I think this is a set?
Inconsistent.
First collect which contexts slots need which context providers.
Better:
Create a map of context providers (service IDs) to context slot names.
s/to them/them/
Why "collection"? It's simply an array.
Better:
Iterate over all context providers (services), gather the run time contexts and assign them to the slots that need them.
s/on/at/
"run time" vs. "runtime"
Elsewhere: "run-time".
Needs tidying up.
Inconsistent with the docs of
getRunTimeContexts()
.s/runtime/configuration time/
s/block//
Interesting that both the existing method has to change and a new method is necessary.
No longer coupled to blocks, and a hundred times clearer what it actually means ("active" is so vague in comparison) — yay!
I have no idea what this is doing.
I almost wonder how one can know which blocks are active if we're still retrieving their required contexts? It sounds like a chicken-egg situation?
What is the meaning of "currently" here? Can during the same request a different set of blocks become active?
Nit: 2
\n
, should be 1.Nit: s/Setups/Sets up/
Nit: s/setup/set up/
Comment #173
larowlan172:
language.
, we're removing themFixed the fail, thanks to Wim who pointed out that the NodeRouteContext was sometimes not returning a context, but it should always return something, even if it is an empty context value. Thanks again to Wim, please add him to commit credits.
Comment #174
Wim LeersAwesome — thanks for the swift reroll, @larowlan! IMHO this patch is ready for new review rounds from others (@lauriii, @dawehner, @Fabianx, @Berdir etc.).
Nit: wrong docblock formatting.
So given that this existing public API function calls the new
getBlocksForTheme()
… I wonder if that new method needs to be a public API at all?IDK yet, just wondering out loud.
Less API change, yay!
This is the bit that @larowlan mentioned at the end of his comment.
s/Active/RunTime/ ?
Comment #175
jibranI don't think we need more tests here but we do need a change notice.
Comment #176
larowlanFixes 174.1 and 5.
174.2 its called by BlockPageVariant, so needs to be public.
Comment #177
tim.plunkettCan we move the !isset check into the method?
Comment #178
larowlansure
Comment #179
larowlanIssue summary update including API changes
Comment #180
larowlanAdded profiling and data model changes to issue summary
Comment #181
larowlanComment #182
tim.plunkettHmm, sorry to nitpick this method again, but I think either we should pass in the $active_theme, or it should be renamed getBlocksForActiveTheme(). I think the first option makes more sense.
Also moving it back closer to the actual loop (or even
foreach ($this->getBlocksForTheme($active_theme) as $block_id => $block) {
) would keep things more logically grouped.Comment #183
dawehnerWorking on it for a while.
Comment #184
Fabianx CreditAttribution: Fabianx for Drupal Association commentedBecause we do an API change here, we should also move out the conditions and condition service definitions out of block in Drupal\Core\ and core/core.services.yml - else this would need to be another API change and also a data model change (all IDs would change again), so likely won't be able to happen anymore in 8.x cycle.
Comment #185
Fabianx CreditAttribution: Fabianx for Drupal Association commentedLooks fantastic, so much more clear!
Here is a review:
I agree with Tim, it is better to pass the active_theme to this function.
Yahoo!
I would feel better to be able to omit the argument completely.
So much better to have a getter return something!
Same as above.
Uhm, that feels like FAIL to me ...
CNW
Ah, this is where we don't have the active theme, then maybe rename to getBlocksForActiveTheme() might be indeed better.
Comment #186
dawehnerHere is an intermediate state. Posting for berdir for review
@fabian
This is maybe not addressing all your points yet
Comment #188
dawehnerPlease clarify why this feels like FAIL for you.
Comment #190
Fabianx CreditAttribution: Fabianx for Drupal Association commentedI am curious what happens on 3v4l: http://3v4l.org/TvfSb
Yes, it works, but it feels wrong to overwrite the variable used with the variable used in the inner loop as $cacheable_metadata in the end is 'b', so that could lead easily to side-effects.
Comment #191
catchOverall this looks great. Few things I noticed between nits and minor.
I wonder a bit why we don't just check if (isset($this->contexts[$id])) in the foreach once or twice rather than the two array_diff() and array_intersect_key() up top. I kept checking where the variables are used below and the answer is not much.
From reading this I have a bit of trouble figuring out what a 'configuration time context' is. We don't care what the context values are at configuration time, but it returns context interface instances - also see below.
This interface looks exactly the same as the one above, except with 10 times the docs. If we need two interfaces, at least the manager could extend from the provider?
Nice.
Nice.
Nit: probably 'Allows filtering of blocks by region and getting all blocks for a theme'.
As above even if this works by accident let's not do it.
Looks great.
Comment #192
catchSorry cross-posted on the status.
Comment #193
BerdirNot sure about the documentation here, part of this should be on the interface if it's not really there. It's just the implementation of that interface, not really anything relevant to add, what do we usually do for that?
I discussed the naming a bit with @alexpott and @dawehner. Neither of us likes slot_name, that's just not what it is.
Given that this only exists between the manager and the providers, we think that we can go with a slightly longer and explicit name, possibly $unqualified_context_id?
Additionally, since this is a hierarchy, we think that it would make more sense to structure them like this: "service_id:unqualified_context_id".
And the last point, I think we should remove the manual namespacing that was introduced a while back, since now the unqualified context id just needs to be unique for a given service. So NodeRouteContext can use just "node" again, not "node.node". If we change it anyway, then it doesn't really matter if we rename the known unqualified names as well?
this could maybe just be $context_ids_by_service? not sure if we need to repeat unqualified, could get quite long.
we don't really iterate over *all* services
$contexts_by_service?
I think this would be a easier to read if you just loop over $unqualified_context_ids and then get it from $contexts_by_service[$...] (including an isset check)
We should probably also document explicitly on the interface that it's not guaranteed that a context can be found. context mapper then later throws an exception if that happens. but throwing one here could be tricky.
Yes, definitely confused by this line now.
i'm not exactly sure what this documentation is telling me and why it's on *this* method?
Maybe this should be part of the class docblocks as part of a bigger summary that explains what the class is doing?
I'm wondering if all this logic should not completely move to the BlockAccessControlHandler. It would possibly be slightly slower but context manager now deals with already loaded things.
The advantage is that we could remove $contexts completely from being passed around between this and block repository and then through setContexts() and getContexts() of the block entity to the block access control handler. We'd just load the contexts of the block we need in the access control handler and use it there. thoughts?
According to the implementation in ContextHandler, context mapping can also happen automatically if the context_id's match. We possibly need to care about that too, but I'm actually not sure anymore what that means with the fully qualified names. Maybe that logic should simply be removed?
The service name hasn't been updated yet, with all the suggestions above, I guess this could become:
language.current_language_context:language_interface.
Comment #194
dawehnerOh yes , I totally agree its wrong how it is, so let's rename it to cacheable_metadata_list, ... given that its an array of it
Oh Fuck, so much better!
Yeah we talked about that ... well the problem is that the keys are different. Once its with the @ and once without.
Yeah that is the fun of context objects, they might or might not have data.
Sure, even I worked for a while to get it below 80 chars before.
Comment #195
dawehnerAdressing some of the feedback from @berdir
Comment #196
Berdir1
This interdiff is basically what I'm suggesting above. Not posting full patch yet, the unit tests would explode but it works fine locally when I tested it manually with a
The optimization from getting all context ids first is gone, but that should only be a very slight regression since ContextManager now has the static cache built in.
By injecting the manager into BlockAccessControlHandler and fetching just the contexts we need there, we can remove the get/setContext methods from BlockInterface, can remove all context things from BlockPageDisplay, can remove $contexts from BlockRepository *and* make getBlocksForTheme() protected.
2
We now have ContextManager and ContextHandler. It seems very hard to explain what the manager is managing and the handler is handling and what the difference between the two.
What if.... we just merged the two new methods into ContextHandler? Or if not, make the new ContextManager name more specific.. maybe something like ContextDiscovery? Or another fancy word for "looking up contexts". Repository?
3
Last poing. We are kind of defining this new thing as a generic service that others can use as well. There's at least one problem there. If you compare it with Page Manager contexts, there's one major difference. Many contexts for Page Manager are specific to the page and need the page (e.g. configuration from it, or the route, ...). I'm sure that other cases will have similar things. I don't know if we want to try and solve this in core, but if we do, then we need.... context for the context providers? An array with arbitrary stuff ($options ?) that other use cases could pass along what they want and then providers can use it?
Comment #197
catchThat's unfortunate. This could use an explanation and an @see then?
Comment #198
dawehnerYes we could extend the interface maybe?
Comment #199
catchSo we'd extend the interface, but the structure of the return value would be slightly different. I think if that's documented it's probably OK but seems borderline.
Comment #200
EclipseGc CreditAttribution: EclipseGc commentedOh wow wtf...
Why don't we check the block's own context mapping in this function as well?
Is this scope creep?
If $key does not appear as a value in $array, unset $array[$key]???
Am I reading that wrong? Sounds a lot like array_filter() to me if we're expecting identical key/value pairings with a 0 value under certain conditions.
Looking at berdir's interdiff, a lot of the code I commented on is removed. The primary arch change in this issue appears to be instead of "getting active contexts" we "get mapped contexts" and then individually invoke each of the services responsible for a context we know we need, so this is lazy context-instantiation. Ideally that sounds good, and for core it is. So long as this is good for core and it doesn't block contrib, I'll be ++.
196.2:
I'd prefer something like LazyContextRepository or LazyContextFactory. Technically, this is an awful lot like a sub-service-container. We're defining services it cares about, and then statically caching objects for use lazily. Simply "ContextRepository" is probably fine. This should definitely NOT be merged with the context handler class in any way.
196.3:
I think that ship has sailed. Solving this for contrib, ideally would be awesome, but it's too big of a problem space at this point in the game. 8.1.x?
Eclipse
Comment #201
Fabianx CreditAttribution: Fabianx for Drupal Association commented#196 looks great to me.
I think adding an optional option for enabling contrib to use it would be okay.
And I am in strong favor of keeping it generic.
Comment #202
dawehnerNo, this has been needed in order to fix some tests, see #2354889-74: Make block context faster by removing onBlock event and replace it with loading from a ContextManager
Comment #203
dsnopek+1 to moving the ContextProviders out of the 'block' module, which was done in the patch on #186! This is what I was asking for in #132 and it looks beautiful. :-) Thanks!
#196.3: Looking at D7 CTools, it appears that context plugins don't need to take any context, but relationship plugins (which are kind of a special context provider) do. So, it seems likely that contrib would probably need the ability for context providers to take both context and some amount of config. :-/ I guess, if necessary, we'll need to add this ability on in CTools for D8, since like EclipseGC said above, it's probably too late to squeak that in.
Doing some experimental patches to adapt Page Manager to work with the service from this patch would help get some perspective on this. If I have time tomorrow, I may give that a shot.
Comment #204
larowlanWorking on this again
Comment #205
catchComment #206
larowlanBut I'm not sure we're using it as we always call the ->getRunTimeContexts method - addressing that and tests as well as renaming to LazyContextRepository which is my preferred name
Comment #207
larowlanIgnore me, we only fetch those which we don't have already
Comment #208
larowlanRenamed to LazyContextRepository(Interface)
Used substr instead of str_replace for micro-optimisation
Fixed exception message and test
Fixed unit tests
Updated IS with up to date API changes, which are shrinking with each pass
Added back the needs profiling tag as we could use a fresh round in light of new changes
Comment #209
dawehnerCertainly +1 for using repository as part of the name.
Same test scenario, the numbers are even better this time.
Comment #210
larowlanhttps://www.drupal.org/node/2527840 for draft change notice
More naming changes as discussed on irc with berdir|dawehner
Comment #211
BerdirI've discussed #196 with EclipseGc.
1
His biggest concern was that the block page variant still has full control over what's happen with blocks, contexts and so on.
While my patch removes a bit of control from it, both the BlockRepository and BlockAccessControlHandler classes are still an implementation detail and block.module specific, a page_manager replacement for example simply not call them.
Given that, he is OK with moving the context stuff to block access control handler directly.
He was also very surprised to learn that block plugins do not get context assigned *ever* in HEAD. It looks like that is definitely supposed to happen, that's why BlockRepository injects ContextHandler but it is never used. We should leave it, and then his block context UI patch can then just also inject the block discovery service and get the contexts and assign them there.
2. EclipseGc was strongly against merging the handler and the manager, but he agreed manager isn't the best name. As written above already, we agreed on LazyBlockRepository and in IRC with @dawehner and @larowlan we just decided to only use that on the implementation and use BlockRepositoryInterface and context.repository as the service name.
3. Agreed with what's been said. We can keep the service out there, if someone wants to use it, great, if not, then page_manager can for example extend it and provide additional methods that are specific for it. Adding $options seems a bit weird when we have no use cases for it in core. And we have no idea if it would really work out for contrib...
Comment #212
larowlanComment #214
larowlanMissed a spot in the rename
Opened #2527866: Investigate why comment.manager service doesn't exist when comment_entity_storage_load() is fired in ConfigImportAllTest
Comment #215
larowlanComment #216
Wim LeersAnother review of mine that's likely to be annoying. Let me know if you want me to stop reviewing this. I just hope that my perspective is useful — of somebody who doesn't know the context system at all, except at a conceptual level. Contexts as a concept make total sense to me, I just have a lot of trouble understanding the naming/implementation I guess…
Nit: unnecessary change.
Too many service IDs :)
Eh "providing plugin contexts" — shouldn't that be "providing plugins with contexts"?
Perhaps also s/an interface/a service interface/? Not sure about that at all though.
This is still quite confusing IMHO.
The first line in the class docblock suggests it only cares about run-time contexts. But then the remainder says it's about all contexts.
The fact that there's a clear distinction between "run time contexts" and "configuration time contexts" sounds sensible. This is how I understood the patch so far. I.e.: my understanding:
NodeRouteContet
Yet given that understanding/world view, the documentation on
getConfigurationTimeContexts()
makes no sense at all: "for the purposes of configuration" and "all available contexts" made me go "WTF?". Reading the class docblock brings clarity: apparently "configuration time contexts" actually means "all contexts in the entire system"?This suggests I'm completely misunderstanding the system.
If a method returns all contexts in the system, why not call it
getAllContexts()
or something similarly non-ambiguous?"result" vs. "lazy".
If something manages *results*, then evaluation has already happened, so where's the lazy evaluation then?
Confusing.
Adds to my confusion from above.
Comment #217
EclipseGc CreditAttribution: EclipseGc commented@Wim
So a clarification. Runtime contexts vs Configuration has a few nuances.
This last point is why I suggested we call it "Lazy". It is perhaps not lazy in the sense that PluginCollections are lazy, but runtime contexts are only instantiated if necessary in this implementation.
Eclipse
Comment #218
dawehnerI think I found something better.
Ehm, no, context are not plugins. They provide those value objects, which then are used by plugins.
So yeah contexts are basically a maybe monad with an additional data definition.
Yeah, let's move that onto the interface.
Comment #219
Fabianx CreditAttribution: Fabianx for Drupal Association commentedTo explain #217 with an example:
It is actually really simple, because it works conceptually exactly like tokens in D7 (contrib):
There is hook_tokens() and hook_token_info().
Compare that to:
e.g.
e.g. both 1 and 2 would work.
In general this means that I can ask specifically for foo.bar, but node.node must not be computed.
That is how far I have understood it.
Comment #220
dawehnerOpened up a critical #2528178: Provide an upgrade path for blocks context IDs #2354889 (context manager) follow up.
Comment #221
BerdirWorking on this, making some renaming and other changes that we discussed.
Comment #222
BerdirA few changes, might need test updates.
* Renamed getConfigurationTimeContexts() to getAvailableContexts(). We discussed it here and I think the previous name was based on a misunderstanding by @Wim Leers. Funny enough, the docblock already used "available"
* Since RunTime now longer needs to be consistent with ConfigurationTime, I renamed that to Runtime.
* I removed the manual namespacing of the unqualified context ID's in the providers, so node.node is now just node., user.current_user is just current_user. You can see in the updated language related test that we still have language in there *three* times. That's plenty :p
* I also removed all notions of slot_name that were added and used qualified/unqualified context ID as discussed before. There's still *one* place that mentions of context_slot in core, but that's contained to local variables in a trait.
Comment #224
BerdirWrong patch.
Comment #225
EclipseGc CreditAttribution: EclipseGc commentedI don't know if we want to document this this way. The ContextHandler does a magic-name mapping if no mapping is provided, so a plugin defining a context key of 'node' would auto-map to a context in the context repository with the same key.
I've not ever been thrilled with that functionality, and it complicates the docs here.
Sorry, I guess I didn't explicitly disagree with Wim's comment on this. I feel like the previous method name was much clearer in terms of what it does. It gets context definitions available during configuration. That's what it does.
Same thing as before. We chose not to do this to prove that real mapping was actually functioning. I'm not committed to one direction or the other, but the magic mapping complicates decisions around this imo. I, for one, would prefer a key like 'route.node' or something.
Continuing to look at the key changes here, we were prefixing these by module we ideally wanted them to exist in. It's funny that now that they're in the appropriate module we lost that prefixing. I have fewer issues with this one but thought it at least worth pointing out.
Comment #226
dawehner+1 for that comment
That is a good improvement IMHO!
Let's use string[]
concrete
80 chars
Let's use runtime now
Not longer needed
Not longer needed
There is some empty $contexts = [] in here which is no longer needed
Yeah not needed either
Comment #228
BerdirWow, #228 already.
This should address that review, thanks!
I'll work on the issue summary next.
Comment #229
Fabianx CreditAttribution: Fabianx for Drupal Association commented#225:
The reason, why neither 'route.node' nor 'node.node' makes sense with the approach here anymore is that in the end the UI / code deals with:
so either
or
would have a lot of redundancy.
[ Edit: Deleted very wrong comment about the wrong service IDs that I made because of dealing with monkeys that broke out of the control room and wrecked havoc with my ErrorContainer ...]
Overall patch looks good.
Comment #230
BerdirDiscussed the review in #225 with EclipseGc.
1/3/4 is fine, because the actual context_id is '@node.node_route_context:node'. Nobody except the provider will see the short value. I've also discussed with EclipseGc that we should probably remove that default-mapping logic. It's not going to do anything ever unless you name your required context on your plugin exactly like that. And nobody will do that. But not in this issue.
2. Discussed on IRC. He can live with the name. Let's get some more feedback on that, however.
Comment #231
EclipseGc CreditAttribution: EclipseGc commentedI think we're all good to go on this issue.
Eclipse
Comment #232
tim.plunkettJust to pile on, I am also happy with this change.
RTBC +1
Comment #233
Fabianx CreditAttribution: Fabianx for Drupal Association commentedRTBC + 1 - Looks really great now!
Proposed commit message:
Comment #234
catchUpdating the commit credits to (hopefully) match #233.
Not committing yet because I need to catch up with #222 onwards, this should not stop someone else doing so if they get here first.
Comment #235
Wim LeersAnothering Annoying-Wim-Is-Asking-Annoying-Questions-Review ®.
This patch is significantly more understandable already than in prior iterations! :)
The service name omits the "lazy" adjective. I'm sure this is intentional. But does this mean that it'll ever make sense to have a non-lazy repository then?
Because if laziness is always desired, then that's an adjective that doesn't really add meaning here.
e.g. "LazyContainer" doesn't make sense because the whole point of a container is that it's lazy. If the same applies here, let's omit that adjective.
Perfect symmetry between these two interfaces' method names! Yay!
This greatly enhances understandability. It makes it immediately clear that the repository interface just returns the joined set of return values by all providers.
Perhaps documenting this on the repository interface would be a good thing, to make this also clear while navigating the code, rather than only when looking at this patch?
Inconsistent.
These are mostly consistent, but it'd be great to document what it takes to be a "fully qualified" context ID, and therefore also what an "unqualified" context ID is.
Almost consistent :) But I suspect the latter is also supposed to be keyed by context ID? That's not documented.
Inconsistent.
Typo in method reference: s/getRuntimeContext()/getRuntimeContexts()/
s/ist/list/
80 cols.
Can there also be context providers outside of the container?
Sounds a bit strange.
manager vs repository
Nice!
Comment #236
catchNot a full review, but posting for now.
Off-topic but no interface to type hint on?
I still find runtime vs.available not entirely self-documenting. Could this be getContextsWithValues() or similar?
No need for "It"
Agreed with Wim that Lazy suggests there's a non-lazy.
Could be an assert later? Or can this come from stored data?
This could use a comment as to why it's necessary.
Comment #237
alexpottWe should document this so it appears on https://api.drupal.org/api/drupal/core%21core.api.php/group/service_tag/8
@see static:: does not work - see https://api.drupal.org/api/drupal/core%21modules%21block%21src%21EventSu...
What happens if the service id does not exist or does not provide the context?
Not used.
Not used.
Slotes? Also I think we stopped using slots?
Comment #238
dawehnerWorking on it
Comment #239
dawehnerTo be clear, this is the interface of CompilerPasses and no, there is none.
Well, there is, page manager will have some of those. They know they contexts in advance, coming from page arguments.
Sure
As talked with berdir, there are usecases for missing context values, and even context objects. I adapted the documentation to take that into account.
Comment #240
Berdiryou should use {} here as well, then.
usecases should be use cases I think. Or just cases.
and *p*roviders :)
that seems complicated.
Mybe just add "The determined available contexts, ..." to the original docs?
The second , is not needed.
two spaces on second line ;)
... why the service does sometimes not exist? (to explain what the todo is actually about)
should be the message?
Comment #242
dawehnerI had a look at the entire code base, but was simply just blind.
Fair, we have more docs above.
Well, this is what we need to investigate!
This was actually not intended to be added.
Comment #243
Wim LeersThanks, looks great now. Will leave to Berdir to re-RTBC.
Two tiny nits that can be fixed upon commit:
Nit: s/context_id/context ID/
I don't think this needs to include the FQCN, but it doesn't hurt of course.
Comment #244
BerdirI meant one more space, not one less :)
Can be done on commit as well.
The @see is actually correct IMHO.
Back to @alexpott/@catch then :)
Comment #245
alexpottCommitted 794be16 and pushed to 8.0.x. Thanks!
re #243.2 well static:: does not work with api.d.o so not fixing.
Minor fixes on commit - thanks @Wim Leers and @Berdir
Comment #247
Wim LeersPublished the CR and tagged #2508884: Make contexts immutable for a reroll.
Comment #248
andypostLooks language context should live in
Core\Language
otherwise we have useless compiler passFiled follow-up to discus and quickfix #2529616: Move CurrentLanguageContext to Core\Language\ContextProvider
Comment #249
jhodgdonFixing "after" in issue summary, since this is apparently wrong (fixing to match change record, which is correct).
Comment #251
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedCreated another follow-up for migrate #2547395: Migration for block visibility on user roles in broken