Make block context faster by removing onBlock event and replace it with loading from a ContextManager

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

So, 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

I noticed similar issues in page_manager.

Indeed! :)

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

Here'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.

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

So maybe better to keep global contexts cached somewhere to not dispatch event on each region?

1. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -44,6 +46,11 @@ class BlockAccessControlHandler extends EntityAccessControlHandler implements En
   +   * The global block contexts.
   ...
   +  protected $contexts;
   
   @@ -85,18 +92,24 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
   +          if ($condition instanceof ContextAwarePluginInterface) {
   +            try {
   +              $contexts = $entity->getContexts();
   +              $this->contextHandler->applyContextMapping($condition, $contexts);
   

   If condition knows about needed contexts so it could be $entity->getNeededContexts($condition) somehow

2. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -115,4 +128,14 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
   +    if (!isset($this->contexts)) {
   +      $this->contexts = \Drupal::service('event_dispatcher')->dispatch(BlockEvents::ACTIVE_CONTEXT, new BlockContextEvent())->getContexts();
   

   probably that disatcher needs injection

Quoting IRC:

WimLeers: EclipseGc: well, blocks could do something similar to what Views does. Views calculates the cacheability of each display at save time, so that at run time, it only has to load it from config, which it loads anyway. Similarly, blocks could analyze which contexts it needs, indicate that in Block config entity at save time, and then you have that information, very cheaply.

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.

I'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

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.

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

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.

We already do that every request

Exactly!

See:

  public function getVisibleBlocksPerRegion(array $contexts) {
    // Build an array of the region names in the right order.
    $empty = array_fill_keys(array_keys($this->getRegionNames()), array());

    $full = array();
    foreach ($this->blockStorage->loadByProperties(array('theme' => $this->getTheme())) as $block_id => $block) {
      /** @var \Drupal\block\BlockInterface $block */
      // Set the contexts on the block before checking access.
      if ($block->setContexts($contexts)->access('view')) {
        $full[$block->getRegion()][$block_id] = $block;
      }
    }

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.

My 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

Yes 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.:

block_ids: [ 'header' => ['block_1', 'block_2'] ]
contexts: [ 'node' ]
conditions: ['node_type']

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?

Per #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:

What if I have 100s of contexts on a site? They will all run automatically and load everything that might apply?

I think introducing onBlock event was an architectural mistake (and hence the criticality as IMHO the only sane way is to remove the onBlock event again).

There is a language for contexts, so we know exactly what each block has for context, so a block should ask a ContextManager service for the contexts it needs (lazy loaded for all blocks together for speed reasons).

Just because SCOTCH did not go in, does not mean we can take short cuts, we have that debt and we need to solve it properly.

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

Yes 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

Over ~10ms savings with warm caches and would have to be deferred to 9.x
[snip]
Gets measurably worse with lots of contrib modules or large data sets (e.g. non-indexed queries) and would have to be deferred to a minor version

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.

re-roll

Updating issue summary to include notes from the weekly criticals meeting.

+1 to #24, #25 and #31.

Taking a look

Looks like these are already being saved in the config - note the context mapping:

uuid: 6d9d4efc-1bed-4e59-a55a-9087fd612e72
langcode: en
status: true
dependencies:
  module:
    - search
    - user
  theme:
    - bartik
id: bartik_search
theme: bartik
region: sidebar_first
weight: -1
provider: null
plugin: search_form_block
settings:
  id: search_form_block
  label: Search
  provider: search
  label_display: visible
  cache:
    max_age: -1
visibility:
  user_role:
    id: user_role
    roles:
      administrator: administrator
    negate: false
    context_mapping:
      user: user.current_user

The on block event also fires in an administrative context - see for example CurrentUserContext::onBlockAdministrativeContext

And then ConditionPluginBase::buildConfigurationForm() also calls addContextAssignmentElement which adds this element

if (count($options) > 1) {
        $assignments = $plugin->getContextMapping();
        $element[$context_slot] = [
          '#title' => $this->t('Select a @context value:', ['@context' => $context_slot]),
          '#type' => 'select',
          '#options' => $options,
          '#required' => $definition->isRequired(),
          '#default_value' => !empty($assignments[$context_slot]) ? $assignments[$context_slot] : '',
        ];
      }

So 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?

Convert the "context providers" (currently the subclasses of \Drupal\block\EventSubscriber\BlockContextSubscriberBase to a tagged service, similar to cache contexts,

Yeah 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

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

Any reason why we can't store the service ID as well as part of the context mapping?

visibility:
  user_role:
    id: user_role
    roles:
      administrator: administrator
    negate: false
    context_mapping:
-      user: user.current_user
+     user: user.current_user@block.current_user_context

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.

Possibly upgrade path blocker for config changes

All I have time for today, WIP

@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

kicking some more

More WIP, out of time - implements most of 35 but haven't looked at tests manual or otherwise

Adding API changes

As 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?

This minor nudge, makes BlockHtmlTest pass.

+++ b/core/modules/block/src/BlockAccessControlHandler.php
@@ -85,18 +92,24 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
+        $contexts = $entity->getContexts();
+        if (!isset($contexts)) {
+          $contexts = $this->getContexts();
+        }
...
+              $contexts = $entity->getContexts();

That looks broken

This should be ready for tests, page renders ok, only needed contexts are loaded.

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

+++ b/core/modules/block/src/BlockAccessControlHandler.php
@@ -85,18 +92,20 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
+      if ($entity->getVisibilityConditions()) {
+        foreach ($entity->getVisibilityConditions() as $condition_id => $condition) {

Why not make

 if ($visibility_conditions = $entity->getVisibilityConditions()) {
foreach($visibility_conditions as ...)?

@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

Overall: Fantastic work!

Some comments:

1. +++ b/core/modules/block/block.services.yml
   @@ -12,16 +12,29 @@ services:
   +    class: Drupal\block\BlockContextManager
   +    tags:
   +      - { name: service_collector, tag: block_context_provider, call: addContextProvider }
   

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

2. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -85,18 +92,20 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
   +      if ($entity->getVisibilityConditions()) {
   +        foreach ($entity->getVisibilityConditions() as $condition_id => $condition) {
   

   I agree with Jibran, that we should store the result.

3. +++ b/core/modules/block/src/BlockContextManager.php
   @@ -0,0 +1,40 @@
   +  public function getContextProvider($id) {
   +    if (isset($this->contextProviders[$id])) {
   +      return $this->contextProviders[$id];
   +    }
   +    throw new \InvalidArgumentException(sprintf('You tried to request a non-existent context-provider (%s)', $id));
   +  }
   

   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?

4. +++ b/core/modules/block/src/EventSubscriber/CurrentLanguageContext.php
   @@ -48,7 +48,7 @@ public function onBlockActiveContext(BlockContextEvent $event) {
   -        $event->setContext('language.' . $type_key, $context);
   +        $event->setContext('language.' . $type_key . '@block.current_language_context', $context);
   
   +++ b/core/modules/block/src/EventSubscriber/CurrentUserContext.php
   @@ -56,7 +56,7 @@ public function onBlockActiveContext(BlockContextEvent $event) {
   -    $event->setContext('user.current_user', $context);
   +    $event->setContext('user.current_user@block.current_user_context', $context);
   

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

5. +++ b/core/modules/block/src/EventSubscriber/UserRouteContext.php
   @@ -0,0 +1,61 @@
   +      $event->setContext('user.user', $context);
   ...
   +    $event->setContext('user.user', $context);
   

   Is this not missing the new '@' notation?

6. +++ b/core/modules/block/src/Plugin/DisplayVariant/BlockPageVariant.php
   @@ -128,9 +135,24 @@ public function build() {
   +    $block_event = new BlockContextEvent();
   +    foreach ($active_blocks as $block_id => $block) {
   +      /* @var \Drupal\block\BlockInterface $block */
   +      foreach ($block->getVisibilityConditions() as $condition) {
   +        if ($condition instanceof ContextAwarePluginInterface) {
   +          /* @var \Drupal\Core\Plugin\ContextAwarePluginInterface $condition */
   +          $mapping = $condition->getContextMapping();
   +          foreach ($mapping as $context_slot => $context_provider_id) {
   +            list(, $context_service_id) = explode('@', $context_provider_id);
   +            $this->contextManager->getContextProvider($context_service_id)->onBlockActiveContext($block_event);
   +          }
   +        }
   

   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.

Fixes unit tests, doesn't touch #58 or functional tests.
Out of time for today.
Time poor.

1. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -85,18 +92,20 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
   +      if ($entity->getVisibilityConditions()) {
   

   Why? This should return empty array if there are no conditions, and foreach empty array still won't perform any of the subsequent actions. ??

2. +++ b/core/modules/block/src/BlockRepositoryInterface.php
   @@ -19,6 +21,14 @@
   +   * @return array
   

   can we get a better type hint return here?

3. +++ b/core/modules/block/src/Plugin/DisplayVariant/BlockPageVariant.php
   @@ -128,9 +135,24 @@ public function build() {
   +            $this->contextManager->getContextProvider($context_service_id)->onBlockActiveContext($block_event);
   

   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

More 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:

• Fix remaining fails
• #62
• #58

This 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

#65:

The tagged context services that are ALSO event subscribers are just weird to me.

+1

+++ b/core/modules/block/src/EventSubscriber/NodeRouteContext.php
@@ -40,16 +41,14 @@ public function __construct(RouteMatchInterface $route_match) {
-      $context = new Context(new ContextDefinition('entity:node'));
-      $context->setContextValue(Node::create(array('type' => $node_type->id())));
+      $context = new Context(new ContextDefinition('entity:node'), EntityAdapter::createFromEntity(Node::create(array('type' => $node_type->id()))));
...
     }

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

#68: good call.

more test fixes
still to do

• #62
• #58
• #67
• #70
• #71

Removes 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).

still to do

• #62
• #58
• #67
• #70
• #71

whoops

Fixes #70 and #71 (parts of 70 were fixed in #74)

Still to do:

• #62
• #58
• #67

+++ b/core/modules/comment/comment.module
@@ -339,7 +339,7 @@ function comment_entity_storage_load($entities, $entity_type) {
-  if (!\Drupal::service('comment.manager')->getFields($entity_type)) {
+  if (!\Drupal::hasService('comment.manager') || !\Drupal::service('comment.manager')->getFields($entity_type)) {

how comment manager could be not registered if comment module enabled?

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

please elaborate

Do we need to fix #2495087: comment_entity_storage_load() is too expensive on cold caches and postpone this issue on that?

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

That 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

I'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.

Fixes #62

Still to do:

• #58
• #67

More tomorrow

#67 was a different approach, so just #58 to go, working on that now

Starts 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

missed an easy one

This should be last of feedback from bot, now for human feedback :)

Overall looks really great! Just some questions and remarks:

1. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -44,6 +46,11 @@ class BlockAccessControlHandler extends EntityAccessControlHandler implements En
      /**
   +   * The global block contexts.
   +   */
   +  protected $contexts;
   

   Unused

2. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -85,12 +92,11 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
   -      $contexts = $entity->getContexts();
   ...
   -            $this->contextHandler->applyContextMapping($condition, $contexts);
   +            $this->contextHandler->applyContextMapping($condition, $entity->getContexts());
   

   Is that change necessary?

   Does it help performance?

   Would that not call getContexts() for each condition?

3. +++ b/core/modules/block/src/BlockContextManager.php
   @@ -0,0 +1,64 @@
   +  public function getContexts(array $context_ids = NULL) {
   ...
   +        $this->container->get($id)->onBlockAdministrativeContext($event);
   ...
   +        $this->container->get($id)->onBlockActiveContext($event);
   
   +++ b/core/modules/block/src/BlockContextManagerInterface.php
   @@ -0,0 +1,26 @@
   +   * @param string[]|NULL $context_ids
   +   *   (optional) Context provider IDs to get contexts for. If none are passed
   +   *   gets all contexts. Should only be used in an administrative context.
   

   I think this should explain clearer when it returns what.

   However my suggestion is have two separate methods:

   - getContexts()
   - getAdministrativeContexts()

4. +++ b/core/modules/block/src/BlockForm.php
   @@ -117,7 +124,7 @@ public function form(array $form, FormStateInterface $form_state) {
   -    $form_state->setTemporaryValue('gathered_contexts', $this->dispatcher->dispatch(BlockEvents::ADMINISTRATIVE_CONTEXT, new BlockContextEvent())->getContexts());
   +    $form_state->setTemporaryValue('gathered_contexts', $this->contextManager->getContexts(NULL));
   

   Would then be:

   ->getAdministrativeContexts()

   That would feel good.

5. +++ b/core/modules/block/src/BlockRepository.php
   @@ -49,15 +49,21 @@ public function __construct(EntityManagerInterface $entity_manager, ThemeManager
   +  public function getVisibleBlocksPerRegion(array $blocks, array $contexts) {
   

   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.

6. +++ b/core/modules/block/src/EventSubscriber/BlockContextSubscriberBase.php
   @@ -14,13 +15,12 @@
        $events[BlockEvents::ADMINISTRATIVE_CONTEXT][] = 'onBlockAdministrativeContext';
   

   Are we using that event still? (It did seem like BlockContextManager now took over that task).

7. +++ b/core/modules/block/src/EventSubscriber/CurrentLanguageContext.php
   @@ -48,7 +48,7 @@ public function onBlockActiveContext(BlockContextEvent $event) {
   +        $event->setContext('language.' . $type_key . '@block.current_language_context', $context);
   

   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)

8. +++ b/core/modules/block/src/Plugin/DisplayVariant/BlockPageVariant.php
   @@ -128,9 +135,29 @@ public function build() {
   +      foreach ($block->getVisibilityConditions() as $condition) {
   

   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.

9. +++ b/core/modules/block/src/Plugin/DisplayVariant/BlockPageVariant.php
   @@ -128,9 +135,29 @@ public function build() {
   +            list(, $context_service_id) = explode('@', $context_provider_id);
   

   nit - simple hardening explode('@', $context_provider_id, 2);

larowlan
timplunkett: hi wanted to ask you about BlockEvent and page manager
7:35
timplunkett
yes
7:35
larowlan
timplunkett: https://www.drupal.org/node/2354889
7:35
Druplicon
https://www.drupal.org/node/2354889 => Make block context faster by removing onBlock event and replace it with loading from a BlockContextManager #2354889: Make block context faster by removing onBlock event and replace it with loading from a ContextManager => 95 comments, 18 IRC mentions
7:35
larowlan
timplunkett: in there moving from event subscribers to tagged services, block context provider manager
timplunkett: does page manager need them tagged as event subscribers/use the event system?
7:36
timplunkett
larowlan: page_manager has its own version of that code. haven't decided if we'll follow core's move
7:36
timplunkett
nope
7:36
larowlan
timplunkett: if not any dramas with removing the event|blocksubscriberbase and adding a new interface for context provider services?
timplunkett: does page manager require block? would the code be more page manager friendly if the manager wasn't part of block module
7:37
timplunkett
larowlan: http://cgit.drupalcode.org/page_manager/tree/src/EventSubscriber/CurrentUserContext.php
7:37
timplunkett
we have a whole other thing for this, because we have PageManagerContextEvent while core uses BlockContextEvent
7:37
timplunkett
larowlan: we need the block plugins, we don't need these contexts
7:38
larowlan
timplunkett: right so page manager could install without block?
7:38
timplunkett
well it'd be all but useless
we have a dependency, i don't know if it matters
7:38
larowlan
timplunkett: do you think it would make sense to try and align the two so there is one lot of code
7:39
timplunkett
larowlan: see https://www.drupal.org/node/2511568
7:39
Druplicon
https://www.drupal.org/node/2511568 => Create "context stack" service where available contexts can be registered #2511568: Create "context stack" service where available contexts can be registered => 0 comments, 2 IRC mentions
7:41
larowlan
timplunkett: sounds a lot like BlockContextProvider
timplunkett++
7:41
timplunkett
larowlan: right but has nothing to do with blocks
7:42
larowlan
timplunkett: yep, can make it more generic
timplunkett: but should it live in core or block module?
7:43
timplunkett
larowlan: would be cool if it was generic enough to be in Drupal\Core
7:43
plach [~plach@host-94-196-68-109.arq1.ldn.uk.sharedband.net] entered the room.
7:43
plach left the room (quit: Client Quit).
7:43
larowlan
I think that can be done

Didn't really read any comments yet and not a full review, so just ignore what has been discussed already...

1. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -85,12 +92,11 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
        }
        else {
   -      $contexts = $entity->getContexts();
          $conditions = [];
          foreach ($entity->getVisibilityConditions() as $condition_id => $condition) {
            if ($condition instanceof ContextAwarePluginInterface) {
              try {
   -            $this->contextHandler->applyContextMapping($condition, $contexts);
   +            $this->contextHandler->applyContextMapping($condition, $entity->getContexts());
              }
              catch (ContextException $e) {
   

   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.

2. +++ b/core/modules/block/src/BlockContextManager.php
   @@ -0,0 +1,64 @@
   +  public function __construct(ContainerInterface $container, array $contexts) {
   +    $this->container = $container;
   +    $this->contextProviderIds = $contexts;
   

   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?

3. +++ b/core/modules/block/src/BlockContextManager.php
   @@ -0,0 +1,64 @@
   +   */
   +  public function getContexts(array $context_ids = NULL) {
   +    $event = new BlockContextEvent();
   +    if (!$context_ids) {
   +      foreach ($this->contextProviderIds as $id) {
   +        $this->container->get($id)->onBlockAdministrativeContext($event);
   +      }
   +    }
   +    else {
   +      foreach ($context_ids as $id) {
   +        $this->container->get($id)->onBlockActiveContext($event);
   +      }
   +    }
   

   Wouldn't it be easier to have two methods for this and one where $context_ids is rquired?

4. +++ b/core/modules/block/src/BlockRepositoryInterface.php
   @@ -19,6 +21,14 @@
       */
   -  public function getVisibleBlocksPerRegion(array $contexts);
   +  public function getVisibleBlocksPerRegion(array $blocks, array $contexts);
   +
   

   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?

5. +++ b/core/modules/block/src/EventSubscriber/BlockContextSubscriberBase.php
   @@ -14,13 +15,12 @@
      /**
       * {@inheritdoc}
       */
      public static function getSubscribedEvents() {
   -    $events[BlockEvents::ACTIVE_CONTEXT][] = 'onBlockActiveContext';
        $events[BlockEvents::ADMINISTRATIVE_CONTEXT][] = 'onBlockAdministrativeContext';
        return $events;
      }
   

   seems like we can remove this completely now?

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

There 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

I think I've addressed comments from #95, #98 and #99 and should have fixed the tests too.

1. +++ b/core/modules/block/src/BlockContextManager.php
   @@ -48,17 +48,18 @@
   -    if (!$context_ids) {
   -      foreach ($this->contexts as $id) {
   -        $this->container->get($id)->onBlockAdministrativeContext($event);
   -      }
   ...
   +  public function getAdministrativeContexts() {
   +    return $this->getContexts($this->contexts);
   +  }
   

   This is wrong.

   This needs to call onBlockAdministrativeContext() and can't use getContexts().

2. +++ b/core/modules/block/src/Plugin/DisplayVariant/BlockPageVariant.php
   @@ -147,17 +148,15 @@
   +      $contexts += $block->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.

picking this back up

This:
* 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

Only 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!

The only clunky bit left here is BlockPageVariant needing to gather the service IDs with explode

Removes the clunkiness of BlockPageVariant needing to know that the mappings contain @

Un-assigning so others can work on it during sprint

We still need to profile against OP

And need to list API changes in OP

#111 looks truly fantastic, the interdiff also makes a lot of sense.

Now 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?

Fixes some tests

1. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextStack.php
   @@ -0,0 +1,52 @@
   +    /* @var \Drupal\Core\Plugin\Context\ContextCollectionInterface $context_stack */
   +    foreach ($collections as $context_stack) {
   +      foreach ($context_stack->getContexts() as $context_id => $context) {
   +        $this->contexts[$context_id . '@' . $context_stack->getServiceId()] = $context;
   

   s/context_stack/collection/g

2. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextStackManager.php
   @@ -0,0 +1,79 @@
   +   * Constructs a CacheContextsManager object.
   

   ContextStackManager

3. +++ b/core/modules/block/block.services.yml
   @@ -8,20 +8,20 @@ services:
   -      - { name: 'event_subscriber' }
   +      - { name: 'context_provider' }
   

   Love it!

4. +++ b/core/modules/block/src/ContextProvider/CurrentLanguageContext.php
   @@ -54,7 +55,7 @@ public function onBlockActiveContext(BlockContextEvent $event) {
   +        $collection->setContext('language.' . $type_key, $context);
   

   Very nice!

5. +++ b/core/modules/block/src/Plugin/DisplayVariant/BlockPageVariant.php
   @@ -129,10 +136,25 @@ public function build() {
   +      foreach ($block->getVisibilityConditions() as $condition) {
   

   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.

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

more test fixes

+++ b/core/lib/Drupal/Core/Plugin/Context/ContextStack.php
@@ -0,0 +1,52 @@
+class ContextStack implements ContextStackInterface {

It's confusing to me, but this class doesn't actually have the semantics of a stack?

Benjy, agree that's why I think plugin manager makes sense

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

@FabianX

This is missing the other contexts that can be on blocks, to e.g. variate by a given node.

Can you elaborate? Are you saying I should be $contexts += $block->getContexts();?

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

A look at this again this morning. It's still awesome! :-)

One quick comment - can we move these out of the 'block' module:

• \Drupal\block\ContextProvider\CurrentUserContext
• \Drupal\block\ContextProvider\CurrentLanguageContext
• \Drupal\block\ContextProvider\NodeRouteContext

I could definitely see these contexts being used by Rules, which has nothing to do with blocks.

Could 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!

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

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

+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).

Discussed #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.

Well, I still think its a separate discussion not needed to fix the critical issue.

+1

I don't think contexts need to work on a low bootstrap level, this is fine as follow-up.

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

1. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextCollection.php
   @@ -0,0 +1,61 @@
   + * Contains \Drupal\Core\Plugin\Context\ContextCollection.
   ...
   +   * The service which provided the contexts.
   

   This seems like a contradiction? (service vs. plugin)

2. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextCollection.php
   @@ -0,0 +1,61 @@
   + * Collects global active contexts for a given service.
   

   What does "global active" mean?

   "given service" -> what kind of service?

3. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextCollection.php
   @@ -0,0 +1,61 @@
   +   * The array of available contexts.
   

   s/array/set/ ?

   "[…] collected from all context provider services"

4. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextCollection.php
   @@ -0,0 +1,61 @@
   +   * Constructs a new context stack object.
   

   s/stack/collection/

5. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextProviderInterface.php
   @@ -0,0 +1,63 @@
   +   * Determines the available run-time contexts.
   ...
   +  public function getActiveContexts(ContextCollection $collection);
   

   "run-time" vs. "active".

   I think "run-time" is much clearer. Can't we name the method accordingly?

6. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextProviderInterface.php
   @@ -0,0 +1,63 @@
   +   * Determines the available configuration-time contexts.
   ...
   +  public function getAdministrativeContexts(ContextCollection $collection);
   

   Same here with "configuration-time" versus "administrative". Configuration-time is much clearer.

7. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextStack.php
   @@ -0,0 +1,52 @@
   +/**
   + * Defines a class for combining ContextCollection objects.
   + */
   +class ContextStack implements ContextStackInterface {
   ...
   +        $this->contexts[$context_id . '@' . $context_stack->getServiceId()] = $context;
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getContexts() {
   +    return $this->contexts;
   +  }
   

   This doesn't seem to be a stack data structure at all?

8. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextStackInterface.php
   @@ -0,0 +1,33 @@
   +interface ContextStackInterface {
   +
   +  /**
   +   * Gets the active context objects.
   

   So context stacks contain context collections, and those collections apparently only contain active (== run-time) context? Then shouldn't ContextCollection be called ActiveContextCollection?

9. +++ b/core/modules/block/src/BlockForm.php
   @@ -69,23 +70,30 @@ class BlockForm extends EntityForm {
   +   * The context manager service.
   ...
   +   * @var \Drupal\Core\Plugin\Context\ContextStackManagerInterface
   ...
   +  protected $contextStackManager;
   ...
   +   * @param \Drupal\Core\Plugin\Context\ContextStackManagerInterface $context_manager
   +   *   The block context manager service.
   

   Confusingly inconsistent.

10. +++ b/core/modules/block/src/BlockRepositoryInterface.php
    similarity index 73%
    rename from core/modules/block/src/EventSubscriber/CurrentLanguageContext.php
    
    rename from core/modules/block/src/EventSubscriber/CurrentLanguageContext.php
    rename to core/modules/block/src/ContextProvider/CurrentLanguageContext.php
    
    +++ b/core/modules/block/src/ContextProvider/CurrentLanguageContext.php
    similarity index 73%
    rename from core/modules/block/src/EventSubscriber/CurrentUserContext.php
    
    rename from core/modules/block/src/EventSubscriber/CurrentUserContext.php
    rename to core/modules/block/src/ContextProvider/CurrentUserContext.php
    
    +++ b/core/modules/block/src/ContextProvider/CurrentUserContext.php
    similarity index 72%
    rename from core/modules/block/src/EventSubscriber/NodeRouteContext.php
    
    rename from core/modules/block/src/EventSubscriber/NodeRouteContext.php
    rename to core/modules/block/src/ContextProvider/NodeRouteContext.php
    

    These all look wonderful :)

#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:

  return clone $this->container->get($serviceId);

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:

foreach ($this->contextPluginManager->getDefinitions() as $plugin_id => $definition) {
  $context_plugin = $this->contextPluginManager->createInstance($plugin_id); // instead of container->get(...)
  $active_contexts = $context_plugin->getAdministrativeContexts();
}

and

getContexts() would then do:

foreach ($context_ids as $plugin_id ) {
  $context_plugin = $this->contextPluginManager->createInstance($plugin_id); // instead of container->get(...)
  $active_contexts = $context_plugin->getActiveContexts();
}

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.

+++ b/core/lib/Drupal/Core/Plugin/Context/ContextStackManager.php
@@ -0,0 +1,79 @@
+  public function getContexts(array $context_ids) {
+    $stack = new ContextStack([]);
+    foreach ($context_ids as $id) {
+      // The IDs have been passed in {context_slot_name}@{service_id} format.
+      if (strpos($id, '@') !== FALSE) {
+        list(, $id) = explode('@', $id, 2);
+      }
+      $collection = new ContextCollection($id);
+      $this->container->get($id)->getActiveContexts($collection);
+      $stack->addContextCollection($collection);
+    }
+
+    return $stack->getContexts();
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function getAdministrativeContexts() {
+    $stack = new ContextStack([]);
+    foreach ($this->contexts as $id) {
+      $collection = new ContextCollection($id);
+      $this->container->get($id)->getAdministrativeContexts($collection);
+      $stack->addContextCollection($collection);
+    }
+
+    return $stack->getContexts();
+  }

There 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 :)

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

Created #2525914: Covert ContextStackManager to plugin manager for plugin manager discussion.

prodding some more

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

missed some spots

Thanks @dawehner - great feedback
Unassigning in case someone wants to work on it at the sprint
If not, will pick it up in the morning

#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?

+++ b/core/lib/Drupal/Core/Plugin/Context/ContextManager.php
@@ -76,2 +75,18 @@
+  protected function getContexts() {
+    if (!$this->contexts) {
+      foreach (array_keys($container->findTaggedServiceIds('context_provider')) as $id) {
+        $this->contexts[] = $id;
+      }
+    }
+
+    return $this->contexts;
+  }

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

Didn't know that is possible, thanks!

Uhm,

That did not work ...

But this should (see RegisterLazyRouteEnhancers):

    $container
      ->getDefinition('context.manager')
      ->addArgument($context_ids);

Can 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?

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

Ready to take the baton back if you're about for a handover

tag

More cleanup
Another test
Removes ContextCollection as we don't need it anymore either

forgot to merge

So if BlockPageVariant fatals, turns out lots of things break :)

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

1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/ContextProvidersPass.php
   @@ -0,0 +1,33 @@
   + * Adds the context providers into the context manager.
   ...
   +   * Collects the context providers into the context_providers parameter.
   

   Nit: s/into/to/ ?

2. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextManager.php
   @@ -0,0 +1,98 @@
   +/**
   + *
   

   Nit: extraneous \n.

3. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextManager.php
   @@ -0,0 +1,98 @@
   +   * An array of available context providers service IDs.
   ...
   +   *   An array of the available cache context service IDs.
   

   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?

4. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextManager.php
   @@ -0,0 +1,98 @@
   +   *   Context provider IDs.
   ...
   +  protected $contextServiceIDs = [];
   ...
   +  public function __construct(ContainerInterface $container, array $context_service_ids) {
   

   Inconsistent.

5. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextManager.php
   @@ -0,0 +1,98 @@
   +    // First collect what context slots are needed by which context provider.
   

   First collect which contexts slots need which context providers.

   Better:

   Create a map of context providers (service IDs) to context slot names.

6. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextManager.php
   @@ -0,0 +1,98 @@
   +    // Call to them and add the results to the context collection.
   

   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.

7. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextManagerInterface.php
   @@ -0,0 +1,35 @@
   + * Manages available contexts and collects context values on runtime.
   

   s/on/at/

8. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextManagerInterface.php
   @@ -0,0 +1,35 @@
   +   * Gets run time context provider objects for given context IDs.
   ...
   +   *   The runtime contexts.
   

   "run time" vs. "runtime"

   Elsewhere: "run-time".

   Needs tidying up.

9. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextManagerInterface.php
   @@ -0,0 +1,35 @@
   +   * Gets all context provider objects for the purposes of configuration.
   

   Inconsistent with the docs of getRunTimeContexts().

10. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextManagerInterface.php
    @@ -0,0 +1,35 @@
    +   *   The runtime contexts.
    

    s/runtime/configuration time/

11. +++ b/core/modules/block/src/BlockForm.php
    @@ -69,23 +69,30 @@ class BlockForm extends EntityForm {
    +   *   The block context manager service.
    

    s/block//

12. +++ b/core/modules/block/src/BlockRepositoryInterface.php
    @@ -22,6 +24,14 @@
    -  public function getVisibleBlocksPerRegion(array $contexts, array &$cacheable_metadata = []);
    +  public function getVisibleBlocksPerRegion(array $blocks, array $contexts, array &$cacheable_metadata = []);
    +
    +  /**
    +   * Gets the active blocks as block entities for the current theme.
    +   *
    +   * @return \Drupal\block\BlockInterface[]
    +   *   Array of block entities keyed by block ID.
    +   */
    +  public function getActiveBlocks();
    

    Interesting that both the existing method has to change and a new method is necessary.

13. +++ b/core/modules/block/src/ContextProvider/CurrentLanguageContext.php
    @@ -41,10 +42,20 @@ public function __construct(LanguageManagerInterface $language_manager) {
    -  public function onBlockActiveContext(BlockContextEvent $event) {
    +  public function getRunTimeContexts(array $context_slot_names) {
    

    No longer coupled to blocks, and a hundred times clearer what it actually means ("active" is so vague in comparison) — yay!

14. +++ b/core/modules/block/src/ContextProvider/CurrentLanguageContext.php
    @@ -41,10 +42,20 @@ public function __construct(LanguageManagerInterface $language_manager) {
    +    if ($context_slot_names) {
    +      foreach ($context_slot_names as $context_slot_name) {
    +        if (array_search(str_replace('language.', '', $context_slot_name), $language_types) === FALSE) {
    +          unset($language_types[str_replace('language.', '', $context_slot_name)]);
    +        }
    +      }
    +    }
    

    I have no idea what this is doing.

15. +++ b/core/modules/block/src/Plugin/DisplayVariant/BlockPageVariant.php
    @@ -189,13 +194,30 @@ public function build() {
    +   * Retrieves the required contexts for active blocks.
    

    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?

16. +++ b/core/modules/block/src/Plugin/DisplayVariant/BlockPageVariant.php
    @@ -189,13 +194,30 @@ public function build() {
    +   *   Currently active blocks.
    

    What is the meaning of "currently" here? Can during the same request a different set of blocks become active?

17. +++ b/core/modules/block/tests/src/Unit/BlockFormTest.php
    @@ -60,6 +53,14 @@ class BlockFormTest extends UnitTestCase {
    +  protected $contextManager;
    +
    +
    +  /**
    

    Nit: 2 \n, should be 1.

18. +++ b/core/tests/Drupal/Tests/Core/Plugin/Context/ContextManagerTest.php
    @@ -0,0 +1,112 @@
    +   * Setups contexts and context providers.
    

    Nit: s/Setups/Sets up/

19. +++ b/core/tests/Drupal/Tests/Core/Plugin/Context/ContextManagerTest.php
    @@ -0,0 +1,112 @@
    +   *   An array of setup contexts.
    

    Nit: s/setup/set up/