Issue #2513244 by Berdir, Wim Leers, EclipseGc, Fabianx: ContextHandler...

Issue #2513244 by Berdir, Wim Leers, EclipseGc, Fabianx: ContextHandler incorrectly checks required/optional contexts of plugins

core/lib/Drupal/Component/Plugin/Context/Context.php

@@ -67,6 +67,13 @@ public function getContextValue() {
     return $this->contextValue;
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  public function hasContextValue() {
+    return (bool) $this->contextValue || (bool) $this->getContextDefinition()->getDefaultValue();
+  }
+
   /**
    * {@inheritdoc}
    */

core/lib/Drupal/Component/Plugin/Context/ContextInterface.php

@@ -30,6 +30,14 @@ public function setContextValue($value);
    */
   public function getContextValue();
 
+  /**
+   * Returns whether the context has a value.
+   *
+   * @return bool
+   *   TRUE if the context has a value, FALSE otherwise.
+   */
+  public function hasContextValue();
+
   /**
    * Sets the definition that the context must conform to.
    *

core/lib/Drupal/Core/Plugin/Context/Context.php

@@ -56,6 +56,13 @@ public function getContextValue() {
     return $this->getTypedDataManager()->getCanonicalRepresentation($this->contextData);
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  public function hasContextValue() {
+    return (bool) $this->contextData || parent::hasContextValue();
+  }
+
   /**
    * {@inheritdoc}
    */

core/lib/Drupal/Core/Plugin/Context/ContextHandler.php

@@ -74,16 +74,40 @@ public function getMatchingContexts(array $contexts, ContextDefinitionInterface
   public function applyContextMapping(ContextAwarePluginInterface $plugin, $contexts, $mappings = array()) {
     $mappings += $plugin->getContextMapping();
     // Loop through each of the expected contexts.
-    foreach (array_keys($plugin->getContextDefinitions()) as $plugin_context_id) {
+
+    $missing_value = [];
+
+    foreach ($plugin->getContextDefinitions() as $plugin_context_id => $plugin_context_definition) {
       // If this context was given a specific name, use that.
       $context_id = isset($mappings[$plugin_context_id]) ? $mappings[$plugin_context_id] : $plugin_context_id;
       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());
+
+        // Pass the value to the plugin if there is one.
+        if ($contexts[$context_id]->hasContextValue()) {
+          $plugin->setContextValue($plugin_context_id, $contexts[$context_id]->getContextValue());
+        }
+        elseif ($plugin_context_definition->isRequired()) {
+          // Collect required contexts that exist but are missing a value.
+          $missing_value[] = $plugin_context_id;
+        }
+      }
+      elseif ($plugin_context_definition->isRequired()) {
+        // Collect required contexts that are missing.
+        $missing_value[] = $plugin_context_id;
+      }
+      else {
+        // Ignore mappings for optional missing context.
+        unset($mappings[$plugin_context_id]);
       }
     }
 
+    // If there are any required contexts without a value, throw an exception.
+    if ($missing_value) {
+      throw new ContextException(sprintf('Required contexts without a value: %s.', implode(', ', $missing_value)));
+    }
+
     // If there are any mappings that were not satisfied, throw an exception.
     if (!empty($mappings)) {
       throw new ContextException(SafeMarkup::format('Assigned contexts were not satisfied: @mappings', ['@mappings' => implode(',', array_keys($mappings))]));

core/modules/system/tests/modules/condition_test/src/Plugin/Condition/OptionalContextCondition.php

+<?php
+
+/**
+ * @file
+ * Contains \Drupal\condition_test\Plugin\Condition\OptionalContextCondition.
+ */
+
+namespace Drupal\condition_test\Plugin\Condition;
+
+use Drupal\Core\Condition\ConditionPluginBase;
+
+/**
+ * Provides a condition with an optional node context.
+ *
+ * The context type entity:node is used since that would allow to also use this
+ * for web tests with the node route context.
+ *
+ * @Condition(
+ *   id = "condition_test_optional_context",
+ *   label = @Translation("Optional context"),
+ *   context = {
+ *     "node" = @ContextDefinition("entity:node", label = @Translation("Node"), required = FALSE),
+ *   }
+ * )
+ */
+class OptionalContextCondition extends ConditionPluginBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function evaluate() {
+    // Grant access if no context value is given.
+    return !$this->getContextValue('node');
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function summary() {
+    return $this->t('Context with optional context.');
+  }
+
+}

core/modules/system/tests/modules/condition_test/src/Tests/OptionalContextConditionTest.php

+<?php
+
+/**
+ * @file
+ * Contains \Drupal\condition_test\Tests\OptionalContextConditionTest.
+ */
+
+namespace Drupal\condition_test\Tests;
+
+use Drupal\Core\Plugin\Context\Context;
+use Drupal\Core\Plugin\Context\ContextDefinition;
+use Drupal\node\Entity\Node;
+use Drupal\simpletest\KernelTestBase;
+
+/**
+ * Tests a condition with optional context.
+ *
+ * @group condition_test
+ */
+class OptionalContextConditionTest extends KernelTestBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  public static $modules = ['system', 'user', 'condition_test', 'node'];
+
+  /**
+   * Tests with both contexts mapped to the same user.
+   */
+  protected function testContextMissing() {
+    /** @var \Drupal\Core\Condition\ConditionPluginBase $condition */
+    $condition = \Drupal::service('plugin.manager.condition')
+      ->createInstance('condition_test_optional_context')
+      ->setContextMapping([
+        'node' => 'node',
+      ]);
+    \Drupal::service('context.handler')->applyContextMapping($condition, []);
+    $this->assertTrue($condition->execute());
+  }
+
+  /**
+   * Tests with both contexts mapped to the same user.
+   */
+  protected function testContextNoValue() {
+    /** @var \Drupal\Core\Condition\ConditionPluginBase $condition */
+    $condition = \Drupal::service('plugin.manager.condition')
+      ->createInstance('condition_test_optional_context')
+      ->setContextMapping([
+        'node' => 'node',
+      ]);
+    $definition = new ContextDefinition('entity:node');
+    $contexts['node'] = (new Context($definition));
+    \Drupal::service('context.handler')->applyContextMapping($condition, $contexts);
+    $this->assertTrue($condition->execute());
+  }
+
+  /**
+   * Tests with both contexts mapped to the same user.
+   */
+  protected function testContextAvailable() {
+    /** @var \Drupal\Core\Condition\ConditionPluginBase $condition */
+    $condition = \Drupal::service('plugin.manager.condition')
+      ->createInstance('condition_test_optional_context')
+      ->setContextMapping([
+        'node' => 'node',
+      ]);
+    $definition = new ContextDefinition('entity:node');
+    $node = Node::create(['type' => 'example']);
+    $contexts['node'] = (new Context($definition))->setContextValue($node);
+    \Drupal::service('context.handler')->applyContextMapping($condition, $contexts);
+    $this->assertFalse($condition->execute());
+  }
+
+}

core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php

@@ -233,6 +233,9 @@ public function testApplyContextMapping() {
     $context_hit->expects($this->atLeastOnce())
       ->method('getContextValue')
       ->will($this->returnValue(array('foo')));
+    $context_hit->expects($this->atLeastOnce())
+      ->method('hasContextValue')
+      ->willReturn(TRUE);
     $context_miss = $this->getMock('Drupal\Core\Plugin\Context\ContextInterface');
     $context_miss->expects($this->never())
       ->method('getContextValue');
@@ -242,13 +245,15 @@ public function testApplyContextMapping() {
       'miss' => $context_miss,
     );
 
+    $context_definition = $this->getMock('Drupal\Core\Plugin\Context\ContextDefinitionInterface');
+
     $plugin = $this->getMock('Drupal\Core\Plugin\ContextAwarePluginInterface');
     $plugin->expects($this->once())
       ->method('getContextMapping')
       ->willReturn([]);
     $plugin->expects($this->once())
       ->method('getContextDefinitions')
-      ->will($this->returnValue(array('hit' => 'hit')));
+      ->will($this->returnValue(array('hit' => $context_definition)));
     $plugin->expects($this->once())
       ->method('setContextValue')
       ->with('hit', array('foo'));
@@ -258,8 +263,11 @@ public function testApplyContextMapping() {
 
   /**
    * @covers ::applyContextMapping
+   *
+   * @expectedException \Drupal\Component\Plugin\Exception\ContextException
+   * @expectedExceptionMessage Required contexts without a value: hit.
    */
-  public function testApplyContextMappingConfigurable() {
+  public function testApplyContextMappingMissingRequired() {
     $context = $this->getMock('Drupal\Core\Plugin\Context\ContextInterface');
     $context->expects($this->never())
       ->method('getContextValue');
@@ -268,13 +276,118 @@ public function testApplyContextMappingConfigurable() {
       'name' => $context,
     );
 
+    $context_definition = $this->getMock('Drupal\Core\Plugin\Context\ContextDefinitionInterface');
+    $context_definition->expects($this->atLeastOnce())
+      ->method('isRequired')
+      ->willReturn(TRUE);
+
+    $plugin = $this->getMock('Drupal\Tests\Core\Plugin\TestConfigurableContextAwarePluginInterface');
+    $plugin->expects($this->once())
+      ->method('getContextMapping')
+      ->willReturn([]);
+    $plugin->expects($this->once())
+      ->method('getContextDefinitions')
+      ->will($this->returnValue(array('hit' => $context_definition)));
+    $plugin->expects($this->never())
+      ->method('setContextValue');
+
+    $this->contextHandler->applyContextMapping($plugin, $contexts);
+  }
+
+  /**
+   * @covers ::applyContextMapping
+   */
+  public function testApplyContextMappingMissingNotRequired() {
+    $context = $this->getMock('Drupal\Core\Plugin\Context\ContextInterface');
+    $context->expects($this->never())
+      ->method('getContextValue');
+
+    $contexts = array(
+      'name' => $context,
+    );
+
+    $context_definition = $this->getMock('Drupal\Core\Plugin\Context\ContextDefinitionInterface');
+    $context_definition->expects($this->atLeastOnce())
+      ->method('isRequired')
+      ->willReturn(FALSE);
+
+    $plugin = $this->getMock('Drupal\Tests\Core\Plugin\TestConfigurableContextAwarePluginInterface');
+    $plugin->expects($this->once())
+      ->method('getContextMapping')
+      ->willReturn(['optional' => 'missing']);
+    $plugin->expects($this->once())
+      ->method('getContextDefinitions')
+      ->will($this->returnValue(array('optional' => $context_definition)));
+    $plugin->expects($this->never())
+      ->method('setContextValue');
+
+    $this->contextHandler->applyContextMapping($plugin, $contexts);
+  }
+
+  /**
+   * @covers ::applyContextMapping
+   *
+   * @expectedException \Drupal\Component\Plugin\Exception\ContextException
+   * @expectedExceptionMessage Required contexts without a value: hit.
+   */
+  public function testApplyContextMappingNoValueRequired() {
+    $context = $this->getMock('Drupal\Core\Plugin\Context\ContextInterface');
+    $context->expects($this->never())
+      ->method('getContextValue');
+    $context->expects($this->atLeastOnce())
+      ->method('hasContextValue')
+      ->willReturn(FALSE);
+
+    $contexts = array(
+      'hit' => $context,
+    );
+
+    $context_definition = $this->getMock('Drupal\Core\Plugin\Context\ContextDefinitionInterface');
+    $context_definition->expects($this->atLeastOnce())
+      ->method('isRequired')
+      ->willReturn(TRUE);
+
     $plugin = $this->getMock('Drupal\Tests\Core\Plugin\TestConfigurableContextAwarePluginInterface');
     $plugin->expects($this->once())
       ->method('getContextMapping')
       ->willReturn([]);
     $plugin->expects($this->once())
       ->method('getContextDefinitions')
-      ->will($this->returnValue(array('hit' => 'hit')));
+      ->will($this->returnValue(array('hit' => $context_definition)));
+    $plugin->expects($this->never())
+      ->method('setContextValue');
+
+    $this->contextHandler->applyContextMapping($plugin, $contexts);
+  }
+
+
+  /**
+   * @covers ::applyContextMapping
+   */
+  public function testApplyContextMappingNoValueNonRequired() {
+    $context = $this->getMock('Drupal\Core\Plugin\Context\ContextInterface');
+    $context->expects($this->never())
+      ->method('getContextValue');
+    $context->expects($this->atLeastOnce())
+      ->method('hasContextValue')
+      ->willReturn(FALSE);
+
+    $contexts = array(
+      'hit' => $context,
+    );
+
+    $context_definition = $this->getMock('Drupal\Core\Plugin\Context\ContextDefinitionInterface');
+    $context_definition->expects($this->atLeastOnce())
+      ->method('isRequired')
+      ->willReturn(FALSE);
+
+    $plugin = $this->getMock('Drupal\Tests\Core\Plugin\TestConfigurableContextAwarePluginInterface');
+    $plugin->expects($this->once())
+      ->method('getContextMapping')
+      ->willReturn([]);
+    $plugin->expects($this->once())
+      ->method('getContextDefinitions')
+      ->will($this->returnValue(array('hit' => $context_definition)));
     $plugin->expects($this->never())
       ->method('setContextValue');
 
@@ -289,18 +402,23 @@ public function testApplyContextMappingConfigurableAssigned() {
     $context->expects($this->atLeastOnce())
       ->method('getContextValue')
       ->will($this->returnValue(array('foo')));
+    $context->expects($this->atLeastOnce())
+      ->method('hasContextValue')
+      ->willReturn(TRUE);
 
     $contexts = array(
       'name' => $context,
     );
 
+    $context_definition = $this->getMock('Drupal\Core\Plugin\Context\ContextDefinitionInterface');
+
     $plugin = $this->getMock('Drupal\Tests\Core\Plugin\TestConfigurableContextAwarePluginInterface');
     $plugin->expects($this->once())
       ->method('getContextMapping')
       ->willReturn([]);
     $plugin->expects($this->once())
       ->method('getContextDefinitions')
-      ->will($this->returnValue(array('hit' => 'hit')));
+      ->will($this->returnValue(array('hit' => $context_definition)));
     $plugin->expects($this->once())
       ->method('setContextValue')
       ->with('hit', array('foo'));
@@ -323,13 +441,15 @@ public function testApplyContextMappingConfigurableAssignedMiss() {
       'name' => $context,
     );
 
+    $context_definition = $this->getMock('Drupal\Core\Plugin\Context\ContextDefinitionInterface');
+
     $plugin = $this->getMock('Drupal\Tests\Core\Plugin\TestConfigurableContextAwarePluginInterface');
     $plugin->expects($this->once())
       ->method('getContextMapping')
       ->willReturn([]);
     $plugin->expects($this->once())
       ->method('getContextDefinitions')
-      ->will($this->returnValue(array('hit' => 'hit')));
+      ->will($this->returnValue(array('hit' => $context_definition)));
     $plugin->expects($this->never())
       ->method('setContextValue');