Conditions / context system does not allow for multiple configurable contexts, eg. language types

This issue hits on a bunch of things that are currently wrong with our approach to contexts/blocks and conditions. I could attempt to solve them all, but I'm just trying to lay groundwork here. I've not updated tests, I want to see what fails and take it from there. I'm also going to make a whole bunch of comments on my own patch after I post it, so here we go.

Eclipse

1. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -187,7 +187,7 @@ public function access(AccountInterface $account) {
   +    $this->eventDispatcher()->dispatch(BlockEvents::ACTIVE_CONTEXT, new BlockConditionContextEvent($conditions));
   

   Currently calling to the BlockEvents class in block module, so we need to fix this, however I really do think that the BlockEvents class belongs to blocks module, so we need to properly untangle this which probably means this needs to be in the entity, not the plugin. That's just a hunch.

2. +++ b/core/lib/Drupal/Core/Condition/ConditionPluginBag.php
   @@ -73,4 +75,25 @@ public function getConditionContexts() {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function initializePlugin($instance_id) {
   +    $configuration = isset($this->configurations[$instance_id]) ? $this->configurations[$instance_id] : array();
   +    if (!isset($configuration[$this->pluginKey])) {
   +      throw new PluginNotFoundException($instance_id);
   +    }
   +    $instance = $this->manager->createInstance($configuration[$this->pluginKey], $configuration);
   +    // If there are contexts configured, extract each of them from the
   +    // condition contexts and set them on the plugin.
   +    if ($instance instanceof ContextAwarePluginInterface && isset($configuration['contexts'])) {
   +      foreach ($configuration['contexts'] as $context_id => $value) {
   +        if (isset($this->getConditionContexts()[$value])) {
   +          $instance->setContext($context_id, $this->getConditionContexts()[$value]);
   +        }
   +      }
   +    }
   +    $this->set($instance_id, $instance);
   +  }
   +
   

   Most of the special things about this PluginBag need an interface & trait so that others (BlockPluginBag for instance) can make use of them. It's important to keep this sort of logic at the PluginBag level because it makes it possible for different implementations to solve context requirements in different ways.

3. +++ b/core/lib/Drupal/Core/Condition/ConditionPluginBase.php
   @@ -41,6 +41,9 @@ public function isNegated() {
   +    $context_form = [];
   +    $context_form['contexts']['#weight'] = -100;
   +    $form += $this->getContextForm($context_form, $form_state);
   

   meh. It works.

4. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php
   @@ -120,6 +121,9 @@ public function setDataType($data_type) {
   +    if ($this->label instanceof Translation) {
   +      return (string) $this->label->get();
   +    }
   

   Must be missing a test somewhere because the intent was for this to always return a string. Going to have to investigate further.

5. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php
   @@ -49,4 +50,58 @@ public function setContext($name, ComponentContextInterface $context) {
   +  /**
   +   * Return the form array for picking relevant context of this plugin.
   +   *
   +   * @param array $form
   +   * @param FormStateInterface $form_state
   +   * @return array
   +   */
   +  protected function getContextForm(array $form, FormStateInterface $form_state) {
   +    /**
   +     * @var \Drupal\Core\Plugin\Context\ContextHandlerInterface $handler
   +     */
   +    $handler = \Drupal::service('context.handler');
   +    $contexts = $form_state->get('#contexts');
   +    /**
   +     * @var \Drupal\Component\Plugin\Context\ContextDefinitionInterface $context_definition
   +     */
   +    if ($this->getContextDefinitions()) {
   +      foreach ($this->getContextDefinitions() as $name => $context_definition) {
   +        $options = array();
   +        $form['contexts']['#tree'] = TRUE;
   +        foreach ($handler->getMatchingContexts($contexts, $context_definition) as $context_id => $context) {
   +          $options[$context_id] = $context->getContextDefinition()->getLabel();
   +          $form['contexts'][$name] = [
   +            '#type' => 'value',
   +            '#value' => $context_id,
   +          ];
   +        }
   +        if (count($options) > 1) {
   +          $form['contexts'][$name] = array(
   +            '#title' => $context_definition->getLabel(),
   +            '#type' => 'select',
   +            '#required' => $context_definition->isRequired(),
   +            '#options' => $options,
   +            '#default_value' => isset($this->configuration['contexts']) ? $this->configuration['contexts'][$name] : '',
   +          );
   +        }
   +      }
   +    }
   +    return $form;
   +  }
   +
   +  /**
   +   * Extract context settings from form values and place them in configuration.
   +   *
   +   * @param array $form
   +   * @param FormStateInterface $form_state
   +   */
   +  protected function saveContextConfiguration(array &$form, FormStateInterface $form_state) {
   +    $contexts = $form_state->getValue('contexts');
   +    if ($contexts) {
   +      $this->configuration['contexts'] = $contexts;
   +    }
   +  }
   +
   

   This abstracts the logic for getting context form components and context configuration into a common plugin class. With the exception of the custom stuff in the ConditionPluginBag, blocks could be context aware (including form elements) today. That's pretty exciting.

6. +++ b/core/modules/block/src/Event/BlockEvents.php
   @@ -18,6 +18,14 @@
   +  const ACTIVE_CONTEXT = 'block.active_context';
   ...
   +  const ADMINISTRATIVE_CONTEXT = 'block.administrative_context';
   

   ok, so this is actually run-time and config-time context availability. There's a big big difference. For instance:

   You'll never have a node available at config time because you're not on a node route. You have to have different events for this.

7. +++ b/core/modules/block/src/EventSubscriber/NodeRouteContext.php
   @@ -53,4 +54,13 @@ protected function determineBlockContext() {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function onBlockAdministrativeContext(BlockConditionContextEvent $event) {
   +    $this->conditions = $event->getConditions();
   +    $context = new Context(new ContextDefinition('entity:node'));
   +    $this->addContext('node', $context);
   +  }
   +
   

   Most Context providing event subscribers will just call their determineBlockContext() method, but as cited above, nodes have to be done by hand. This is super cool though because it means that an new subscriber could be introduce that exposes nodes in some other way, and the form elements for that would just be autogenerated and the contexts appropriately passed. YAY!

Hopefully this self-review explains what I did and why. I'd like to take this further, but it seems like it should be follow-up territory.

Eclipse

I bet this is going to blow up, I probably needed to include some config updates. Let's see what happens though.

Eclipse

Ok, I _THINK_ this should pass tests. I made a couple changes that are questionable here, primarily the schema change for condition plugins. Tim Plunkett rightly points out that it may already be in use under the old name, so if we decide to change it, we'll just need to update the code to match the old name.

The other thing here is that the ConditionFormTest is probably going to need some additional tests to cover various context availability. I've not even attempted to consider that yet. Likewise we need to double check all the Condition tests, but I'm just trying to get the stuff that absolutely failed previously passing and then I'll make another iteration on these things.

Eclipse

oops

Agreed on points 3 & 4.

Point 1.)

The reason this was moved is because core has a very specific (and not particularly flexible) methodology for managing available contexts. We couldn't embed this as part of the block itself because we don't want to inherit that implementation. This allows us to cleanly separate the assumptions of core from the desired reusability of both blocks and conditions by embedding these assumptions in a core module instead of porting this rather limited approach to contexts into a Component.

Point 2.)
Empty contexts are for use during administration. Nodes are a great example of this since we won't ever have a node available from the URL when on the blocks UI, but we know one can be available, so we provide an empty one during administration time so that we can properly configure the plugin. (The system programmatically configures these when only one is available) In the case of multiple contexts, obviously language is a great example. If we want to test block visibility against Content Language or Interface language, we need to pick which we want to test. Since there are certain scenarios where these are NOT always the same, you may find that custom administrative blocks are useful to your foreign language content administrators and this is very different from the use case of swapping blocks based on content language.

Hopefully that is fairly logical and explains the need for these things appropriate.

Eclipse

Oh, on point 4... This is completely unnecessary any longer (the specific code) now we need to pass a language to the plugin as context so that it can properly test. This may require us to set the context in the configuration to get proper coverage, but I didn't want to dig into this yet. The previous patch is just trying to get the existing tests to pass, we should probably make up a batch of tests that hit our coverage properly and do the whole FAIL/PASS dual patch thing. I'll try to take care of that before Amsterdam, but a lot of stuff happening right now.

Eclipse

1. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -308,7 +308,7 @@ condition.plugin:
   -    context_mapping:
   +    contexts:
   

   \Drupal\Core\Plugin\Context\ContextHandler uses context_mapping

2. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php
   @@ -49,4 +50,58 @@ public function setContext($name, ComponentContextInterface $context) {
   +    $contexts = $form_state->get('#contexts');
   
   +++ b/core/modules/block/src/BlockForm.php
   @@ -65,6 +67,56 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $form_state->set('#contexts', $contexts);
   
   +++ b/core/modules/system/tests/modules/condition_test/src/FormController.php
   @@ -42,6 +42,7 @@ public function __construct() {
   +    $form_state->set('#contexts', []);
   

   We don't use #-prefixed properties in $form_state

3. +++ b/core/modules/block/src/BlockForm.php
   @@ -65,6 +67,56 @@ public function form(array $form, FormStateInterface $form_state) {
        $form['settings'] = $entity->getPlugin()->buildConfigurationForm(array(), $form_state);
   +    $form['settings']['visibility_tabs'] = array(
   

   If you're going to move this back up a level, it has to actually go up a level. The block.module form doesn't get to touch 'settings', that needs to be completely controlled by the plugin

4. +++ b/core/modules/block/src/EventSubscriber/BlockConditionContextSubscriberBase.php
   @@ -26,24 +26,38 @@
   +    $events[BlockEvents::ACTIVE_CONTEXT][] = 'onBlockActiveContext';
   +    $events[BlockEvents::ADMINISTRATIVE_CONTEXT][] = 'onBlockAdministrativeContext';
   

   We absolutely need this second event, thanks

3.) BAAAAAAH yuck you're right. Ok, I'll see what I can do.

Eclipse

+++ b/core/modules/block/src/EventSubscriber/BlockConditionContextSubscriberBase.php
@@ -26,24 +26,38 @@
+   * Determine available configuration-time contexts.
+   *
+   * This allows empty contexts to be returned at configuration time in order
+   * to properly configure plugins. This also opens the door to multiple
+   * context availability during both administration and run time in order to
+   * provide nuanced configuration options.
+   *
+   * @param BlockConditionContextEvent $event
+   *   The Event to which we can register available contexts.
+   */
+  abstract public function onBlockAdministrativeContext(BlockConditionContextEvent $event);

I did document it right here (and also a specific implementation on the node context subscriber)

FWIW, chx has asked to help document the ContextInterface as well, #2344045: ContextInterface needs documentation and I wrote up https://etherpad.mozilla.org/50vwJOqZEO as something to bounce back and forth with him last night. This should help in this regard as well.

Eclipse

Ok, this was a bit of effort. Moving visibility up out of settings made it kind of obvious that the block interface visibility stuff had to move to the entity as well. Updates as relevant.

Context mapping was changed back to what the ContextHandler expects, however since this issue has grown into a more generic context handling solution, I changed block's schema to match since it was still "contexts".

Other then that I've tried to take all the feedback here into account except for Gabor's issue with my docs. The patch was working and burning a whole in my pocket. I'll check to see tests tomorrow and if all is well, proceed with further docs and tests.

Eclipse

hotel wifi--

Just to get more info out of the test failures. The BlockBaseTest I commented out needs a new home.

I reviewed the patch. The general approach makes sense to me, while I'd like to see global context to be separated into its own event. But as discussed, that can be a non-API breaking follow-up issue.

Here some remarks:

1. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php
   @@ -49,4 +50,58 @@ public function setContext($name, ComponentContextInterface $context) {
   +   * @return array
   

   Missing NL between the two lines.

2. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php
   @@ -49,4 +50,58 @@ public function setContext($name, ComponentContextInterface $context) {
   +   * @param array $form
   +   * @param FormStateInterface $form_state
   +   */
   

   Documentation misses description.

3. +++ b/core/modules/block/src/BlockForm.php
   @@ -154,6 +208,15 @@ public function validate(array $form, FormStateInterface $form_state) {
   +      // Allow the condition to validate the form.
   +      $condition_values = (new FormState())
   +        ->setValues($form_state->getValue(['visibility', $condition_id]));
   +      $condition->validateConfigurationForm($form, $condition_values);
   +      // Update the original form values.
   +      $form_state->setValue(['visibility', $condition_id], $condition_values->getValues());
   

   That's funky, is there no better way to trigger validation? Same for submit.

4. +++ b/core/modules/block/src/BlockInterface.php
   @@ -40,4 +40,43 @@ public function getPlugin();
   +   * @return \Drupal\Core\Condition\ConditionInterface[]|\Drupal\Core\Condition\ConditionPluginBag
   

   That's a bit weird, cannot it be just either of both?

5. +++ b/core/modules/block/src/BlockInterface.php
   @@ -40,4 +40,43 @@ public function getPlugin();
   +  public function getVisibilityCondition($instance_id);
   

   Shouldn't it require configuration as well? I see there is a setter, but why not have it as second parameter to make sure it's there?

6. +++ b/core/modules/block/src/BlockInterface.php
   @@ -40,4 +40,43 @@ public function getPlugin();
   +   * Gets the values for all defined contexts.
   

   What are defined contexts for a condition? Does it mean context available to the block?

1.) Noted
2.) Noted
3.) It's how we've done it for forever. I'm not opposed to discussing changes here, but probably follow-up material. Also not sure what you have in mind.
4.) ConditionPluginBag is iterable, so both are simultaneously true.
5.) No, I think all of that is provided to the plugin bag during instantiation, and we're just asking it for the specific instance that it already has all that information for.
6.) No, the context is actually not (yet) available to the block. It's just contexts available for the visibility conditions.

Eclipse

ad 3, I see - its own issue then.

>4.) ConditionPluginBag is iterable, so both are simultaneously true.
Yes, but it's weird not to know what you are iterating on. It should be clear what kind of animals this method returns.

>ad 5)
I see.

> 6.) No, the context is actually not (yet) available to the block. It's just contexts available for the visibility conditions.

ok, so the docs could use some clarification here as it was not clear to me what context this is about. available, required context?

Added an issue for separating global context #2349679: Support registration of global context.

Gabor,

I've spoken with Angie. No one can give me any guarantees obviously, but I'm hoping that since the API that changes here is used by a very small subset of contrib modules and it is largely moving methods around (and fixing tests to match) that we'll be ok.

What is complicating things right now is that our old visibility UI displays form elements no matter what. With the addition of context to the system, this gets tricky because context is always saved, event when you set no visibility rules. This can result in blocks that disappear, even though their visibility settings haven't been altered. The simplest fix to this is to re-work the UI to only show forms for visibility conditions we actually want to use. (i.e. the user opted into them) The other option is building some system that can know if the configuration for the conditions is "null" and have it throw away all the null values (including context which is not null). We don't have a system like that so I'm sort of debating my way forward here. I ran into this today and have not come to a conclusion.

Eclipse

I reviewed this from a high level.

1. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -264,7 +264,7 @@ block_settings:
   -        contexts:
   +        context_mapping:
   

   This name doesn't make sense. Cache contexts should remain unchanged.

2. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -152,25 +125,8 @@ public function calculateDependencies() {
   -    if ($this->resolveConditions($conditions, 'and', $contexts, $mappings) !== FALSE && $this->blockAccess($account)) {
   +    if ($this->blockAccess($account)) {
   

   This change looks like it *could* be a performance regression.

   Applying block visibility is very cheap. Running the access system, plus the things it needs to load in order to perform access checking, is much more expensive.

   But, having seen the changes in BlockAccessControlHandler::access() further down the patch, it looks like this execution order is still the same, hence it's fine :)

3. +++ b/core/modules/block/src/BlockForm.php
   @@ -65,6 +67,58 @@ public function form(array $form, FormStateInterface $form_state) {
   +    \Drupal::service('event_dispatcher')->dispatch(BlockEvents::ADMINISTRATIVE_CONTEXT, new BlockConditionContextEvent($conditions));
   

   Event dispatching doesn't happen very often. We should document this very, very clearly.

   (Also: the service should be injected.)

4. And related to the first point plus #2349679: Support registration of global context: it looks increasingly to be the case that any block plugin context should have an associated cache context. See #2349679-2: Support registration of global context.

re-point 1.) I totally agree. I was moving too fast and didn't realize this was cache context. I found it during testing and just having uploaded a new patch since I was struggling with the issues I mentioned in #30.

Eclipse

This patch won't work, but at least it documents what I've been doing.

Eclipse

Going to rework the summary today. Here's a current patch, it still won't pass but I want to see how badly the tests are doing after this last set of changes.

rerolled against head.

Glad you fixed it, but it's important that we take cache context into account while working on all this: please reply to #2349679: Support registration of global context.

I'm not sure it's important to fix that in this issue. I'd rather keep status quo and not add yet another moving part in this patch. I'm all for the notion of moving current user and current language(s) to a global context solution, but it'll be one more thing to justify in this patch, and I'd prefer to not complicate this further.

Eclipse

Ok, I had to resort to some pretty huge changes in order to get some of the basic functionality the way it should be. The ConditionPluginBag was way too fiddly for what I was doing to the condition values, so I ejected it in favor of (mostly) properly injected ConditionManager calls to instantiate the Conditions when necessary. This is a TON easier to read and simplifies the BlockInterface further.

In addition to these changes, I moved context gathering outside the scope of the block completely and moved it into the FullPageVariant. This is a FAR saner solution and allows our blocks to actually leverage context properly when that day comes but also I added methods to the BlockInterface for containing the entire context stack which allows us to pass it to condition properly. The event constants & classes that did this work have been updated to reflect these changes.

I've not run a full suite of tests on this yet, and there are definitely still problems but I wanted a better picture of what these changes may have broken.

Eclipse

1. +++ b/core/lib/Drupal/Core/Condition/ConditionPluginBase.php
   @@ -26,8 +26,6 @@
   -    $this->setConfiguration($configuration);
   

   This is HUGELY concerning to me. If all configurable plugins are abusing their defaults this way we have a potential problem. It's complicated by block visibility since it's not "opt-in" right now, you just get ALL conditions, but if any other system in Drupal core uses this same approach, it's a huge pain to untangle this and you can't tell which plugins have been interacted with by the user since they're all on screen and all have values. This is actually why I supported defaults in the annotation (ages ago now) because there was no way to confuse defaults with user configuration until the form was submitted. This approach configures a plugin before anything has even happened. Very concerning.

2. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginBag.php
   @@ -78,6 +78,12 @@ public function __construct(PluginManagerInterface $manager, array $configuratio
   +      /*$debug = debug_backtrace();
   +      foreach ($debug as $stack => $info) {
   +        $callable = !empty($info['class']) ? $info['class'] . '::' . $info['function'] : $info['function'];
   +        $callable .= ' Line' . $info['line'];
   +        drupal_set_message(t('!test', ['!test' => '<pre>' . print_r($callable, TRUE)]));
   +      } */
   

   oops...

3. +++ b/core/modules/block/src/Event/BlockContextEvent.php
   @@ -0,0 +1,36 @@
   + * * @see \Drupal\block\Event\BlockEvents::ADMINISTRATIVE_CONTEXT
   

   also oops...

4. +++ b/core/modules/block/src/Event/BlockEvents.php
   @@ -18,6 +18,14 @@
   +   * @see \Drupal\block\Event\BlockConditionContextEvent
   

   more oops...

5. +++ b/core/modules/block/src/Plugin/DisplayVariant/FullPageVariant.php
   @@ -116,11 +129,18 @@ protected function getTheme() {
   +    $contexts = $this->dispatcher->dispatch(BlockEvents::ACTIVE_CONTEXT, new BlockContextEvent())->getContexts();
   

   We get context here now instead of foreach block's conditions. This means that Contextual blocks are technically possible and that means we could actually filter available blocks by context on the block admin UI. Cool!

6. +++ b/core/modules/block/src/Plugin/DisplayVariant/FullPageVariant.php
   @@ -116,11 +129,18 @@ protected function getTheme() {
   +            $this->contextHandler->applyContextMapping($block_plugin, $contexts);
   

   need to put a try catch on this that prevents contextual blocks blowing the system up.

7. +++ b/core/modules/language/src/Form/NegotiationSessionForm.php
   @@ -43,7 +43,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    $this->config('language.negotiation')
   

   This is actually an unrelated bug I ran into when I was first investigating this issue. @Gabor, care to move this into another issue? Let me know.

8. +++ b/core/modules/language/src/Plugin/Condition/Language.php
   @@ -31,23 +31,23 @@ class Language extends ConditionPluginBase {
   +      $languages = \Drupal::languageManager()->getLanguages(LanguageInterface::STATE_CONFIGURABLE);
   

   Need to inject this properly.

I've taken care of the few oopses I had here already locally and they'll be in the next patch.

Eclipse

I spent some time reviewing and testing this patch. I think I understand what's being done here conceptually, and if so, I'm all for it. :-) But I'm not an expert in the code this is changing, so I don't really have any valuable line-by-line review. Instead, I'm going to attempt to describe what I think this patch is doing and say why I think this is a good thing:

1. Its moving the hard-coded visibility form (which is the root of this problem) from \Drupal\Core\Block\BlockBase (which is part of the core block API) and moving it to Drupal\block\BlockForm in the block module. So, essentially, removing it from being an inherint part of ALL block plugins, and instead having it live in the block module which is responsibile for block placement. This will make it possible for contrib modules like Panels (which offer an alternative mechanism for block placement) to easily implement visibility using an alternative UI where users add conditions (ie. an actual sane UI), rather than having all of them always set.
2. It attempts to detect conditions that were empty and remove them. This is key to fixing this issue, because without this, all conditions are always set and always have values (since they are all always being shown on the form). Unfortunately, I don't think this can always correctly make this determination -- switching to a UI where conditions are explicitly added by the user would be better, but that's out of scope for this issue, so this is a net improvement over current HEAD.
3. Changes to the way contexts are handled to specifically address the language bug (I think - I'm not entirely sure from the issue summary and early comments how to test if the language bug fixed or not).

Anyway, this all seems great! It moves some nastiness from the core API, which many modules will be using, and isolates it in the block module. This will also make our efforts at porting Panels to D8 easier, and will probably have similarly positive effects on other layout modules.

However, my manual testing of the patch in #45 showed some problems. I configured the "Powered by Drupal" block to only show up on "<front>", but it showed up on all pages anyway. It seems like my configuration isn't actually saving?

In any case, great work so far! @EclipseGC++

Hmm, I'll check the pages one out. Node type and user role are definitely saving.

Lets get some feedback.

I wasn't really ready to do that, but ok, for committers it's worth noting no block plugins were altered in order to make these changes work. Shouldn't affect anyone implementing blocks, this affects (positively) contrib that wants to make alternate use-cases for blocks.

Eclipse

Ok, I tried to take care of most of my own commentary from 47 and I removed point 7 on that list as it's not actually related to this. Interdiff is from before I merged to HEAD, PluginBags were named to collections and that complicated this merge somewhat.

Eclipse

The serialization error is weird and I've not tracked it down yet, but thought I'd put up my progress. This last set of tests found an awesome bug I'd missed so ++ to that

Eclipse

+++ b/core/modules/views/tests/src/Unit/Plugin/Block/ViewsBlockTest.php
@@ -126,10 +126,6 @@ public function testBuild() {
-    $reflector = new \ReflectionClass($plugin);
-    $property = $reflector->getProperty('conditionPluginManager');
-    $property->setAccessible(TRUE);
-    $property->setValue($plugin, $this->getMock('Drupal\Core\Executable\ExecutableManagerInterface'));

Since the block visibility is no longer a core part of the block plugin, we no longer need this sort of code in tests (or elsewhere).

Can't reproduce this fatal locally, so I'm trying stuff...

Eclipse

Anyone know what gives on this container serialization error?

Eclipse

@EclipseGc - I am just posting a comment now because timezones are a funny thing ... and you are maybe awake...

I have seen this a few times before and it was always because a "use DependancySerializationTrait" was missing from the thing that was simulating the container...

I am just off the bed, but I will track this down tomorrow, if that helps....

It only happens when you don't pass the $theme in the url. Very interesting, looking further.

Eclipse

Ok, I spoke with dawehner for a bit and he gave me a hint. The context_mapping in the form_state could have things in it we don't want to cache, and we only need it during build of the form array, so I set it to an empty array before returning the form. Let's see how this goes this time.

Eclipse

1. +++ b/core/modules/block/src/BlockForm.php
   @@ -85,6 +85,8 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $contexts = $this->dispatcher->dispatch(BlockEvents::ADMINISTRATIVE_CONTEXT, new BlockContextEvent())->getContexts();
   +    $form_state->set('context_mapping', $contexts);
   
   @@ -98,8 +100,6 @@ public function form(array $form, FormStateInterface $form_state) {
   -    $contexts = $this->dispatcher->dispatch(BlockEvents::ADMINISTRATIVE_CONTEXT, new BlockContextEvent())->getContexts();
   -    $form_state->set('context_mapping', $contexts);
   

   Moved this up just a tad which will make it available for blocks to become context configurable just like conditions are with relative ease.

2. +++ b/core/modules/block/src/BlockForm.php
   @@ -201,6 +201,7 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $form_state->set('context_mapping', []);
   

   throwing away the context_map during administration time before we return the form to prevent nasty things from being cached with the form.

+++ b/core/tests/Drupal/Tests/Core/Block/BlockBaseTest.php
@@ -57,8 +49,10 @@ public function testGetMachineNameSuggestion() {
+  public function _testConditionsBagInitialization() {

Need to fix this test.

#41: Oh, absolutely. That's not what I was asking. I was only asking for a reply to #2349679: Support registration of global context, so that you at least don't do things that you'll have to undo when you fix that issue. Did you do that while working on this? Perhaps that doesn't make sense — I don't know Contexts well enough. Then again, how could I, with #2344045: ContextInterface needs documentation still open?
I'd almost say we should block this on #2344045: ContextInterface needs documentation, because how can anybody who wasn't involved with building Contexts properly review this without at least knowing what Contexts are, exactly?

#45: Why no interdiff?! This is the big chunk I should review, but I have to postpone now because reviewing a 80K patch just before calling it a day is not going to work out well.

Subsequent interdiffs look fine. Will try to review the entire patch tomorrow.

Sorry Wim,

I'm not sure the interdiff would have made a ton of sense since I was tearing out the ConditionPluginBag/Collection in that particular patch because I could only get the patch half working with that in there. Frankly, I put that patch up expecting everything to go nuclear, so when it came back with so few errors, it was a total surprise. I knew the lack of interdiffs was going to be a pain point and I tried to make sure interdiffs were provided from that point forward. Sorry.

I'll respond to #2349679: Support registration of global context now and I've been trying to stay on top of #2344045: ContextInterface needs documentation as I had time.

Eclipse

I failed to document a method I added. I'll take a pass at the renamed test in my next patch.

Eclipse

Frankly, I put that patch up expecting everything to go nuclear, so when it came back with so few errors, it was a total surprise.

Hah! :D That makes sense. No worries about the interdiff. As I said, I'll try to review this patch later today. Great to see a new green patch.

I looked at this test for a bit, and it's testing functionality of the ConditionPluginCollection. This patch a.) removes that, and b.) remove the behaviors it was expect in favor of a more straight-forward code path. So, with those things being true, I removed the last test, because the rest of what it did (configuring conditions and such) is well tested elsewhere both procedurally and through the UI.

I consider this very ready for review.

Eclipse

1. +++ b/core/lib/Drupal/Core/Condition/ConditionPluginBase.php
   @@ -26,8 +26,6 @@
       */
      public function __construct(array $configuration, $plugin_id, $plugin_definition) {
        parent::__construct($configuration, $plugin_id, $plugin_definition);
   -
   -    $this->setConfiguration($configuration);
      }
    
      /**
   

   If you remove the call to setConfiguration you can drop the entire constructor.

2. +++ /dev/null
   diff --git a/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php b/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php
   index 8063b2d..a866f2c 100644
   
   index 8063b2d..a866f2c 100644
   --- a/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php
   
   --- a/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php
   +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php
   
   +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php
   @@ -8,6 +8,7 @@
   +use Drupal\Core\Annotation\Translation;
   
   @@ -120,6 +121,9 @@ public function setDataType($data_type) {
   +    if ($this->label instanceof Translation) {
   +      return (string) $this->label->get();
   +    }
        return $this->label;
   

   Is there no other way to fix this? One way what we could do is to implement __toString for Translation

3. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php
   @@ -49,4 +50,61 @@ public function setContext($name, ComponentContextInterface $context) {
   +    /**
   +     * @var \Drupal\Core\Plugin\Context\ContextHandlerInterface $handler
   +     */
   +    $handler = \Drupal::service('context.handler');
   

   Can you please wrap that inside a getContextHandler() method?

4. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php
   @@ -49,4 +50,61 @@ public function setContext($name, ComponentContextInterface $context) {
   +    /**
   +     * @var \Drupal\Component\Plugin\Context\ContextDefinitionInterface $context_definition
   +     */
   +    if ($this->getContextDefinitions()) {
   +      foreach ($this->getContextDefinitions() as $name => $context_definition) {
   

   Why the hell do we call getContextDefinitions()
   twice here?
   Note: If we would document this function property: @return \Drupal\Component\Plugin\Context\ContextDefinitionInterface[] and you would not need this inline documentation.

5. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php
   @@ -49,4 +50,61 @@ public function setContext($name, ComponentContextInterface $context) {
   +    if (!is_null($contexts)) {
   

   It seems for me that if (isset($contexts)) would be easier to read?

6. +++ b/core/modules/block/config/schema/block.schema.yml
   @@ -24,6 +24,12 @@ block.block.*:
   +    visibility:
   +      type: sequence
   +      label: 'Visibility Conditions'
   +      sequence:
   +        - type: condition.plugin.[id]
   +          label: 'Visibility Condition'
   

   So we basically move the storage of the visibility out of the block itself into another level so each system using blocks would have to implement it again, but from a general perspective I totally agree. The visibility does not belong into the block itself. This was an API design driven by the UI

7. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -7,23 +7,65 @@
   +
   +  public function __construct(EntityTypeInterface $entity_type, ExecutableManagerInterface $manager, ContextHandlerInterface $context_handler) {
   

   Feel free to document it.

8. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -33,8 +75,34 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
   +      // This should not be hardcoded to an uncacheable access check result, but
   +      // in order to fix that, we need condition plugins to return cache contexts,
   +      // otherwise it will be impossible to determine by which cache contexts the
   +      // result should be varied.
   

   Well, I am not sure whether this statement is entirely correct. The context is actually also part of the AccessResult object.

9. +++ b/core/modules/block/src/BlockForm.php
   @@ -33,13 +38,29 @@ class BlockForm extends EntityForm {
   +   *
   +   * @var \Drupal\Core\Condition\ConditionManager
   +   */
   +  protected $manager;
   +
   ...
   +    $this->manager = $manager;
   
   +++ b/core/modules/block/tests/src/Unit/BlockFormTest.php
   @@ -49,11 +49,11 @@ public function testGetUniqueMachineName() {
   +    $condition_manager = $this->getMock('Drupal\Core\Executable\ExecutableManagerInterface');
   ...
   +    $block_form_controller = new BlockForm($entity_manager, $condition_manager, $event_dispatcher);
   

   ... We are so not sure everywhere whether we talk about conditions or executables (whatever the later one is), but a little bit more standardization would be maybe cool. Fell free to give counterpoints.

10. +++ b/core/modules/block/src/BlockForm.php
    @@ -33,13 +38,29 @@ class BlockForm extends EntityForm {
        * Constructs a BlockForm object.
        *
        * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
        *   The entity manager.
        */
    -  public function __construct(EntityManagerInterface $entity_manager) {
    +  public function __construct(EntityManagerInterface $entity_manager, ExecutableManagerInterface $manager, EventDispatcherInterface $dispatcher) {
    

    docs ...

11. +++ b/core/modules/block/src/BlockForm.php
    @@ -62,9 +85,66 @@ public function form(array $form, FormStateInterface $form_state) {
    +    $form['settings']['visibility_tabs'] = array(
    +      '#type' => 'vertical_tabs',
    +      '#title' => $this->t('Visibility'),
    +      '#parents' => array('visibility_tabs'),
    +      '#attached' => array(
    +        'library' => array(
    +          'block/drupal.block',
    +        ),
    +      ),
    +    );
    +    // @todo Allow list of conditions to be configured in
    +    //   https://drupal.org/node/2284687.
    +    $visibility = $this->entity->getVisibility();
    +    foreach ($this->manager->getDefinitions() as $condition_id => $defintion) {
    +      // Don't display the current theme condition.
    +      if ($condition_id == 'current_theme') {
    +        continue;
    +      }
    +      $condition = $this->manager->createInstance($condition_id, isset($visibility[$condition_id]) ? $visibility[$condition_id] : array());
    +      $form_state->set(['conditions', $condition_id], $condition);
    +      $condition_form = $condition->buildConfigurationForm(array(), $form_state);
    +      $condition_form['#type'] = 'details';
    +      $condition_form['#title'] = $condition->getPluginDefinition()['label'];
    +      $condition_form['#group'] = 'visibility_tabs';
    +      $form['visibility'][$condition_id] = $condition_form;
    +    }
    +
    +    // @todo Determine if there is a better way to rename the conditions.
    +    if (isset($form['visibility']['node_type'])) {
    +      $form['visibility']['node_type']['#title'] = $this->t('Content types');
    +      $form['visibility']['node_type']['bundles']['#title'] = $this->t('Content types');
    +      $form['visibility']['node_type']['negate']['#type'] = 'value';
    +      $form['visibility']['node_type']['negate']['#title_display'] = 'invisible';
    +      $form['visibility']['node_type']['negate']['#value'] = $form['visibility']['node_type']['negate']['#default_value'];
    +    }
    +    if (isset($form['visibility']['user_role'])) {
    +      $form['visibility']['user_role']['#title'] = $this->t('Roles');
    +      unset($form['visibility']['user_role']['roles']['#description']);
    +      $form['visibility']['user_role']['negate']['#type'] = 'value';
    +      $form['visibility']['user_role']['negate']['#value'] = $form['visibility']['user_role']['negate']['#default_value'];
    +    }
    +    if (isset($form['visibility']['request_path'])) {
    +      $form['visibility']['request_path']['#title'] = $this->t('Pages');
    +      $form['visibility']['request_path']['negate']['#type'] = 'radios';
    +      $form['visibility']['request_path']['negate']['#title_display'] = 'invisible';
    +      $form['visibility']['request_path']['negate']['#default_value'] = (int) $form['visibility']['request_path']['negate']['#default_value'];
    +      $form['visibility']['request_path']['negate']['#options'] = array(
    +        $this->t('Show for the listed pages'),
    +        $this->t('Hide for the listed pages'),
    +      );
    +    }
    +    if (isset($form['visibility']['language'])) {
    +      $form['visibility']['language']['negate']['#type'] = 'value';
    +      $form['visibility']['language']['negate']['#value'] = $form['visibility']['language']['negate']['#default_value'];
    +    }
    

    For readability it seems to help a bit to extract that into a small helper method on the same class

12. +++ b/core/modules/block/src/BlockForm.php
    @@ -154,6 +235,24 @@ public function validate(array $form, FormStateInterface $form_state) {
    +    // Validate visibility condition settings.
    +    foreach ($form_state->getValue('visibility') as $condition_id => $values) {
    +      if ($this->clearInvalidConditions($values)) {
    +        // Allow the condition to validate the form.
    +        $condition = $form_state->get(['conditions', $condition_id]);
    +        $condition_values = (new FormState())
    +          ->setValues($values);
    +        $condition->validateConfigurationForm($form, $condition_values);
    +        // Update the original form values.
    +        $form_state->setValue(['visibility', $condition_id], $condition_values->getValues());
    +      }
    +      else {
    +        $form_state->unsetValue(['visibility', $condition_id]);
    +      }
    +    }
    +    if (!$form_state->hasValue('visibility')) {
    ...
    +    }
    

    Same here, small helper methods improves the visibility a lot and also makes it potentially easier to extract later.

    PS: why do we check hasValue at the end again? Previously we assume that visibility is an array, so its either not empty or still an

    itself?
    </li>
    
    <li>
    <code>
    +++ b/core/modules/block/src/BlockForm.php
    @@ -218,4 +332,33 @@ public function getUniqueMachineName(BlockInterface $block) {
    +    // Remove empty values.
    +    $values = array_filter($values);
    +    foreach ($values as $key => $value) {
    +      if (is_array($value)) {
    +        $value = $this->clearInvalidConditions($value);
    +        if (empty($value)) {
    +          unset($values[$key]);
    +        }
    +        else {
    +          $values[$key] = $value;
    +        }
    +      }
    +    }
    

    This is such a sad story, well, maybe we can change in the future

13. +++ b/core/modules/block/src/BlockInterface.php
    @@ -40,4 +40,42 @@ public function getPlugin();
    +  /**
    +   * Sets the visibility configurations for specified conditions.
    +   *
    +   * @param array $visibility
    +   *   An array of plugin_id keys and configuration values.
    +   *
    +   * @return $this
    +   */
    +  public function setVisibility(array $visibility);
    +
    +  /**
    +   * Sets the visibility condition configuration.
    +   *
    +   * @param string $instance_id
    +   *   The condition instance ID.
    +   * @param array $configuration
    +   *   The condition configuration.
    +   *
    +   * @return $this
    +   */
    +  public function setVisibilityConfig($instance_id, array $configuration);
    +
    +  /**
    +   * Get all available contexts.
    +   *
    +   * @return \Drupal\Component\Plugin\Context\ContextInterface[]
    +   *   An array of set contexts, keyed by context name.
    +   */
    +  public function getAvailableContexts();
    +
    +  /**
    +   * Set the contexts that are available for use within the block entity.
    +   *
    +   * @param array $contexts
    +   * @return mixed
    +   */
    +  public function setAvailableContexts(array $contexts);
    +
    

    I wonder whether we could move this onto a separate interface like BlockConfigurationInterface(No, I don't really like that name at all)

14. +++ /dev/null
    --- /dev/null
    +++ b/core/modules/block/src/Event/BlockContextEvent.php
    

    One thing I don't understand is why this is not part of Core itself ... every implementation of a block like system would need to fire an event?

15. +++ b/core/modules/block/src/EventSubscriber/BlockContextSubscriberBase.php
    @@ -0,0 +1,46 @@
    +   */
    +  abstract public function onBlockActiveContext(BlockContextEvent $event);
    ...
    +   */
    +  abstract public function onBlockAdministrativeContext(BlockContextEvent $event);
    

    We could just pass along the array of contexts by reference, so usercode would never have deal with the event itself. In reality all of them will do a getContexts() and a setContexts() right?

16. +++ b/core/modules/block/src/EventSubscriber/BlockContextSubscriberBase.php
    @@ -0,0 +1,46 @@
    +   * This allows empty contexts to be returned at configuration time in order
    +   * to properly configure plugins. This also opens the door to multiple
    +   * context availability during both administration and run time in order to
    +   * provide nuanced configuration options.
    

    I think an example for an empty context would be great here. Just add a small @code @endcode block and make lifes people easy

17. +++ b/core/modules/block/src/EventSubscriber/CurrentUserContext.php
    @@ -50,12 +51,19 @@ public function __construct(AccountInterface $account, EntityManagerInterface $e
    +   */
    +  public function onBlockAdministrativeContext(BlockContextEvent $event) {
    +    $this->onBlockActiveContext($event);
       }
    

    This is a bit confusing, can you add a comment why you can use the same thing twice?

18. +++ b/core/modules/block/src/Plugin/DisplayVariant/FullPageVariant.php
    @@ -116,11 +129,20 @@ protected function getTheme() {
    +    $event = new BlockContextEvent();
    +    $this->dispatcher->dispatch(BlockEvents::ACTIVE_CONTEXT, $event);
    +    $contexts = $event->getContexts();
    

    I don't know but it feels wrong that the variant have to determine the available global contexts ... could it ask some helper method? Note: Mini panels would have to do the same stuff maybe as well.

19. +++ b/core/modules/block/src/Tests/BlockLanguageTest.php
    @@ -53,11 +54,11 @@ public function testLanguageBlockVisibility() {
    -    $this->assertField('settings[visibility][language][langcodes][en]', 'Language visibility field is visible.');
    +    $this->assertField('visibility[language][langcodes][en]', 'Language visibility field is visible.');
    ...
    -      'settings[visibility][language][langcodes][en]' => TRUE,
    +      'visibility[language][langcodes][en]' => TRUE,
    

    Did you considered not changing the form parents by speciying #tree => TRUE?

20. +++ b/core/modules/block/src/Tests/BlockLanguageTest.php
    @@ -69,11 +70,11 @@ public function testLanguageBlockVisibility() {
    -    // Check that a page has a block.
    +    // Check that a page has the block.
         $this->drupalGet('en');
         $this->assertText('Powered by Drupal', 'The body of the custom block appears on the page.');
     
    -    // Check that a page doesn't has a block for the current language anymore.
    +    // Check that the page in a different language does not have the block.
         $this->drupalGet('fr');
    

    This seems to be a bit out of scope ...

1.) Done
2.) True, but that seems like a follow up. I was just trying to fix something that was broken here and since I could call to available methods, that seemed the better thing to do for the moment instead of changing another class.
3.) Done
4.) Done, however, even if we did document that return properly, the ContextHandlerInterface expects the Core version of the ContextDefinition, which I didn't initially notice, so I actually updated that variable documentation inline to make phpstorm stop complaining.
5.) Done
6.) Yes all around, it also means that people implementing this now have the opportunity to create their own UI w/o having to spend effort and time destroying our UI first.
7.) Done
8.) Not my comment, I just moved the comment with the code. I've asked Wim to comment (he might beat me to commenting) but I will note that we should re-arrange this in a follow up to execute block access before block visibility since visibility is user configurable and could get very hairy, and access is hard coded. I know it used to work this way and was inverted before conditions were in charge of block visibility, but given the final result here, regardless of this patch, we should revert that change and check block access first.
9.) The ExecutablePluginInterface is the interface which the ConditionManager implements. We typehint by one, but we expect an instance of the other. The Action plugins should also be using ExecutablePluginInterface. This was debated and negotiated at length with fago. I know it's weird, but it's how the current system works, and there are "reasons". I was mostly trying to make many parties happy when that code was being written, so it is what it is. PageManager and Rules both have plans for those systems, and I was just trying to make sure we wouldn't need completely different plugin types again. *shrug*
10.) Done
11.) & 12.a.) Fair enough. Not to sidetrack my own issue here... but why isn't $form passed by reference to the validate() method? That gives me pause. Also the "hasValue()" check is just me being uber-paranoid.
12.b.) Yes, sad story, but it was my best solution to a bad problem. If we can swap out the UI later, we can remove this... we could also actually #require the appropriate values in the conditions, which is another WTF for me.
13.) I don't think that's necessary. The block entity (for which this is the interface) is the holder of all this sort of logic regardless of what we do, so having these methods as part of this interface is the cleanest and most straight-forward solution. No other object has to hold this stuff ever.
14.) There are certain contexts which are global in nature and others which will be specific to blocks. There's an issue for the global ones so this patch just sets that patch up to make it much easier. In short, language and current user will move out of block specific contexts and node will likely stay since other systems will determine the availability of route-based contexts in a completely different way.
15.) Actually most will just do a $event->setContext(). I chose not to send this to yet another abstract method because interacting with the event is literally just calling the setContext() method to add your context if necessary. I don't think they should ever have to call getContext() on the event, that's really for the class that dispatched the event to do to get all the contexts set by the subscriber. I just went for the least abstracted scenario here since there's only one method the developer has to interact with.
16.) Done
17.) Done
18.) Actually I think this is where this belongs. The Variant can frame the entire block placement/context discussion. That's actually a huge part of its purpose. In the case of panels (not mini-panels) Panels should provide a custom Variant class as well, and would want to do something like this (but as alluded to elsewhere in my comments will probably have a different methodology for getting much of it. Putting this in FullPageVariant, in my opinion, contains the approach that core needs in a place where it is unlikely to influence the design of the system in such a way as to make contrib have to "undo" things. They can replace this class with their own, but they'd have had to do that anyway. As far as mini-panels goes, that should be implemented as a block, so it should get contexts pushed down to it by whatever methodology the rendering Variant chooses. Hopefully that made sense, I'm open to debate on this one, but this is how I see it currently.
19.) Yes, in fact my early patch(es) did exactly that, but as Tim Plunkett points out earlier in this comment thread, 'settings' is specific to the block plugin, and we're separating visibility from that, so we need to move the form elements as well.
20.) The verb tenses were wrong and my ocd kicked in. do I HAVE to remove it?

Eclipse

I read through the interdiffs since I last tested this patch, and nothing jumped out at me. That said, I'm really not qualified to do a line-by-line review of this code. :-)

However, I did manual testing again, and everything worked great! In my previous review/testing in #48, I set a block to only be visible on "<front>" and it wasn't working. In my testing with the latest patch from #74, it worked as it should!

I'm really looking forward to this getting committed, and then the expected follow up on #2349679: Support registration of global context! This will be a major win for Panels (and just Blocks in general).

2.) True, but that seems like a follow up. I was just trying to fix something that was broken here and since I could call to available methods, that seemed the better thing to do for the moment instead of changing another class.

Can you please open one :)

15.) Actually most will just do a $event->setContext(). I chose not to send this to yet another abstract method because interacting with the event is literally just calling the setContext() method to add your context if necessary. I don't think they should ever have to call getContext() on the event, that's really for the class that dispatched the event to do to get all the contexts set by the subscriber. I just went for the least abstracted scenario here since there's only one method the developer has to interact with.

Well I prefer it the other way round, see EntityTypeEventSubscriberTrait but yeah you seem to have more words so you win, HAHA.

20.) The verb tenses were wrong and my ocd kicked in. do I HAVE to remove it?

ctrl+alt+z is the shortcut for me to undo a specific hunk in storm. This is really handy once you use it everyday.

Re:2.) I was wrong, and the code in this patch handled it wrong. The ContextDefinition annotation class should have unpacked this before it ever ended up in the ContextDefinition implementation class. My bad. Other Plugin Annotations extend the Plugin class which includes a protected parse() method that unpacks this stuff for us. I added a simple bit of code to the ContextDefinition annotation class that will properly get() the translation & cast it to a string. I think it should still pass tests. No follow up for this one.

20.) ok removed.

Eclipse

missed a semi-colon

here's the interdiff for documentation purposes.

@EclipseGc Remind me, why did you not implemented __toString in TranslationWrapper?

2.) True, but that seems like a follow up. I was just trying to fix something that was broken here and since I could call to available methods, that seemed the better thing to do for the moment instead of changing another class.

I think having that one single method would be less change than fixing the one instance, isn't it?

Yeah, a __toString() implementation could still be great for us to have, filed a follow up: #2362727: Implement __toString() on Translation Annotation That being said, the changes I made to this patch are the more appropriate changes for this particular problem until that patch lands, so... Nothing to change here.

Eclipse

Gabor,

Can you give steps to reproduce the blank condition?

Eclipse

Danke!

Gabor, I worked against your test a bit and manually went through the same steps. Locally, when I manually reproduce your steps, the "session" detection on content doesn't save the first time through and I have to resave the form again. Once I do this, it saves the session settings properly and manually following the same steps as your test works. I think there's a bug in that form.

Let me know

Eclipse

Ok, I added Gabor's patch in (no interdiff since he essentially provided that) and fixed the condition descriptions.

With regard to the empty Language condition, I think we should provide a core-specific solution much like we do with current theme and simply remove it from the list if there aren't multiple configured languages. Gabor, does that seem sensible to you?

I've worked against these tests quite a bit, and the english language gets passed as a context instead of french for the one failing test. This doesn't happen outside of tests, so I'm not sure what the issue is. :-(

More this weekend I guess.

Eclipse

Ok, hunting down my theory about language_content further, I decided to flip flop the tests and see if that passed, it DOES, which means there's an issue of some sort with the form. Again, walking through this manually in a fully installed instance, I can make either work, but I have to save the form twice for language_content to work. Anyway, this should show that the code in this patch is actually working and doing what gabor needed, as well as exposing some functionality contextual plugins where intended to have, all around win I hope. We still have the open question about the language condition when there are no configured languages. I'm still leaning heavily toward removing it from the list when the language count is less than 2.

Eclipse

This should break a few tests, but identifying them this way will be faster than doing it locally.

Eclipse

This should fix it.

Eclipse

+1 for moving up the plugin settings.

Can we please add a IS in order to make it possible somehow to describe people what they have to do, when they update their existing site?

+++ b/core/modules/block/src/BlockForm.php
@@ -33,13 +39,41 @@ class BlockForm extends EntityForm {
+   * The condition plugin manager.
+   *
+   * @var \Drupal\Core\Condition\ConditionManager
+   */
+  protected $manager;
...
+  public function __construct(EntityManagerInterface $entity_manager, ExecutableManagerInterface $manager, EventDispatcherInterface $dispatcher, LanguageManagerInterface $language) {

Do we really want to typehint here different?

I'm going to spend some time reviewing this during the week.

There is no ConditionManagerInterface, so yes we want to type hint by ExecutableManagerInterface, however we fully expect the ConditionManager to be passed to us. If we want to hint that variable the same way we hint the constructor, I can understand that, but this is the most accurate currently.

Issue Summary should be all up to date. I'm afraid resaving blocks is probably currently required in order to get the context behaving properly, however I'm not sure if that matter horribly since we're not supporting upgrade paths yet. I'll get started on a Change Notice though.

Eclipse

So ...

Change record added: https://www.drupal.org/node/2367239

I think this is really close.

I rerolled for a conflict in HEAD, and made two small tweaks (see interdiff.txt)

1. +++ b/core/modules/block/src/Plugin/DisplayVariant/FullPageVariant.php
   @@ -116,11 +129,20 @@ protected function getTheme() {
            if ($block->access('view')) {
   +          $block_plugin = $block->getPlugin();
   +          if ($block_plugin instanceof ContextAwarePluginInterface) {
   +            $this->contextHandler->applyContextMapping($block_plugin, $contexts);
   +          }
   

   Shouldn't this happen before this ->access() call?

2. +++ b/core/modules/language/src/Plugin/Condition/Language.php
   @@ -31,23 +31,23 @@ class Language extends ConditionPluginBase {
   -        '#default_value' => $this->configuration['langcodes'],
   ...
   +        '#default_value' => !empty($this->configuration['langcodes']) ? $this->configuration['langcodes'] : array(),
   ...
   -        '#value' => $this->configuration['langcodes'],
   +        '#value' => !empty($this->configuration['langcodes']) ? $this->configuration['langcodes'] : array(),
   
   @@ -98,17 +98,9 @@ public function evaluate() {
   -  public function defaultConfiguration() {
   -    return array('langcodes' => array()) + parent::defaultConfiguration();
   -  }
   

   This goes against how all plugins are written. They have default config, and you don't need !empty checks when using it. I think you're just doing this to get around export issues, ones we don't have in HEAD.

3. +++ b/core/modules/system/src/Plugin/Condition/CurrentThemeCondition.php
   @@ -97,7 +97,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   +      '#default_value' => !empty($this->configuration['theme']) ? $this->configuration['theme'] : '',
   
   +++ b/core/modules/system/src/Plugin/Condition/RequestPath.php
   @@ -98,7 +98,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   -      '#default_value' => $this->configuration['pages'],
   +      '#default_value' => !empty($this->configuration['pages']) ? $this->configuration['pages'] : '',
   
   +++ b/core/modules/user/src/Plugin/Condition/UserRole.php
   @@ -32,7 +32,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   -      '#default_value' => $this->configuration['roles'],
   +      '#default_value' => !empty($this->configuration['roles']) ? $this->configuration['roles'] : [],
   

   These too.

1.) Good catch!

2/3.) So that's not the reason at all. The reason is that as conditions stand in core today, their defaults are added to the plugin config on __construct, and this makes them exceptionally difficult to determine if they existed before this moment, which is made doubly bad by the block visibility implicitly displaying all conditions forms all the time (instead of explicitly asking for the visibility conditions you need). With that in mind, I agree that they could function this way if we had a different UI, but as it stands now, I did quite a lot of work to make it possible to determine if we needed various conditions values saved (since context mapping occurs if we do have legit values, and this can cause false negatives for block visibility if we have contexts mapped into a condition we don't actually want to have run.

Again, if the UI for this were not core's traditional UI and were instead one where users specified the conditions they wanted to have run, I wouldn't have had to do this particular thing. I can't say I'm a big fan of setting the configuration from defaults on __construct() in any event though. Manually checking empty config and falling back to sane defaults would be way safer in terms of understanding a plugin's state. Sounds like that ship may have sailed. I'm happy to discuss this further, though if we agree that block visibility UI should have an overhaul before 8.0.0, then we could focus on that next and get these back in line with other config plugins. Thoughts?

Eclipse

I went ahead and tested the revert of 2/3, and it worked fine. I also reinstated the ConditionPluginCollection.

But while digging in, I had some larger questions, especially since this is now duplicating some code from page_manager.
Hopefully I'll be able to catch up with @EclipseGc tomorrow or at BADCamp to resolve them.

A few nitpick doc cleanups.

1. +++ b/core/lib/Drupal/Core/Annotation/ContextDefinition.php
   @@ -94,9 +95,17 @@ public function __construct(array $values) {
   +    // Annotation classes extract data from passed annotation classes direct
   +    // use in the classes they pass to.
   

   s/direct use/directly used/ ?

2. +++ b/core/modules/block/src/BlockForm.php
   @@ -33,13 +38,41 @@ class BlockForm extends EntityForm {
   +   *   The EventDispatcher for gathering administrative contexts.
   

   Missing language manager @param.

3. +++ b/core/modules/block/src/BlockForm.php
   @@ -133,6 +173,82 @@ public function themeSwitch($form, FormStateInterface $form_state) {
   +   * @param FormStateInterface $form_state
   

   Missing full namespace.

I figured out part of what was bothering me.

BlockForm is using $form_state->set('context_mapping', $contexts); as a temporary place to store context *objects*.

However, the rest of the core code base, and much more so page_manager/rules, is using it for a mapping of what context represents what.
It even has a config schema entry for it.

For example, if a page_manager page is taking over /user/%, and you are placing a block to show a user, should it use the current user or the one from the URL? 'context_mapping' would store an associative array of strings, like 'user1' => 'current_user'.

Block might not have to deal with that, but it could. And regardless, for the temporary storage of context objects, it should not abuse 'context_mapping'.
Also note the difference between $form_state->getValue('context_mapping') and $form_state->get('context_mapping').

That's not changed in this patch. This addresses @kim.pepper's feedback and tweaks the existing code to match existing naming and responsibilities better. For example, ContextAwarePluginBase shouldn't deal with FormStateInterface directly.

Still a couple things left to resolve, but at least I was able to remove the weird unsetting of contexts (instead of using the persistent FAPI storage, using the temporary storage).

Also renamed clearInvalidConditions and made it more obvious by returning a boolean.

Finally, fixed the misuse of the name "context_mappings" and used "gathered_contexts" instead.

SO who will RTBC it?

It's not ready yet, I have more updates pending after a good conversation with @EclipseGc at badcamp.

We discussed readding the usage of the collection for the non-form use cases.

Ah, we have a systemic problem with copyFormValuesToEntity when used with EntityWithPluginCollectionInterface. Fixing that generically, borrowing straight from my workaround in page_manager.

First interdiff is for that fix, the second is for the changes after rerolling for #2367579: Move retrieval of visible blocks to a standalone service.

I'm trying to rewrite page_manager to use this new code, and I really feel like addContextAssignmentElement and saveContextConfiguration are in the wrong place.
Hopefully I can track down @EclipseGc and discuss this more.

Let me dump some thoughts here:

The way I have addContextAssignmentElement in page_manager is that it is a method on trait that is to be used by a form.
It takes a plugin and an array of contexts.
This way, any form can choose to add this form element for a plugin.

But in this patch, it's a method on the plugin directly, and each plugin would have to decide to call it.
In the case of page_manager, we want to make the blocks context aware. We can do that TO a block because our call is external.
If we were to only use the approach from this patch, every plugin that ever could be context-aware would have to call these methods themselves.

One option would be to make it public, but then what interface will it live on?

Well, I mean yes and no. Yes a plugin could choose to NOT call it, but it would have to NOT inherit from the base class provided by core. If someone wanted to code around the base class they could end up with contextual plugins that don't properly invoke these methods, but I don't think it's really that big a problem because this is the sort of thing that our plugin base classes usually provide. In the case of blocks this is especially true because the base class adds form elements for caching, label, etc and this would get added in exactly the same way as it is in conditions i.e. BlockBase::buildConfigurationForm() & BlockBase::submitConfigurationForm(). Every plugin won't have to opt-in to their proper context form elements. I didn't have to change any Condition plugins in this patch in this regard. The same will be true of blocks in the follow up we file for that purpose, and it's the plugin base class that intercedes on our behalf in both instances.

This doesn't mean I'm not open to discussing an alternative architecture for this problem if you think your is just vastly better and more understandable/usable, but the approach I pursued here was designed to put the work on plugin type developers and to be invisible to individual plugin developers.

Your interdiffs look good at first glance. I'll apply the patch when I get home to just follow the code and make sure I grok it all still. We can discuss these other issues and see if we can get on the page early this week.

Eclipse

Talked with @effulgentsia and @EclipseGc about this, I think I finally found the flow I wanted.

Silly mistake.

I don't think this needs any explicit committer feedback, just a solid review.

@EclipseGc expressed concern about the core plugins we know about being able to present their entire form as-is. We still want to leave addContextAssignmentElement on a trait to allow contrib to use it, but core can handle itself.

So yeah, I'm pretty happy with that.

Eclipse

Do we need a change notice for this?
Can we please ask @fago or @effulgentsia to review it?

I already wrote a change notice, it is in draft mode. See #103.

Eclipse

Over at #2370667: [Meta] user/password flame graph, the block condition checking is showing up to be extremely expensive. For that simple /user/password page, we're seeing the following, with all percentages being percentages of the total page load time:

• 0.27%: getting the context handler
• 3.82%: getting the condition contexts
• 0.21%: getting the visibility conditions
• 0.53%: resolving the visibility conditions
• 1.27%: applying the context mapping

So 6.1% of total page load time of a super simple page is spent on handling block condition/contexts. This will likely be much worse on more complex sites that have many more blocks set up.

I think we want to make sure that this doesn't cause a regression.

Also: it feels very bizarre to me that EclipseGc would be allowed to RTBC what is mostly his patch… somebody else should also review it.

We should also re-flip-flop the block access and conditions check. But that should be a follow up.

Eclipse

Playing with this, I ran into another bug, so definitely ++ing the NW status.

Eclipse

@Wim Leers
The silly fact that we do N*M context getting is just silly, totally agreed. Therefore I opened up #2354889: Make block context faster by removing onBlock event and replace it with loading from a ContextManager which is not part of this issue
though.

[deleted]

I'm rerunning all these numbers. Let's pretend I didn't post anything in this regard yet. :-)

Eclipse

This addresses the two bugs @EclipseGc found.
First was that removing a visibility condition through the UI was not working.
The second was that no-op, unconfigured conditions were still being stored in the entity.

I added test coverage for both of those cases.

Self-review!

1. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -300,12 +300,6 @@ block_settings:
          label: 'View mode'
   -    visibility:
   -      type: sequence
   -      label: 'Visibility Conditions'
   -      sequence:
   -        - type: condition.plugin.[id]
   -          label: 'Visibility Condition'
   
   +++ b/core/modules/block/config/schema/block.schema.yml
   @@ -24,6 +24,12 @@ block.block.*:
          type: block.settings.[%parent.plugin]
   +    visibility:
   +      type: sequence
   +      label: 'Visibility Conditions'
   +      sequence:
   +        - type: condition.plugin.[id]
   +          label: 'Visibility Condition'
   

   This is moved. Still block specific, but just entity not plugin.

2. +++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
   @@ -125,4 +125,28 @@ public function setContextValue($name, $value);
   +  public function getContextMapping();
   ...
   +  public function setContextMapping($context_mapping);
   

   These two methods elegantly solve the problem of not being able to rely on ConfigurablePluginInterface.

3. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -107,13 +83,6 @@ public function setConfiguration(array $configuration) {
   -    unset($visibility['current_theme']);
   
   +++ b/core/modules/block/src/BlockForm.php
   @@ -133,6 +179,82 @@ public function themeSwitch($form, FormStateInterface $form_state) {
   +      // Don't display the current theme condition.
   +      if ($condition_id == 'current_theme') {
   +        continue;
   +      }
   

   This is still present, just in a different form.

4. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -287,53 +226,6 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   -    foreach ($this->getVisibilityConditions() as $condition_id => $condition) {
   
   @@ -362,15 +254,6 @@ public function validateConfigurationForm(array &$form, FormStateInterface $form
   -    foreach ($this->getVisibilityConditions() as $condition_id => $condition) {
   
   @@ -394,14 +277,6 @@ public function submitConfigurationForm(array &$form, FormStateInterface $form_s
   -      foreach ($this->getVisibilityConditions() as $condition_id => $condition) {
   
   +++ b/core/modules/block/src/BlockForm.php
   @@ -133,6 +179,82 @@ public function themeSwitch($form, FormStateInterface $form_state) {
   +    foreach ($this->manager->getDefinitions() as $condition_id => $defintion) {
   
   @@ -154,6 +276,28 @@ public function validate(array $form, FormStateInterface $form_state) {
   +    foreach ($form_state->getValue('visibility') as $condition_id => $values) {
   
   @@ -173,6 +317,24 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +    foreach ($form_state->getValue('visibility') as $condition_id => $values) {
   

   This is one of the major parts of the fix. We no longer blindly add all conditions to the plugin collection for runtime usage, but instead gather all plugins in the form instead.

5. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -503,61 +378,4 @@ public function isCacheable() {
   -  public function getVisibilityConditions() {
   ...
   -  public function getVisibilityCondition($instance_id) {
   ...
   -  public function setVisibilityConfig($instance_id, array $configuration) {
   
   +++ b/core/modules/block/src/Entity/Block.php
   @@ -186,8 +218,69 @@ public function getCacheTags() {
   +  public function setVisibilityConfig($instance_id, array $configuration) {
   ...
   +  public function getVisibilityConditions() {
   ...
   +  public function getVisibilityCondition($instance_id) {
   

   This is moved from the plugin to the entity

6. +++ b/core/lib/Drupal/Core/Condition/ConditionPluginBase.php
   @@ -41,6 +44,9 @@ public function isNegated() {
   +    $temporary = $form_state->getTemporary();
   +    $contexts = isset($temporary['gathered_contexts']) ? $temporary['gathered_contexts'] : [];
   
   +++ b/core/modules/block/src/BlockForm.php
   @@ -63,8 +102,15 @@ public function form(array $form, FormStateInterface $form_state) {
   +    // Store the gathered contexts in the form state for other objects to use
   +    // during form building.
   +    $temporary = $form_state->getTemporary();
   +    $temporary['gathered_contexts'] = $this->dispatcher->dispatch(BlockEvents::ADMINISTRATIVE_CONTEXT, new BlockContextEvent())->getContexts();
   +    $form_state->setTemporary($temporary);
   

   This is a very useful place to put the contexts. Also #2371853: Add more helper methods around temporary FAPI storage would make this easier to read and understand

7. +++ b/core/lib/Drupal/Core/Entity/EntityForm.php
   @@ -291,11 +291,18 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   +    $keys_to_skip = [];
   +    if ($this->entity instanceof EntityWithPluginCollectionInterface) {
   +      // Do not manually update values represented by plugin collections.
   +      $keys_to_skip += array_keys($this->entity->getPluginCollections());
   +    }
        // @todo: This relies on a method that only exists for config and content
        //   entities, in a different way. Consider moving this logic to a config
        //   entity specific implementation.
        foreach ($form_state->getValues() as $key => $value) {
   -      $entity->set($key, $value);
   +      if (!in_array($key, $keys_to_skip)) {
   +        $entity->set($key, $value);
   +      }
   

   This approach is borrowed directly from page_manager, and should have been added alongside the EntityWithPluginCollection originally. It prevents block, image, page_manager and all of contrib from having to remember this VERY important step.

8. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginAssignmentTrait.php
   @@ -0,0 +1,72 @@
   +trait ContextAwarePluginAssignmentTrait {
   

   This class is 1:1 out of page_manager, and mimics what @EclipseGc had written, just changing that you pass the plugin to it.

9. +++ b/core/modules/block/js/block.js
   @@ -26,10 +26,10 @@ function checkboxesSummary(context) {
   -      $('#edit-settings-visibility-node-type, #edit-settings-visibility-language, #edit-settings-visibility-user-role').drupalSetSummary(checkboxesSummary);
   +      $('#edit-visibility-node-type, #edit-visibility-language, #edit-visibility-user-role').drupalSetSummary(checkboxesSummary);
   

   There are a fair number of changes like these, just because visibility is moving up a level.

10. +++ /dev/null
    @@ -1,39 +0,0 @@
    -class BlockConditionContextEvent extends Event {
    
    +++ b/core/modules/block/src/Event/BlockContextEvent.php
    @@ -0,0 +1,53 @@
    +class BlockContextEvent extends Event {
    

    This is a much better name, and less coupled to the conditions use case. A good step towards #2349679: Support registration of global context

11. +++ b/core/modules/block/src/Event/BlockEvents.php
    @@ -16,8 +16,16 @@
    +  const ACTIVE_CONTEXT = 'block.active_context';
    ...
    +  const ADMINISTRATIVE_CONTEXT = 'block.administrative_context';
    

    The split from one event to two is extremely important, I'm glad we're tackling that here.

12. +++ b/core/modules/block/src/EventSubscriber/CurrentUserContext.php
    @@ -50,12 +51,23 @@ public function __construct(AccountInterface $account, EntityManagerInterface $e
    +  public function onBlockAdministrativeContext(BlockContextEvent $event) {
    +    $this->onBlockActiveContext($event);
       }
    
    +++ b/core/modules/block/src/EventSubscriber/NodeRouteContext.php
    @@ -37,20 +38,28 @@ public function __construct(RouteMatchInterface $route_match) {
    +  public function onBlockAdministrativeContext(BlockContextEvent $event) {
    +    $context = new Context(new ContextDefinition('entity:node'));
    +    $event->setContext('node', $context);
    +  }
    

    Many of them will just call the active handling during the administrative, but not always. This is why we need this

Just for the sake of moving this along, Tim and I are both just waiting for an RTBC on this.

Assigning to effulgentsia to see if I can prompt a review. :-D

Eclipse

Rerolled on top of #2352155: Remove HtmlFragment/HtmlPage.

Still digesting this patch. Some preliminary thoughts/questions:

1. +++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
   @@ -125,4 +125,28 @@ public function setContextValue($name, $value);
   +  public function getContextMapping();
   +  public function setContextMapping($context_mapping);
   

   At first glance, this seems like adding unnecessary coupling. Why should the plugin be in any way responsible for the context mappings? Shouldn't that be entirely managed from the outside? Looking at the patch, it doesn't look to me like these are called in many places, and where they are, I think the caller can manage the storage/retrieval itself without delegating to the plugin. Or am I missing something?

2. +++ b/core/modules/block/src/BlockInterface.php
   @@ -40,4 +40,53 @@ public function getPlugin();
   +   * @return \Drupal\Core\Condition\ConditionInterface[]|\Drupal\Core\Condition\ConditionPluginCollection
   

   Looks to me like we always return the latter. Do we still need to support the former?

3. +++ b/core/modules/block/src/BlockInterface.php
   @@ -40,4 +40,53 @@ public function getPlugin();
   +  public function getAvailableContexts();
   +  public function setAvailableContexts(array $contexts);
   

   Does the word "Available" add any meaning here? BlockRepository::getVisibleBlocksPerRegion() is simply passed a $contexts variable (not $available_contexts), and that's what it passes to $block->setAvailableContexts(), so I'm confused what "available" is disambiguating. AFAICT, a block entity has no contexts that it deals with other than the available ones, or is that not true?

1) I know we discussed this some at BADCamp, and while it may seem unnecessary, this is definitely codifying what is currently done in HEAD.
Another object, like the block entity, could certainly maintain this mapping for the visibility. But then you'd end up with a YAML structure like

id: block_whatever
...
visibility:
  user_role:
    roles:
      authenticated_user: authenticated_user
context_mapping:
  user_role:
    user: current_user

Instead of

id: block_whatever
...
visibility:
  user_role:
    roles:
      authenticated_user: authenticated_user
    context_mapping:
      user: current_user

Which means one less custom schema, one less thing to be sure of adding and removing when a condition is changed, and one less new property on the entity.

This may not seem like a lot, but when considering page manager, they have access conditions, selection conditions, and visibility conditions. That becomes a lot to manage.

Also, a condition once configured still requires its context_mapping in order to function. I would consider that part of its configuration.

2) We need that for iterating over the plugin collection, which we do in BlockAccessControlHandler for example

3) Honestly I'm not sure. @EclipseGc, any opinion?

Partial review. Posting it before dreditor decides to crash.

1. +++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
   @@ -125,4 +125,28 @@ public function setContextValue($name, $value);
   +  /**
   +   * Returns a mapping of the expected assignment names to their context names.
   +   *
   +   * @return array
   +   *   A mapping of the expected assignment names to their context names. For
   +   *   example, if one of the $contexts is named 'entity', but the plugin
   +   *   expects a context named 'node', then this map would contain
   +   *   'entity' => 'node'.
   +   */
   +  public function getContextMapping();
   

   I think a better example would help clarify this more. Perhaps 'user' and the possibility of multiple things mapping to it (like 'current_user' and whatever the other ones are named).

2. +++ b/core/lib/Drupal/Core/Annotation/ContextDefinition.php
   @@ -94,9 +95,17 @@ public function __construct(array $values) {
   +    // Annotation classes extract data from passed annotation classes directly
   +    // used in the classes they pass to.
   +    foreach (['label', 'description'] as $key) {
   +      if (isset($values[$key]) && $values[$key] instanceof TranslationWrapper) {
   +        $values[$key] = (string) $values[$key]->get();
   +      }
   +      else {
   +        $values[$key] = NULL;
   +      }
   +    }
   

   Let's add a @todo to the __toString() issue, so we remember to remove this ugliness.

3. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -152,25 +120,8 @@ public function calculateDependencies() {
   -    // This should not be hardcoded to an uncacheable access check result, but
   -    // in order to fix that, we need condition plugins to return cache contexts,
   -    // otherwise it will be impossible to determine by which cache contexts the
        // result should be varied.
   

   We either need to keep the entire comment, or remove the entire comment. If we remove the comment (since condition checking is now done elsewhere), we should also remove the setCacheable(FALSE) that's at the end of this function.

4. +++ b/core/lib/Drupal/Core/Condition/ConditionPluginCollection.php
   @@ -41,6 +41,10 @@ public function getConfiguration() {
   +      // For the purposes of comparison, remove the context mapping. If the
   +      // current configuration matches the default configuration, the context
   +      // mapping was not used anyway.
   +      unset($instance_config['context_mapping']);
   

   Comment could be improved to explain:
   - why default config means that context mapping wasn't used
   - why remove the context mapping from the comparison (performance? some other problem?)

5. +++ b/core/lib/Drupal/Core/Entity/EntityForm.php
   @@ -291,11 +291,18 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
        foreach ($form_state->getValues() as $key => $value) {
   -      $entity->set($key, $value);
   +      if (!in_array($key, $keys_to_skip)) {
   +        $entity->set($key, $value);
   

   Minor, but can we do an array_intersect_key() instead of repeated in_array() calls?

6. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -33,8 +85,32 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
   +      // This should not be hardcoded to an uncacheable access check result, but
   +      // in order to fix that, we need condition plugins to return cache contexts,
   +      // otherwise it will be impossible to determine by which cache contexts the
   +      // result should be varied.
   

   Let's move this comment to just above where we actually call setCacheable(FALSE). Bonus points for also filing an issue and adding a @todo linking to it.

7. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -33,8 +85,32 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
   +      return $access->setCacheable(FALSE)->cacheUntilEntityChanges($entity);
   

   The combination of setCacheable(FALSE) and cacheUntil...() is very confusing. If the idea is to ensure we do the latter when we resolve the @todo about the former, then perhaps that would be better to express in the comment rather than in the code?

8. +++ b/core/modules/block/src/BlockRepository.php
   @@ -65,12 +70,20 @@ protected function getTheme() {
   +      // Set the contexts on the block before checking access.
   +      $block->setAvailableContexts($contexts);
   +      $block_plugin = $block->getPlugin();
   +      if ($block_plugin instanceof ContextAwarePluginInterface) {
   +        $this->contextHandler->applyContextMapping($block_plugin, $contexts);
   +      }
   

   A comment explaining why we're setting contexts on both the entity and the plugin would be helpful. For example, is it because there might be use cases of the plugin implementing additional access logic based on context? If not, and the plugin context is only for preparing the plugin prior to returning it, perhaps we should move its context assignment to after the access check to clarify that?

9. +++ b/core/modules/block/src/Event/BlockContextEvent.php
   @@ -0,0 +1,53 @@
   + * Wraps block contexts in order for event subscribers to add context.
   

   I'm confused by this sentence.

The rest of the patch looks good to me. Just a couple other docs nits:

1. +++ b/core/modules/block/src/EventSubscriber/BlockContextSubscriberBase.php
   @@ -0,0 +1,56 @@
   +   * This allows empty contexts to be returned at configuration time in order
   +   * to properly configure plugins. This also opens the door to multiple
   +   * context availability during both administration and run time in order to
   +   * provide nuanced configuration options. Empty context definitions can be
   +   * added in this way:
   +   * @code
   +   *   $context = new Context(new ContextDefinition('entity:node'));
   +   *   $event->setContext('node', $context);
   +   * @endcode
   +   *
   +   * The above example would create a new empty node context definition inside
   +   * of a Context object which can be added by an arbitrary name on the event
   +   * for the block UI to use to determine which plugins are available, and what
   +   * their default/available context(s) might be.
   

   This could be improved/clarified. If someone else wants to take a crack at doing so, go for it. Or else, I might when I have a chance.

2. +++ b/core/modules/block/src/EventSubscriber/CurrentUserContext.php
   @@ -50,12 +51,23 @@ public function __construct(AccountInterface $account, EntityManagerInterface $e
   +   * Since administrative contexts are not executed against and any user object
   +   * will work, and we always have a current user, we can simply execute the
   +   * active context method.
   

   Can be removed.

If possible, it would be great to have confirmation from @Gábor that there is sufficient test coverage here for the original multilingual problems that started this issue. I think there is, but I could easily have overlooked something.

Unassigning myself until #136.3 and #138 - #140 are addressed.

I'm ok with #137's answers to #136.1 and #136.2, given that those are pre-existing conditions of HEAD so not part of this issue's scope to change.

#138
1) fixed
2) added
3) I opened #2375689: BlockBase::blockAccess() should return AccessResult instead of a bool, because we still have to mark this uncacheable.
4) I tried to rewrite this.
5) Rewrote this, and added a unit test (should have done that earlier!)
6) Moved this.
7) Added an @todo pointing to #2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed and adjusted the code.
8) Actually, this is redundant. I think we ended up with two calls after the rerolls for BlockRepository and HtmlPage. The setAvailableContexts() call is so that BlockAccessControlHandler can retrieve them, so it can apply them to the plugins. Fixed.
9) Tried to fix this some. An additional thought about this, maybe the block can passed to this event, which might help resolve #2374295: Get the block context in a condition plugin?

#139
1) Not sure what to do with this, leaving for @effulgentsia
2) Removed

#140
@gabor wrote the test coverage, and it seems complete to me.

1. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -110,7 +106,13 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
   -      return $access->setCacheable(FALSE)->cacheUntilEntityChanges($entity);
   

   So, neither Tim, nor myself added this hunk, it was just moved from where it was. HEAD does this today. Are we sure we want to change it?

2. +++ b/core/modules/block/src/BlockRepository.php
   @@ -78,13 +78,7 @@ public function getVisibleBlocksPerRegion(array $contexts) {
   -      $block_plugin = $block->getPlugin();
   -      if ($block_plugin instanceof ContextAwarePluginInterface) {
   -        $this->contextHandler->applyContextMapping($block_plugin, $contexts);
   -      }
   

   Whoa!? When did we add this? I mean I'm all for it, but we haven't expose the UI for block contexts yet, so this won't function till we do.

136.3: get/setAvailableContexts(). Let's use the same naming convention the events use and call this get/setActiveContexts(). I don't think there's a custom method for this in the form for get/setAdministrativeContexts(). We might consider making a trait out of either or both of these in a follow up.

138.3: Conditions should not care about cache-ability of access. They have no notion of this 3-option system. Conditions return TRUE or they return FALSE. A condition does not have 'no opinion'. The block system cares about cache-ability, and should be the one to determine whether or not a user has access to this block. Conditions just help to inform it what decision to make. We've discussed this before I think. Think of conditions outside of blocks before doing stuff to it. Rules has no need of condition cache-ability independent of what it's own wrapping implementation may choose to do. Let's not get in the way of that by being more opinionated than a simple boolean. (If I'm missing the point here, lets spend some time bringing me up to speed in irc or something)

138.7: As I said in the code comment above, neither tim, nor I added this. This is what HEAD does currently. I just moved it to its current location. In contrast to my opinion in 138.3, I think it makes perfect sense to annotate the cache contexts, however I'd like to understand how this is different from the actual context itself. I suppose this is probably the difference between varying on role and varying by user?

138.8: As I said in the code comment above, this actually applies contexts to the blocks. It won't do anything till we have a UI in place for mapping contexts to the block. I'm fine with removing it, but we'll be re-adding it in a follow up. Not sure how/when it got added here. I don't recall doing that. :-D

138.9: Tim, I've been thinking about that particular issue for a while. Since we're passing the array of contexts in (not by reference) I think each block could add its own block entity to the context array for its own conditions. Definitely follow-up worthy, but I don't think it changes the implementation here at all.

Yes, Gabor wrote the test coverage, so I think we're covered.

Eclipse

Added a test for the multilingual bit.
Renamed getAvailableContexts to getContexts
Adjusted how buildVisibilityInterface works, with @EclipseGc's blessing.

Interdiff looks great!

Eclipse

Noob mistake, I uploaded those patches in the wrong order.

RTBC?

Eclipse

This addresses #139.1 and the other instance of #138.7.

+++ b/core/modules/block/src/BlockForm.php
@@ -63,8 +102,15 @@ public function form(array $form, FormStateInterface $form_state) {
+    $form['visibility'] = $this->buildVisibilityInterface([], $form_state);
...
   /**
+   * Helper function for building the visibility UI form.
+   *
+   * @param array $form
+   *   An associative array containing the structure of the form.
+   * @param \Drupal\Core\Form\FormStateInterface $form_state
+   *   The current state of the form.
+   *
+   * @return array
+   *   The form array with the visibility UI added in.
+   */
+  protected function buildVisibilityInterface(array $form, FormStateInterface $form_state) {
+    $form['visibility_tabs'] = [

Do we have other places where we use a pattern of having a $form parameter actually be only a part of a form? For WidgetInterface::settingsForm(), we pass a complete $form in, but the function returns a $element that the caller inserts into the right place. Should we change this to the same pattern as that?

+++ b/core/modules/block/src/BlockForm.php
@@ -63,8 +102,15 @@ public function form(array $form, FormStateInterface $form_state) {
     $form['settings'] = $entity->getPlugin()->buildConfigurationForm(array(), $form_state);
+    $form['visibility'] = $this->buildVisibilityInterface([], $form_state);

Look at the line directly above the hunk you quote.

We do this in several places, where the subform doesn't need to know anything about the parent form.

I'm great with everything I'm seeing here. Effulgentsia asked me for a "Number of Function Calls" diff from xhprof. I'm doing this in a vm, so my wall times are worthless but the function diff is static. All tests were performed against /user/login as an anonymous user.

Fresh Install of Drupal 8 vs D8 with this patch: (Blocks as configured by standard profile out of the box)

Powered by drupal block dependent upon authenticated user:

I tend to think there's a performance improvement in here as well, but w/o a good environment to actually test it on, I can't prove it. Just given how things were re-arranged, I was expecting it.

Eclipse

I think all feedback has been addressed.

First: the numbers in #153 look great! :) I can't wait for #2370667: [Meta] user/password flame graph to be redone once this lands, looks like this addresses most of the poor performance we found there, as I described in #126. AFAICT the reason is that we now dispatch BlockEvents::ACTIVE_CONTEXT once, rather than dispatching BlockEvents::CONDITION_CONTEXT once per block? Please confirm.

Second: I'm very glad to see the visibility key moved out of the settings key in block config entities. I have an off-topic question about that for you all: a long time ago, when I was working with msonnabaum and others on refactoring block rendering and making them cacheable, we did the same for the cache key as was the case for the visibility key: we put it under settings. I remember disliking it and running into nasty things with Form API because of that (since the cacheability settings are shared, but the other settings aren't). Do we want to move cache out of settings, just like this patch does for visibility?

I also went over the patch once more, to understand the performance improvement above, but in doing so I found a bunch of nitpicks. Sorry.

1. +++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
   @@ -125,4 +125,28 @@ public function setContextValue($name, $value);
   +  public function setContextMapping($context_mapping);
   

   Typehint to array, to match the docblock.

2. +++ b/core/lib/Drupal/Core/Condition/ConditionPluginCollection.php
   @@ -41,6 +41,12 @@ public function getConfiguration() {
   +      // its default. The default configuration of a plugin does not contain
   

   s/its default/its default configuration/
   I find these fiveish lines of comments very difficult to understand, but I think it's actually a trivial thing. It's a case where explaining it clearly is more difficult than doing it. So it's probably ok.

3. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -7,23 +7,75 @@
   +   * @param EntityTypeInterface $entity_type
   ...
   +   * @param ExecutableManagerInterface $manager
   ...
   +   * @param ContextHandlerInterface $context_handler
   

   Needs fully qualified interfaces.

4. +++ b/core/modules/block/src/BlockForm.php
   @@ -133,6 +179,80 @@ public function themeSwitch($form, FormStateInterface $form_state) {
   +      if ($condition_id == 'current_theme') {
   ...
   +      if ($condition_id == 'language' && !$this->language->isMultilingual()) {
   

   Why not use strict equality?

5. +++ b/core/modules/block/src/BlockForm.php
   @@ -133,6 +179,80 @@ public function themeSwitch($form, FormStateInterface $form_state) {
   +    // @todo Determine if there is a better way to rename the conditions.
   

   Either the TODO should be resolved or there should be an issue for it?

6. +++ b/core/modules/block/src/EventSubscriber/BlockContextSubscriberBase.php
   @@ -0,0 +1,75 @@
   +   * @param BlockContextEvent $event
   ...
   +   * @param BlockContextEvent $event
   

   Should be a fully qualified class.

This patch takes care of everything except 156.4 since we never really use strict equality for strings since they don't need to be evaluated this way.

Yes, the big performance boost here (at least in number of function calls made) is largely due to the fact that contexts are only requested once and not per block.

Yes I'd love to explore moving the cache form element out of the block settings too. I really firmly believe that's a completely separate task and we need to discuss that, but that's a completely separate topic.

Eclipse

My bad.

Eclipse

I really firmly believe that's a completely separate task and we need to discuss that, but that's a completely separate topic.

Of course! That's why I said it's an off-topic question :)

1. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
   @@ -73,12 +73,7 @@ public function getMatchingContexts(array $contexts, ContextDefinitionInterface
   -    if ($plugin instanceof ConfigurablePluginInterface) {
   

   Can remove use Drupal\Component\Plugin\ConfigurablePluginInterface;

2. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginAssignmentTrait.php
   @@ -0,0 +1,72 @@
   +    foreach ($plugin->getContextDefinitions() as $context_slot => $definition) {
   +      $valid_contexts = $this->contextHandler()->getMatchingContexts($contexts, $definition);
   

   getContextDefintions() returns \Drupal\Component\Plugin\Context\ContextDefinitionInterface[] but getMatchingContexts() accepts \Drupal\Core\Plugin\Context\ContextDefinitionInterface. I guess that the latter should be changed to the less specific interface.

3. +++ b/core/modules/block/src/BlockForm.php
   @@ -133,6 +179,79 @@ public function themeSwitch($form, FormStateInterface $form_state) {
   +    foreach ($this->manager->getDefinitions() as $condition_id => $defintion) {
   

   $definition

4. +++ b/core/modules/block/src/BlockRepository.php
   @@ -7,7 +7,9 @@
   +use Drupal\Component\Plugin\ContextAwarePluginInterface;
   

   Not used

5. +++ b/core/modules/block/src/EventSubscriber/CurrentLanguageContext.php
   @@ -39,10 +40,24 @@ public function __construct(LanguageManagerInterface $language_manager) {
   +        $event->setContext($type_key, $context);
   

   Use $type_key looks fragile and has a high potential for collisions, no? Shouldn't we be keying by provider and a key? ie. something like language.$type_key or maybe block.$type_key since this is provided by the block module. I guess that context names don't have a namespacing strategy in HEAD either. This looks fragile.

6. +++ b/core/modules/block/src/Tests/BlockLanguageTest.php
   @@ -7,6 +7,7 @@
   +use Drupal\Core\Language\LanguageInterface;
   

   Not used

7. +++ b/core/profiles/standard/src/Tests/StandardTest.php
   @@ -7,6 +7,7 @@
   +use Drupal\block\Entity\Block;
   

   Unused.

Ok, punted on 162.2 since that exists in HEAD, I fixed all the rest. The main change here is around 162.5. Alex asked for a provider.context_id notation. I set these up as though they were from the code base that will eventually provide the events. These are all in block currently, but as we transition to a more global context setup, these will move. It doesn't hurt us either way though, and they're more specific now.

Eclipse