ContextHandler cannot validate constraints

This really needs tests to prove the bug.

EclipseGC explained this to me on IRC, and I sort of get it. But the issue summary here is really difficult for me to parse, and I think should be updated to describe the problem more clearly.

1. +++ b/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
   @@ -29,124 +31,92 @@ class ContextHandlerTest extends UnitTestCase {
   -    $data[] = array(array(), array(), TRUE);
   -    $data[] = array(array(), array($requirement_any), FALSE);
   -    $data[] = array(array(), array($requirement_optional), TRUE);
   -    $data[] = array(array(), array($requirement_any, $requirement_optional), FALSE);
   -    $data[] = array(array($context_any), array($requirement_any), TRUE);
   -    $data[] = array(array($context_constraint_mismatch), array($requirement_specific), FALSE);
   -    $data[] = array(array($context_datatype_mismatch), array($requirement_specific), FALSE);
   -    $data[] = array(array($context_specific), array($requirement_specific), TRUE);
   ...
   +    $this->assertTrue($this->contextHandler->checkRequirements([], []));
   +    $this->assertFalse($this->contextHandler->checkRequirements([], [$requirement_any]));
   +    $this->assertTrue($this->contextHandler->checkRequirements([], [$requirement_optional]));
   +    $this->assertFalse($this->contextHandler->checkRequirements([], [$requirement_any, $requirement_optional]));
   +    $this->assertTrue($this->contextHandler->checkRequirements([$context_any], [$requirement_any]));
   +    $this->assertFalse($this->contextHandler->checkRequirements([$context_constraint_mismatch], [$requirement_specific]));
   +    $this->assertFalse($this->contextHandler->checkRequirements([$context_datatype_mismatch], [$requirement_specific]));
   +    $this->assertTrue($this->contextHandler->checkRequirements([$context_specific], [$requirement_specific]));
   +    $this->assertTrue($this->contextHandler->checkRequirements([$context_specific], [$requirement_complex]));
   +    $this->assertTrue($this->contextHandler->checkRequirements([$context_constraint_mismatch], [$requirement_complex]));
   

   If this were a new test, I would say "this should use a @dataProvider". But you're removing one instead! Why?

2. +++ b/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
   @@ -29,124 +31,92 @@ class ContextHandlerTest extends UnitTestCase {
   -    $data = array();
   -    // No context will return no valid contexts.
   -    $data[] = array(array(), $requirement_any);
   -    // A context with a generic matching requirement is valid.
   -    $data[] = array(array($context_any), $requirement_any);
   -    // A context with a specific matching requirement is valid.
   -    $data[] = array(array($context_specific), $requirement_specific);
   -
   -    // A context with a mismatched constraint is invalid.
   -    $data[] = array(array($context_constraint_mismatch), $requirement_specific, array());
   -    // A context with a mismatched datatype is invalid.
   -    $data[] = array(array($context_datatype_mismatch), $requirement_specific, array());
   ...
   +    $this->assertEquals([], $this->contextHandler->getMatchingContexts([], $requirement_any));
   +    $this->assertEquals([$context_any], $this->contextHandler->getMatchingContexts([$context_any], $requirement_any));
   +    $this->assertEquals([$context_specific], $this->contextHandler->getMatchingContexts([$context_specific], $requirement_specific));
   +    $this->assertEquals([], $this->contextHandler->getMatchingContexts([$context_constraint_mismatch], $requirement_specific));
   +    $this->assertEquals([], $this->contextHandler->getMatchingContexts([$context_datatype_mismatch], $requirement_specific));
   

   Same here!

1. +++ b/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
   @@ -56,40 +71,33 @@ public function providerTestCheckRequirements() {
   -    $data[] = array(array(), array(), TRUE);
   ...
   +    $data[] = [[], [], TRUE];
   
   @@ -111,41 +119,28 @@ public function testGetMatchingContexts($contexts, $requirement, $expected = NUL
   -    $data[] = array(array(), $requirement_any);
   ...
   +    $data[] = [[], $requirement_any];
   

   Sure, we all love the new array syntax. But now I can't tell what you're changing here. Or in this entire test.

2. +++ b/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
   @@ -174,56 +157,52 @@ public function testFilterPluginDefinitionsByContexts($has_context, $definitions
        // No context and no plugins, no plugins available.
   -    $data[] = array(FALSE, $plugins, array());
   -
   -    $plugins = array('expected_plugin' => array());
   -    // No context, all plugins available.
   -    $data[] = array(FALSE, $plugins, $plugins);
   -
   -    $plugins = array('expected_plugin' => array('context' => array()));
   -    // No context, all plugins available.
   -    $data[] = array(FALSE, $plugins, $plugins);
   -
   -    $plugins = array('expected_plugin' => array('context' => array('context1' => new ContextDefinition('expected_data_type'))));
   -    // Missing context, no plugins available.
   -    $data[] = array(FALSE, $plugins, array());
   -    // Satisfied context, all plugins available.
   -    $data[] = array(TRUE, $plugins, $plugins);
   -
   -    $mismatched_context_definition = (new ContextDefinition('expected_data_type'))->setConstraints(array('mismatched_constraint_name' => 'mismatched_constraint_value'));
   -    $plugins = array('expected_plugin' => array('context' => array('context1' => $mismatched_context_definition)));
   -    // Mismatched constraints, no plugins available.
   -    $data[] = array(TRUE, $plugins, array());
   -
   -    $optional_mismatched_context_definition = clone $mismatched_context_definition;
   -    $optional_mismatched_context_definition->setRequired(FALSE);
   -    $plugins = array('expected_plugin' => array('context' => array('context1' => $optional_mismatched_context_definition)));
   -    // Optional mismatched constraint, all plugins available.
   -    $data[] = array(FALSE, $plugins, $plugins);
   -
   -    $expected_context_definition = (new ContextDefinition('expected_data_type'))->setConstraints(array('expected_constraint_name' => 'expected_constraint_value'));
   -    $plugins = array('expected_plugin' => array('context' => array('context1' => $expected_context_definition)));
   -    // Satisfied context with constraint, all plugins available.
   -    $data[] = array(TRUE, $plugins, $plugins);
   -
   -    $optional_expected_context_definition = clone $expected_context_definition;
   -    $optional_expected_context_definition->setRequired(FALSE);
   -    $plugins = array('expected_plugin' => array('context' => array('context1' => $optional_expected_context_definition)));
   -    // Optional unsatisfied context, all plugins available.
   -    $data[] = array(FALSE, $plugins, $plugins);
   -
   -    $unexpected_context_definition = (new ContextDefinition('unexpected_data_type'))->setConstraints(array('mismatched_constraint_name' => 'mismatched_constraint_value'));
   -    $plugins = array(
   -      'unexpected_plugin' => array('context' => array('context1' => $unexpected_context_definition)),
   -      'expected_plugin' => array('context' => array('context2' => new ContextDefinition('expected_data_type'))),
   -    );
   -    // Context only satisfies one plugin.
   -    $data[] = array(TRUE, $plugins, array('expected_plugin' => $plugins['expected_plugin']));
   

   All of this can't be 100% wrong...

This is all done via the constraint system, however the ContextHandler essentially expects the contexts we have and the required contexts on a plugin to be equal.

Yeah, this needs to do proper matching. We are working on having a type-inheritance systems for that in Rules.

Constraints expect to evaluate against the corresponding data object. In the case of bundles (and a few other constraint validators) that would be an entity. The context system can be functional before we have a given entity (say during administration), so constraints need to be validated against contextual metadata.

While the premise is correct, I don't think the conclusion is. Constraints are for validating data, yes. When you need to check whether some context definition matches with some data definition that's something different - I don't think it makes sense to use the constraints which already have the logic for checking the data, for checking the metadata also.

I think the case of bundles can be covered perfectly fine with a data type based matching system, e.g. entity:node:page matches entity:node. Regarding, the more complex case you suggested, having a context plugin that requires a node with a certain field X: I'm not convinced we need to have support for that, e.g. Rules never was able to express that, still the matching was powerful enough in D7.
However, one thing that we had in d7 which we are not use it's required to bring back in D8 is support for having a contextual plugin with a context that must be of a list of allowed types, e.g. entity:node:page OR entity:node:article. Instead of that, constraints might be a good way to express that as you can specify that you require a node with the constraints of having either an article or page bundle. However, the specified constraints are metadata, so I could see us making use of them for matching metadata in combination with metadata assertions. Metadata assertions, are what Rules makes for pre-computing the available context: e.g. when you have a node type condition checking for a page, it can provide the assertion that after its execution the passed context is not only of type entity:node but also has the constraint "Bundle is page" fulfilled. Given we have that in place (we are working on assertions still) we could leverage that for context-fullfillment-checking by matching the asserted constraints with the needed constraints.

Thus, we'd not execute the constraints at configuration time, we'd only match the arrays of known, asserted constraints with the array of required constraints. Would that meet your use-case also?

@core-context-handler:
We actually do not use that, it has not much logic we could re-use, so it was simpler to have our own code for mapping context for now. FYI: It's no service, but a (re-usable) trait: https://github.com/fago/rules/blob/8.x-3.x/src/Context/ContextHandlerTra... Would be good if we could merge those approaches in the long run though.

Besides, I've troubles finding issues related to the Context System in core. Should we start using some tag at least to make those issue findable? Having a component would probably be better, but that doesn't match the code-structure. (Would have been nice there as well.)

At that point there hasn't been too much discussion, basically fago made a comment and disagreed with the idea.
In general it seems to be that we are extending the behaviour of the context handler, so I'm wondering whether a class ContextHandlerWithValidationConstraints would be a usable alternative.

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
   @@ -19,7 +20,8 @@ class EntityChangedConstraintValidator extends ConstraintValidator {
          /** @var \Drupal\Core\Entity\EntityInterface $entity */
   

   This comment can be dropped now.

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityTypeConstraintValidator.php
   @@ -23,6 +24,15 @@ public function validate($entity, Constraint $constraint) {
   +      if ($entity->getDataDefinition()->getConstraint('EntityType') != $constraint->type) {
   

   This doesn't make sense for me. \Drupal\Core\TypedData\DataDefinitionInterface::getConstraint documents to return a array (the constraint configuration) and $constraint->type is a string: \Drupal\Core\Entity\Plugin\Validation\Constraint\EntityTypeConstraint::$type

3. +++ b/core/lib/Drupal/Core/Plugin/Plugin/DataType/DataTypeWrapper.php
   @@ -0,0 +1,31 @@
   + * Contains \Drupal\ctools\Plugin\DataType\DataTypeWrapper.
   

   points to ctools still

4. +++ b/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
   @@ -56,40 +71,33 @@ public function providerTestCheckRequirements() {
   +    $context_any = new Context(new ContextDefinition('any'));
   

   +1 to not mock in a provider

5. +++ b/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
   @@ -56,40 +71,33 @@ public function providerTestCheckRequirements() {
   +    $data = [];
   +    $data[] = [[], [], TRUE];
   +    $data[] = [[], [$requirement_any], FALSE];
   +    $data[] = [[], [$requirement_optional], TRUE];
   +    $data[] = [[], [$requirement_any, $requirement_optional], FALSE];
   +    $data[] = [[$context_any], [$requirement_any], TRUE];
   +    $data[] = [[$context_constraint_mismatch], [$requirement_specific], FALSE];
   +    $data[] = [[$context_datatype_mismatch], [$requirement_specific], FALSE];
   +    $data[] = [[$context_specific], [$requirement_specific], TRUE];
   +    $data[] = [[$context_specific], [$requirement_complex], TRUE];
   +    $data[] = [[$context_constraint_mismatch], [$requirement_complex], TRUE];
   

   I agree with tim, let's not convert everything. Why? It moves focus of reviews from fundamental reviews to codestyle only.

+++ b/core/lib/Drupal/Core/Plugin/Plugin/DataType/DataTypeWrapper.php
@@ -0,0 +1,31 @@
+ * other when a raw data object is not available to validated against.

validated=>validate (be validated)

I really think this is a big step in the right direction, and apparently won't adversely affect Rules

Right, still we should try to do not fork the way context is handled needlessly and use the same way of matching context as far as possible. Such that the context system works the same way for users/devs in both cases + we have the chance to unify code in future.

Oh, also, on the topic of the entity:node:page level validation. I don't really see how this is possible. I can't attach multiple requirements to a plugin. The plugin requires 'entity:node' of bundles page or article, not entity:node:page || entity:node:article with some special handling.

Right. That's what I'm saying that we do not support right now and I'm fine with, in particular if we can get to "the plugin requires 'entity:node' of bundles page or article".

I don't think updating a handful of constraint validators to respond appropriately to metadata is onerous.

If the approach taken does not work with a general symfony validation constraint, then it looks like mis-use of the validation API to me. It does not make much sense to use symfony validation constraints for contexts IF you have to modify them in some special way to make it work imo.

Afaics, my suggestions would achieve the same without the need for extending symfony validation constraints. Let's discuss the idea to see whether it can meet your requirements also.

+    // Validate against wrapper bundle constraint when necessary.
+    if ($entity instanceof DataTypeWrapper) {
+      $bundles = $entity->getDataDefinition()->getConstraint('Bundle');

Seeing that in the validator is quite a no-go imo, as we did quite some work to make coding validators so easy that you know that there is NO wrapped object going to be passed to you. We should not go back to a situation where sometimes wrapped objects are passed and sometimes not.

Patch no longer applies.

Re-rolled.

Addressed test failures.
Corrected coding standards errors.

Unable to replicate the test failures on my local build.

I ain't qualified to RTBC this, but honestly, it makes a lot of sense to me. I don't really see anything wrong this patch.

1. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
   @@ -118,4 +165,64 @@ public function applyContextMapping(ContextAwarePluginInterface $plugin, $contex
   +    if (strpos($definition->getDataType(), 'entity:') === 0 && !in_array('EntityType', array_keys($definition->getConstraints()))) {
   

   Micro-optimization: rather than call array_keys() here, we could do:
   !array_key_exists('EntityType, $definition->getConstraints())

2. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
   @@ -118,4 +165,64 @@ public function applyContextMapping(ContextAwarePluginInterface $plugin, $contex
   +    foreach ($definition->getConstraints() as $constraint_name => $constraint_settings) {
   

   Another micro-optimization: can we call $definition->getConstraints() once and reuse the return value?

3. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
   @@ -118,4 +165,64 @@ public function applyContextMapping(ContextAwarePluginInterface $plugin, $contex
   +    return $this->typedDataManager->createInstance($context->getContextDefinition()->getDataType());
   

   Once again, let's call $context->getContextDefinition() once and use that value.

Applied @phenaproxima's micro-optimisations.

Points 1-3 all need test coverage, we had 100% before and I think its worth the extra effort here.

1. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
   @@ -118,4 +165,66 @@ public function applyContextMapping(ContextAwarePluginInterface $plugin, $contex
   +    if ($context->hasContextValue()) {
   +      return $context->getContextData();
   +    }
   

   This

2. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
   @@ -118,4 +165,66 @@ public function applyContextMapping(ContextAwarePluginInterface $plugin, $contex
   +    if (!empty($constraints['EntityType']) && $constraints['EntityType'] instanceof EntityTypeConstraint) {
   ...
   +    // No entity related constraints, so generate a basic typed data object.
   +    return $this->typedDataManager->createInstance($context_definition->getDataType());
   

   The `else` case of this, aka the final return statement

3. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
   @@ -118,4 +165,66 @@ public function applyContextMapping(ContextAwarePluginInterface $plugin, $contex
   +      if ($storage instanceof ContentEntityStorageInterface) {
   

   The `else` case of this

4. +++ b/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
   @@ -1,26 +1,23 @@
   +class ContextHandlerTest extends KernelTestBase {
   +  use ContentTypeCreationTrait;
   

   Nit: empty line between these, please

5. +++ b/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
   @@ -30,202 +27,178 @@ class ContextHandlerTest extends UnitTestCase {
   +  public static $modules = ['user', 'system', 'node', 'field', 'text', 'filter'];
   ...
   +    $this->installEntitySchema('user');
   +    $this->installEntitySchema('node_type');
   +    $this->installEntitySchema('node');
   +    $this->installConfig('node');
   

   This could all be done for you if the class extended \Drupal\KernelTests\Core\Entity\EntityKernelTestBase

   I think you'd just need to keep `node` in the $modules line

6. +++ b/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
   @@ -30,202 +27,178 @@ class ContextHandlerTest extends UnitTestCase {
   +    $this->assertSame(FALSE, $this->contextHandler->checkRequirements([], [$requirement_any]));
   

   Nit: assertFalse

7. +++ b/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
   @@ -30,202 +27,178 @@ class ContextHandlerTest extends UnitTestCase {
   +    $this->assertSame(TRUE, $this->contextHandler->checkRequirements([], [$requirement_optional]));
   

   Okay not gonna highlight all these

8. +++ b/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
   @@ -30,202 +27,178 @@ class ContextHandlerTest extends UnitTestCase {
   +    $this->assertSame(FALSE, $this->contextHandler->checkRequirements([], [$requirement_any, $requirement_optional]));
   

   Wait why can't this go back to being a dataProvider? Those work for KTB too

9. +++ b/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
   @@ -30,202 +27,178 @@ class ContextHandlerTest extends UnitTestCase {
       * @covers ::getMatchingContexts
   ...
   +  public function testGetMatchingContexts() {
   

   The @covers need to be updated to include the new protected methods

10. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
    @@ -51,11 +96,13 @@ public function getMatchingContexts(array $contexts, ContextDefinitionInterface
    +        $violations = $validator->validate($typed_data_definition, array_values($this->getConstraintsFromContextDefinition($definition)));
    
    @@ -118,4 +165,66 @@ public function applyContextMapping(ContextAwarePluginInterface $plugin, $contex
    +    $constraints = $this->getConstraintsFromContextDefinition($context_definition);
    

    I notice we call getConstraintsFromContextDefinition() twice in the same code path. Why not pass the result of the first call into getTypedDataFromContext() to avoid the need for the second one?

The entity-specfic-ness of this code worries me.
Why is ContextHandler left holding the bag on so much of this?
Why are getConstraintsFromContextDefinition() and getTypedDataFromContext() part of this service?
Is there a need for other handling similar to this, for non-entity stuff?

Tried to rewrite the IS

I think ideally, all of this would be a new service, and the changes to ContextHandler would consist entirely of this:

    */
   public function getMatchingContexts(array $contexts, ContextDefinitionInterface $definition) {
     return array_filter($contexts, function (ContextInterface $context) use ($definition) {
-      $context_definition = $context->getContextDefinition();
-
-      // If the data types do not match, this context is invalid unless the
-      // expected data type is any, which means all data types are supported.
-      if ($definition->getDataType() != 'any' && $definition->getDataType() != $context_definition->getDataType()) {
-        return FALSE;
-      }
-
-      // If any constraint does not match, this context is invalid.
-      foreach ($definition->getConstraints() as $constraint_name => $constraint) {
-        if ($context_definition->getConstraint($constraint_name) != $constraint) {
-          return FALSE;
-        }
-      }
-
-      // All contexts with matching data type and contexts are valid.
-      return TRUE;
+      return $this->ourNewService->areConstraintsSatisfied($definition->getConstraints(), $context->getContextDefinition()->getConstraints());
     });
   }
 

I really like moving this responsibility out of ContextHandler.
I've gone a step further, and moved it to ContextDefinition itself.
Opened #2671964: ContextHandler cannot validate constraints for moving the entity logic out of this class.
Restored a lot of the original tests, but added a new one for the new parts.

Once again, the interdiff is bigger than the patch, so it is not included.

Discussed the failing test with @EclipseGc and came up with this solution.

Haven't figured out the block fail yet, but here's the yield.

Somewhere I missed adding the test coverage to prove that the yield is actually a benefit.
While doing that I was able to confirm that the change in #61 is not needed. Undoing that.

Additionally, I found the root issue causing the password fails.
\Drupal\Core\TypedData\Plugin\DataType\Map::getValue() loops over each property, but only currently works for those properties that happened to be instantiated already.
Switching to iterate over the full list of properties causes the comparison in \Drupal\user\Plugin\Validation\Constraint\ProtectedUserFieldConstraintValidator::validate() to correctly pass.

That exposed a lot of other bugs, so opened #2934192: FieldItemBase::getValue() returns partial or full values depending on state for that. Restoring the workaround with an @todo pointing there.

Looks good to me 👍

+++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php
@@ -252,4 +257,105 @@ public function getDataDefinition() {
+  public function isSatisfiedBy(ContextInterface $context) {

+++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinitionInterface.php
@@ -20,4 +20,16 @@
+  public function isSatisfiedBy(ContextInterface $context);

This is a BC break.

I discussed this with @catch and we were of the opinion that in order to maintain BC we might need to add a new interface for this method, and have ContextDefinition implement it.

What would the implications of that be?

What are the implications on contrib if we don't?

We have the 'contributed blocker' tag but there isn't any detail on what it blocks.

Thanks

However, we reserve the ability to add methods to these interfaces in minor releases to support new features. When implementing the interface, module authors are encouraged to either:
Where there is a 1-1 relationship between a class and an interface, inherit from the class. Where a base class or trait is provided, use those. This way your class should inherit new methods from the class or trait when they're added.

There is a 1:1 relationship here.

I spent a good amount of time on the IS, it explains why this issue is essential.

Without this fix, context definitions are pretty useless. Which is why no one has implemented the interface besides this single class.

It blocks any contrib module from being able to use the entire subsystem.

1. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php
   @@ -252,4 +257,105 @@ public function getDataDefinition() {
   +      $violations = $validator->validate($value, array_values($this->getConstraintObjects()));
   +      // If a value has no violations then the requirement is satisfied.
   ...
   +        return TRUE;
   

   what if there is more than one value?

   if the first one is valid, we return true without checking the later ones.

2. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php
   @@ -252,4 +257,105 @@ public function getDataDefinition() {
   +    // @todo Move the entity specific logic out of this class in
   +    //   https://www.drupal.org/node/2671964.
   ...
   +    // @todo Move the entity specific logic out of this class in
   +    //   https://www.drupal.org/node/2671964.
   

   that is this issue?

3. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php
   @@ -252,4 +257,105 @@ public function getDataDefinition() {
   +    if (!empty($constraints['EntityType']) && $constraints['EntityType'] instanceof EntityTypeConstraint) {
   +      $entity_type_manager = \Drupal::entityTypeManager();
   +      $entity_type_id = $constraints['EntityType']->type;
   +      $storage = $entity_type_manager->getStorage($entity_type_id);
   +      // If the storage can generate a sample entity we might delegate to that.
   +      if ($storage instanceof ContentEntityStorageInterface) {
   +        if (!empty($constraints['Bundle']) && $constraints['Bundle'] instanceof BundleConstraint) {
   +          foreach ($constraints['Bundle']->bundle as $bundle) {
   +            // We have a bundle, we are bundleable and we can generate a sample.
   +            yield EntityAdapter::createFromEntity($storage->createWithSampleValues($bundle));
   +          }
   +          return;
   +        }
   

   this feels back to front.

   we're embedding specific knowledge of an 'EntityType' constraint in ContextDefinition.

   Feels like there is a missing concept between the constraint and the definition.

4. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php
   @@ -252,4 +257,105 @@ public function getDataDefinition() {
   +    if (strpos($this->getDataType(), 'entity:') === 0) {
   +      $entity_type_id = substr($this->getDataType(), 7);
   +      $constraint_definitions['EntityType'] = ['type' => $entity_type_id];
   

   Shouldn't the context definition constructor add this for us, then we don't need to embed this logic

   Or does this indicate there is a missing EntityContextDefinition that sub-classes ContextDefinition and auto-adds this constraint definition.

   I note that \Drupal\layout_builder\Plugin\Derivative\FieldBlockDeriver::getDerivativeDefinitions adds the Bundle constraint - but not the EntityType? Should it?

   If someone already added an alternate EntityType constraint - would this clobber it?

   tl;dr if we expect a context with naming entity:{type} will have an EntityType constraint, shouldn't it happen before here?

There was no discussion here of why #51 was abandoned

#74

1) One is enough

2) That should have linked to #2932462: Add EntityContextDefinition for the 80% use case

3) That's what that ^ was about

4) Also covered by the above.

#75

If you'll note, the diff of ContextHandler is almost identical to #51. But instead of delegating to a service, it's delegating directly to the ContextDefinition object in question.

Not playing status wars anymore.

If you'll note, the diff of ContextHandler is almost identical to #51. But instead of delegating to a service, it's delegating directly to the ContextDefinition object in question.

Right, but keeping it somewhere else outside the context definition would remove the API change and that whole debate becomes moot.

However, in light of the right @todo link, the current approach here (the API addition) gives a clear path forward for #2932462: Add EntityContextDefinition for the 80% use case with a clean API design so that's reason not to make it a separate service.

I think that as per #72, there is a 1:1 relationship.

Given the full scope of the interface:

I just don't see anyone implementing it without sub-classing the ContextDefinition class.

Putting back to RTBC to get opinions from other committers.

We still need the @todo links fixed, but that can remain RTBC

Fixing NID

I'd forgotten about the explicit 1-1 rule when discussing this last night. Given that, I think this should be OK to go into 8.5.x too.

Reflected that in the IS

I know this is an underused API, but can we get a change record for the new functionality, as its a vast improvement over the current logic.

Thanks

1. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php
   @@ -252,4 +257,105 @@ public function getDataDefinition() {
   +    // If the data types do not match, this context is invalid unless the
   +    // expected data type is any, which means all data types are supported.
   +    if ($this->getDataType() != 'any' && $definition->getDataType() != $this->getDataType()) {
   +      return FALSE;
   +    }
   

   Related question, I was wondering about hierarchy in regards to context data types before.. lets say I have something requires a type "entity" and I have a node, aka "entity:node". Clearly that's not supported yet, is there any issue/discussion on supporting that?

   This isn't making it any worse than it is now, so obviously not a blocker.

2. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php
   @@ -252,4 +257,105 @@ public function getDataDefinition() {
   +    // If the data type is an entity, manually add one to the constraints array.
   +    if (strpos($this->getDataType(), 'entity:') === 0) {
   +      $entity_type_id = substr($this->getDataType(), 7);
   

   similar here, if we look explicitly for entity:, what if I have a context that accepts any entity...

3. +++ b/core/tests/Drupal/Tests/Core/Plugin/Context/ContextDefinitionIsSatisfiedTest.php
   @@ -0,0 +1,340 @@
   +    $this->entityTypeBundleInfo->getBundleInfo('test_config')->willReturn([]);
   

   not sure if/how this is relevant for the test, but it's worth pointing out that getBundleInfo() always returns a bundle, if there is none, then there is a default/fallback bundle where bundle === entity_type_id, see \Drupal\Core\Entity\EntityTypeBundleInfo::getAllBundleInfo(). Might result in a more realistic test to simulate that behavior?

For #87.3

Let's fix that

Doesn't affect the test, but might as well do it.

Crediting @fago, @dawehner, @phenaproxima and @Berdir as their reviews shaped the patch

Discussed the earlier concerns of @fago and @dawehner with @EclipseGc.

For posterity, the patch changed tack in #46 to address those concerns and the final solution doesn't special case and validation constraints or validators.

Instead, it relies on the ::generateSampleValues functionality since added to core, so that validators don't have to do anything different.

This is a better solution as we don't embed any knowledge of the context api in the validation api.

Committed as 0c1de8e and pushed to 8.5.x.

Published change record, unpostponed issues blocked on this.

Wow! This has huge implications (the good kind :-)) for Panelizer in Drupal 8 :-)

I think the commit in #93 above has caused new test failuers in Rules #2936553: Rules kernel tests fail with "Creating default object from empty value", judging by the timing of the commit and the backtrace. Not saying that the fault does not originate within Rules code, but this core change definitely is involved in it.

In fact, from the description in the issue it doesn't even sound like a bug in Rules, but a failed assumption in the Rules test setup

Thanks EclipseGc and tim.plunkett.

I was asking on this issue because the new line $current_user->_skipProtectedUserFieldConstraint = TRUE; definitely causes the "Creating default object from empty value" error. Is there any way that:

  public function getRuntimeContexts(array $unqualified_context_ids) {
    $current_user = $this->userStorage->load($this->account->id());

should ever validly return NULL i.e. not a user object? Adding the temporary line

    if (empty($current_user)) { $current_user = new \stdClass; }

fixes the problem and the Rules tests run OK. But obviouly that's not the solution. I don't know enough about the contexts to work out where the real source of the problem is, so just thought I would ask here, as this is where that new line came from.

Jonathan

I guess this could be broken for anonymous users.

diff --git a/core/modules/user/src/ContextProvider/CurrentUserContext.php b/core/modules/user/src/ContextProvider/CurrentUserContext.php
index 73be2caa09..aacc5f774a 100644
--- a/core/modules/user/src/ContextProvider/CurrentUserContext.php
+++ b/core/modules/user/src/ContextProvider/CurrentUserContext.php
@@ -50,9 +50,11 @@ public function __construct(AccountInterface $account, EntityManagerInterface $e
   public function getRuntimeContexts(array $unqualified_context_ids) {
     $current_user = $this->userStorage->load($this->account->id());
 
-    // @todo Do not validate protected fields to avoid bug in TypedData, remove
-    //   this in https://www.drupal.org/project/drupal/issues/2934192.
-    $current_user->_skipProtectedUserFieldConstraint = TRUE;
+    if ($current_user) {
+      // @todo Do not validate protected fields to avoid bug in TypedData,
+      //   remove this in https://www.drupal.org/project/drupal/issues/2934192.
+      $current_user->_skipProtectedUserFieldConstraint = TRUE;
+    }
 
     $context = new Context(new ContextDefinition('entity:user', $this->t('Current user')), $current_user);
     $cacheability = new CacheableMetadata();

Can we get a major follow up filed to add add the diff at #100?

Done #2936642: Getting runtime contexts will generate an E_WARNING for anonymous users

thanks

Apparently, this change breaks certain uses of COnfig Entities, since they do not require the getIterator() method.

See:

#2941319: Error: Call to undefined method Drupal\domain\Entity\Domain::getIterator()
#2759267: Undefined method YamlForm::getIterator()

I have a temporary fix here: #2941457: EntityAdapter::getIterator requires undocumented methods

Without the fix, Config Entities will throw fatal errors in unpredictable areas, such as Context negotiation. The only other fix is to break BC and force Config Entities to add the method.

The bug in EntityAdapter was introduced in 2014 via #2002138: Use an adapter for supporting typed data on ContentEntities, but only exposed to wider runtime code by this issue.
I think that the "temporary fix" issue is actually a long-term fix, even after #1818574: Support config entities in typed data EntityAdapter happens.
It is now RTBC.

Did this issue cause the linked regression?

Another regression: #2958785: ContextDefinition isSatisfiedBy() check fails for context using inherited class.

There was a major performance regression with this change that is visible in layout builder. Issue created:#2981889: Performance Degradation in Layout Builder and other places likely