ContextHandler incorrectly checks required/optional contexts of plugins

Looks good.

Just a small remark about the code structure.

1. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
   @@ -74,14 +74,35 @@ public function getMatchingContexts(array $contexts, ContextDefinitionInterface
   +        // Collect contexts that exist but are missing a value.
   +        if ($plugin_context_definition->isRequired() && !$contexts[$context_id]->hasContextValue()) {
   +          $missing_value[] = $plugin_context_id;
   +          continue;
   +        }
   +
   +        if ($contexts[$context_id]->hasContextValue()) {
   +          $plugin->setContextValue($plugin_context_id, $contexts[$context_id]->getContextValue());
   +        }
   

   Wouldn't "if/else" be clearer then "if,continue,if"?

2. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
   @@ -74,14 +74,35 @@ public function getMatchingContexts(array $contexts, ContextDefinitionInterface
   +        continue;
   

   This "continue" seems similarly unnecessary.

+++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
@@ -70,6 +70,13 @@ public function getContextValue() {
+    return $this->contextValue || $this->getContextDefinition()->getDefaultValue();

+++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
@@ -59,6 +59,13 @@ public function getContextValue() {
+    return $this->contextData || parent::hasContextValue();

Need some explicit (bool) castings.

Worth noting that berdir's code tip-toes around the plugin's implementation of setContextValue() because that method can throw an exception. It needs to continue to throw that exception in the case of someone not using the ContextHandler to set contexts on an object, so I approve whole-heartedly of the approach here. ++ all around, change the boolean problems and I'm very on board.

Eclipse

Here's a review with minor stuff. This looks great to me, but I think only EclipseGc/tim.plunkett can RTBC this. At least definitely not me.

1. +++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
   @@ -70,6 +70,13 @@ public function getContextValue() {
   +    return (bool) $this->contextValue || (bool) $this->getContextDefinition()->getDefaultValue();
   

   Nit: I'd have expected

   (bool) (A || B)
   

2. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
   @@ -74,14 +74,36 @@ public function getMatchingContexts(array $contexts, ContextDefinitionInterface
   +    // if there are any required contexts without a value, throw an exception.
   

   Nit: s/if/If/

3. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
   @@ -74,14 +74,36 @@ public function getMatchingContexts(array $contexts, ContextDefinitionInterface
   +      throw new ContextException(SafeMarkup::format("Required contexts without a value: @contexts.", array('@contexts' => implode(', ', $missing_value))));
   

   We use sprintf() for these.

   (Yes, much code is doing this incorrectly, but Alex Pott will push it back for that.)

4. +++ b/core/modules/system/tests/modules/condition_test/src/Plugin/Condition/OptionalContextCondition.php
   @@ -0,0 +1,40 @@
   + *     "node" = @ContextDefinition("entity:node", label = @Translation("Node"), required = FALSE),
   ...
   +class OptionalContextCondition extends ConditionPluginBase {
   

   Name mismatch?

1. Ok.
2. \
3. I think in this case it's fine to just use sprintf() for this one. It's not as jarring as seeing different array syntax.
4. A comment would be okay too probably. But right now it's just confusing.

Looks good to me and is passing. This is a big oops, the sooner this is fixed the better.

Eclipse

+++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
@@ -74,14 +74,36 @@ public function getMatchingContexts(array $contexts, ContextDefinitionInterface
+      throw new ContextException(sprintf('Required contexts without a value: %s.', implode(', ', $missing_value)));

This method already has @throws, so we're good here.

The unit and KTB tests are also looking good.

+1 for RTBC!

+++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
@@ -74,14 +74,36 @@ public function getMatchingContexts(array $contexts, ContextDefinitionInterface
       if (!empty($contexts[$context_id])) {
         // This assignment has been used, remove it.
         unset($mappings[$plugin_context_id]);
-        $plugin->setContextValue($plugin_context_id, $contexts[$context_id]->getContextValue());
+
+        // Collect contexts that exist but are missing a value.
+        if ($plugin_context_definition->isRequired() && !$contexts[$context_id]->hasContextValue()) {
+          $missing_value[] = $plugin_context_id;
+        }
+        elseif ($contexts[$context_id]->hasContextValue()) {
+          $plugin->setContextValue($plugin_context_id, $contexts[$context_id]->getContextValue());
+        }
       }
+      elseif ($plugin_context_definition->isRequired()) {
+        $missing_value[] = $plugin_context_id;
+      }
+      else {
+        // Ignore mappings for optional missing context.
+        unset($mappings[$plugin_context_id]);
+      }

Uber nit - As the logic is now the $contexts[$context_id]->hasContextValue() will be executed two times.

Using:

if ($contexts[$context_id]->hasContextValue()) {
  $plugin->set...
}
elseif ($plugin_context_definition->isRequired()) {
}

would avoid that.

----

Written that the whole logic can be even more simplified:

- Move unset() of the mappings out of condition.

=>

 if (!empty($contexts[$context_id]) && $contexts[$context_id]->hasContextValue()) {
  $plugin->setContextValue(...);
}
elseif (plugin_context_definition->isRequired()) {
  $missing_value[] = ...;
}
unset($mappings[$plugin_context_id]);

That should make that code way simpler to read ...

Leaving at RTBC as a pure refactoring this could be follow-up.

Also: RTBC + 1

Okay, could we still try to avoid calling ->hasContextValue() twice? (e.g. the first suggestion)

Back to RTBC, thanks for fixing!

Doesn't check if it's required if it's present +1 to rtbc.

Eclipse

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 05aa5ea and pushed to 8.0.x. Thanks!

Interesting, this broke the Rules unit tests for node conditions: https://travis-ci.org/fago/rules/builds/69448449

Thanks to git bisect I was able to trace it down to this issue.

Not sure what is going on at all here since https://github.com/fago/rules/blob/8.x-3.x/tests/src/Integration/Conditi... only makes use of setContextValue() and nothing else. What I found so far is that the ->isPromoted() method on my mocked node has already been called twice when the condition is evaluated, which does not make sense. Will report back here once I make progress in tracking this down.

Actually the test fails started with the commits from #2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed.

This is now fixed in Rules: https://github.com/fago/rules/commit/cfa9337d05d5b7944d50a372d33e14d14a5...

Moral of the story: don't use $this->at() in a PHPUnit test unless you know what you are doing. It can behave in unexpected ways. In our case additional methods are called on the node mock object ("getCacheContexts" and others) which PHPUnit matched to the configured method calls with ->at().