\Drupal\Core\Plugin\Context\Context should use a ContextDefinition class instead of arrays

The code developed so far lives in https://github.com/fago/rules/tree/8.x-3.x/src/Context - so we can get started with move it to Core instead and improve it as necessary.

Will work on this as part of the layout sprint: https://groups.drupal.org/node/423448

Would need some help here.

For example the LanguageConditionTest, here is the code that fails:

$condition = $this->manager->createInstance('language')
      ->setConfig('langcodes', array('en' => 'en', 'it' => 'it'))
      ->setContextValue('language', $language);

What happens when calling setContextValue() is that it tries to set the $language object into the context found under the 'language' key. But that blows up because there isn't any definition for 'language' key in ContextAwarePluginBase::contextDefinitions. From reading the code I understand that the static contextDefinitions() should provide these definitions, but I do not see it implemented anywhere.

The patch also adds ContextInterface into Core/Plugin/Context dir, and this interface does not seem to be used.

Just a reroll as ContextAwarePluginBase ended up with conflicts after merge with latest HEAD.

wrong patch... retry

With big support from @fubhy here comes the patch. It still does not do the merge of context definitions with other plugin configuration, that is to come after the review of the current state.

1. +++ b/core/lib/Drupal/Component/Plugin/Context/ContextInterface.php
   @@ -33,13 +33,11 @@ public function getContextValue();
   +  public function setContextDefinition($contextDefinition);
   

   That needs a type hint to the ContextDefinitionInterface

2. +++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
   @@ -8,84 +8,107 @@
   +    //   more.
   

   White space issue. Anymore is one word :)

3. +++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
   @@ -8,84 +8,107 @@
   +      return $this->setContextData(\Drupal::typedDataManager()->create($this->contextDefinition->getDataDefinition(), $value));
   

   That should use $this->getTypedDataManager().

4. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php
   @@ -0,0 +1,239 @@
   +    // @todo Setters are missing from the core data definition interfaces.
   

   Thats not true anymore :)

5. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinitionInterface.php
   @@ -0,0 +1,191 @@
   +   * @param \Drupal\Core\TypedData\TypedDataManager $typed_data_manager
   ...
   +   * @return \Drupal\Core\TypedData\TypedDataManager
   

   Documentation missing for the @param and @return value.

6. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinitionInterface.php
   @@ -0,0 +1,191 @@
   +  public function setTypedDataManager(TypedDataManager $typed_data_manager);
   

   Should return $this.

7. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinitionInterface.php
   @@ -0,0 +1,191 @@
   +   * See \Drupal\Core\TypedData\TypedDataManager::getConstraints() for details.
   ...
   +   * See \Drupal\Core\TypedData\TypedDataManager::getConstraints() for details.
   ...
   +   * See \Drupal\Core\TypedData\TypedDataManager::getConstraints() for details.
   

   Shouldn't that be @see?

8. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinitionInterface.php
   @@ -0,0 +1,191 @@
   +   * @return \Drupal\Core\TypedData\DataDefinitionInterface
   

   Misses documentation

9. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextInterface.php
   @@ -0,0 +1,35 @@
   +use \Drupal\Component\Plugin\Context\ContextInterface as ComponentContextInterface;
   

   That leading \ is redundant.

10. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextInterface.php
    @@ -0,0 +1,35 @@
    +   * @return \Drupal\Core\TypedData\TypedDataInterface
    

    Misses documentation.

11. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php
    @@ -7,21 +7,16 @@
    +abstract class ContextAwarePluginBase extends PluginBase implements ContextAwarePluginInterface {
    
    @@ -34,35 +29,111 @@
    +      $this->contextDefinitions = static::contextDefinitions(\Drupal::service('typed_data_manager'));
    

    We need to inject the typed data manager there. So that means we should extend the ContainerPluginFactoryInterface here.

12. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginInterface.php
    @@ -0,0 +1,37 @@
    + * Interface for context-aware plugins.
    

    Without the dash! :) (I think?!) :)

13. +++ b/core/modules/system/src/Tests/Plugin/ContextPluginTest.php
    @@ -36,51 +37,26 @@ function testContext() {
    +    $this->assertEqual($plugin->getContextDefinitions(), array('user' => ContextDefinition::create('entity:user')->setLabel(t('User'))));
    ...
    +    $this->assertEqual($plugin->getContextDefinition('user'), ContextDefinition::create('entity:user')->setLabel(t('User')));
    
    +++ b/core/modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/mock_block/MockComplexContextBlock.php
    @@ -16,6 +17,15 @@
    +    $definitions['user'] = ContextDefinition::create('entity:user')->setLabel(t('User'));
    +    $definitions['node'] = ContextDefinition::create('entity:node')->setLabel(t('Node'));
    

    I would prefer testing the separate methods like ->getLabel(), etc.

14. +++ b/core/modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/mock_block/MockComplexContextBlock.php
    @@ -16,6 +17,15 @@
    +    $definitions['user'] = ContextDefinition::create('entity:user')->setLabel(t('User'));
    +    $definitions['node'] = ContextDefinition::create('entity:node')->setLabel(t('Node'));
    
    +++ b/core/modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/mock_block/MockUserNameBlock.php
    @@ -16,6 +17,14 @@
    +    $context['user'] = ContextDefinition::create('entity:user')->setLabel(t('User'));
    

    Can you put these into their own code blocks? with Line breaks? :)

15. +++ b/core/modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/mock_block/MockUserNameBlock.php
    @@ -16,6 +17,14 @@
    +    return $context;
    

    Can you name it $definitionss please?

- Implemented comments from #13
- Provided the TypedDataTrait as well as TypedDataAwareInterface as discussed in person with @fubhy. However I am not that sure I got right the interface as now to have something TypedDataAware the interface needs to be implemented as well as the trait used in the class.

1. +++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
   @@ -33,7 +33,7 @@ class Context implements ContextInterface {
       * Sets the contextDefinition for us without needing to call the setter.
   

   Lets fix the documentation on the constructor too.

2. +++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
   @@ -33,7 +33,7 @@ class Context implements ContextInterface {
   +  public function __construct($context_definition) {
   

   Missing typehint.

3. +++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
   @@ -54,7 +54,7 @@ public function getContextValue() {
   +  public function setContextDefinition($context_definition) {
   

   Missing typehint.

4. +++ b/core/lib/Drupal/Component/Plugin/Context/ContextInterface.php
   @@ -33,13 +33,11 @@ public function getContextValue();
   +   * @param \Drupal\Core\Plugin\Context\ContextDefinitionInterface$context_definition
   

   Missing whitespace before $.

5. +++ b/core/lib/Drupal/Component/Plugin/Context/ContextInterface.php
   @@ -33,13 +33,11 @@ public function getContextValue();
   +  public function setContextDefinition($context_definition);
   

   Missing typehint.

6. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinitionInterface.php
   @@ -0,0 +1,179 @@
   +use Drupal\Core\TypedData\TypedDataAwareInterface;
   

   The trait is good I guess, but we don't need that interface.

7. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinitionInterface.php
   @@ -0,0 +1,179 @@
   +   *   The context definition object.
   

   That's the data definition, not the context definition :).

8. +++ b/core/lib/Drupal/Core/TypedData/TypedDataAwareInterface.php
   @@ -0,0 +1,15 @@
   +interface TypedDataAwareInterface {
   

   As said before, I don't think we need that interface. :/

Yes, the interface is now gone. Other than that I fixed few minor things but there is one problem with context in component. As now the ContextInterface depends on TypedData stuff we have a problem in the component part. Discussed this with @Berdir and he suggested to move the whole Context thing into the core.

So after discussion with @Berdir and @fubhy we decided to move the ContextDefinitionInterface to component and having such interface also in core where it actually provides the only method coupled with TypedData "getDataDefinition()"

1. +++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
   @@ -26,14 +26,17 @@ class Context implements ContextInterface {
   +   * @var \Drupal\Core\Plugin\Context\ContextDefinitionInterface
   ...
   +   * @param \Drupal\Component\Plugin\Context\ContextDefinitionInterface $context_definition
   

   Should be "Component" in both cases I guess ;)

2. +++ b/core/lib/Drupal/Component/Plugin/Context/ContextDefinitionInterface.php
   @@ -0,0 +1,162 @@
   +namespace Drupal\Component\Plugin\Context;
   +
   +
   

   2 newlines.

3. +++ b/core/lib/Drupal/Component/Plugin/Context/ContextDefinitionInterface.php
   @@ -0,0 +1,162 @@
   +   * @see \Drupal\Core\TypedData\TypedDataManager::getConstraints()
   ...
   +   *   \Drupal\Core\TypedData\TypedDataManager::getConstraints() for details.
   

   These references to typed data should probably not stay there if we move it to Component.

4. +++ b/core/lib/Drupal/Core/TypedData/TypedDataTrait.php
   @@ -0,0 +1,49 @@
   + * @file
   + * Contains \namespace Drupal\Core\TypedData\TypedDataTrait.
   

   :)

5. +++ b/core/lib/Drupal/Core/TypedData/TypedDataTrait.php
   @@ -0,0 +1,49 @@
   +   * The typed data manager used for creating the data types of the context.
   

   This is in the trait so it should not refer to context

Thnx for review, all implemented.

+++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinitionInterface.php
@@ -0,0 +1,25 @@
+ * Contains \Drupal\Component\Plugin\Context\ContextDefinitionInterface.

That should be \Drupal\Core\...

hopefully this time...

1. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php
   @@ -0,0 +1,215 @@
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getDataDefinition() {
   +    if ($this->isMultiple()) {
   +      $definition = $this->getTypedDataManager()->createListDataDefinition($this->getDataType());
   +    }
   +    else {
   +      $definition = $this->getTypedDataManager()->createDataDefinition($this->getDataType());
   +    }
   +    $definition->setLabel($this->getLabel())
   +      ->setDescription($this->getDescription())
   +      ->setRequired($this->isRequired())
   +      ->setConstraints($this->getConstraints());
   +    return $definition;
   

   Wondering, this is the only reason we have this whole complexity with injecting typed data manager, and having two interfaces...

   Would not be as nice, but could we move this to the typed data manager and have something like createDataDefinitionFromContextDefinition($context_definition) (a bit long...

   Would make it a bit more complicated to use, but it would remove a lot of complexity from this patch..

2. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginInterface.php
   @@ -0,0 +1,37 @@
   +  /**
   +   * Defines the needed context of this plugin.
   +   *
   +   * @return \Drupal\Core\Plugin\Context\ContextDefinitionInterface[]
   +   *   The array of context definitions, keyed by context name.
   +   */
   +  public static function contextDefinitions();
   

   Not sure I like the method name, but have no better suggestion right now.

   baseFieldDefinitions() and propertyDefinitions() are what the thing contains, this is what it needs.. but as I said, no better idea..

I'm also not completely happy with the two interfaces, but I also don't see how this would look much better directly on the typed data manager.

The removed test cases seem to be fine, as they were asserting behavior that we don't want. Just returning NULL when the context is not set makes more sense.

Since this is already used by Rules and Page Manager in contrib I think this is a good step forward in making progress, so RTBC.

Yeah no, we have a few problems here.

1.) Contexts in static methods... I am very against this. Yes it can work, but you can't use it this way, it has to be parsed back into the definition inside the manager.

2.) in addition to this problem, you have a problem of derivatives for plugins whose context changes. Case in point "Entity is of bundle" would need to change to "Node is of bundle", "Product is of bundle" etc. This is why contexts are on plugin definitions, and why they can be changed via derivatives. If you want to define contexts this way, please see point 1, but keep in mind that you are actively retarding the capabilities of plugin contexts through this methodology and generally making people's lives more difficult.

3.) In order to support plugins that CAN just define a single context (say user role) I have previously suggested (and still favor) a custom annotation class that returns a context definition object. Let's go that route. I think it will make life generally easier. If rules wants to define conditions this way, that's fine, it can hijack the condition manager service and parse contexts into its custom plugin definitions in its own way. Changing core's approach to this seems completely unnecessary and a complete deviation from the approach already set forth.

In short, this needs some work still. But I think it's got some good things going for it. Lets fix these problems and get this in.

Eclipse

There has been some discussion on how to proceed with this between fubhy and Berdir. They have not yet come to a conclusion on what to do here. For now unassigning as I do not know how to proceed.

Yeah, not sure either.

- The current patch did not update filterPluginDefinitionsByContexts() at all, this would be completely broken now, I guess there are only unit tests that still use the old definitions?

- I don't think our current plugin system can deal with separate @annotations, so it would be easier to keep it inside the plugin definition just like we do right now?

- But then the question is who will give you context definition objects based on the current arrays.

- If you look closer at ContextHandler, you can see that it currently hardcodes the assumption that context definitions *are* data definitions, the reason we can't do that anywhere is that context definition and context in general right now is in Drupal\Component.

- I agree that the static method pattern doesn't make much sense because it's a completely different scenario than we have for typed data/field type property definitions, because those receive the field/data definition object and based on that, they can make the returned properties dynamic.

I'm only going to address your 4th point for the moment berdir, just because I don't want this to get out of control.

I specifically didn't push for a ContextHandler in component. This is for reasons two fold:

1.) Drupal needs typed data (or similar) to really function at this level

2.) The plugin component is only interested in classes & interfaces. I have solutions for making that a REALLY workable tool, but drupal core and I have history there so I was seeking the road of least debate and didn't push tim & others toward build the plugin component element of this. I figured I could build it later and then change what's there currently to be a subclass.

In short, I still think my plan there is not bad, and will totally be stuff that can be feature addition instead of API change. Getting context in this way gave me a lot of leeway in terms of this being a relevant API in core today... otherwise we'd have to justify it all again.

I'll see if I can wade in here today and help with this patch, but I'm taking my family out here this morning, so it'll be a few hours.

Eclipse

This is just a reroll against head.

Eclipse

Ok, I ran a limited set of tests against this to make sure I didn't COMPLETELY break stuff. This introduces a ContextDefinition Annotation class and reverts all the static context definition stuff. I also updated the various context subscribers to continue to work, and updated the node type, user role and language conditions. All basically seems to still be working, a cursory run through the UI revealed nothing broken. I dunno that it'll just magically go green, but hey, I can hope. :-D

Eclipse

PS: Sorry for the lack of interdiff, this was hard to debug, and my normal flow didn't support an interdiff right off the bat. I'll try to get one up soon.

Fixed the Contextual Plugins tests, dunno what's up with Node Block functionality, but waaaay too tired to care tonight. Uploaded the interdiff between the last patch and this one.

Eclipse

This should fix the derivative tests. I had to move it to assertEqual() since we have an object in the definition now. (Thanks to fubhy for the explanation)

Eclipse

Ok, I THINK I got it. In addition to the problem's Tim found I looked through the changes in the Context classes (Core and Component) and found that in my haste to adopt what looked like generally saner approaches I removed some of the exception throwing we were depending on which was the root of the problem Tim found. Without both of these fixes all hell breaks loose.

I attempted to update the docs as well to better illustrate how this works (and also to not be a direct copy of @Translation()).

Eclipse

Also, while I'm at it, here's an interdiff from 30-40 which will hopefully give a better snapshot of the whole set of changes from what was initially proposed here.

Eclipse

Beautiful!

1. +++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
   @@ -48,13 +51,21 @@ public function setContextValue($value) {
   +      if ($this->getContextDefinition()->isRequired()) {
   +        $type = $this->getContextDefinition()->getDataType();
   

   This is not about micro optimization or anything... I just find that it looks ugly to do the same chaining twice. Can we do like $definition = $this->getContextDefinition(); and then continue from there? :)

2. +++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginBase.php
   @@ -63,50 +63,47 @@ public function getContextDefinitions() {
   +    // Make sure all context objects are initialized.
   +    foreach ($this->getContextDefinitions() as $name => $definition) {
   +      $this->getContext($name);
        }
   

   That looks rather sub-optimal but I can't think of a better solution right now.

3. +++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
   @@ -7,7 +7,7 @@
   +use \Drupal\Component\Plugin\Context\ContextInterface;
   
   +++ b/core/lib/Drupal/Core/Annotation/ContextDefinition.php
   @@ -0,0 +1,108 @@
   + * Contains ContextDefinition.php.
   

   Wrong "Contains".

4. +++ b/core/lib/Drupal/Core/Annotation/ContextDefinition.php
   @@ -0,0 +1,108 @@
   \ No newline at end of file
   

   Newline.

5. +++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
   @@ -8,84 +8,98 @@
   +      if ($this->getContextDefinition()->isRequired()) {
   +        $type = $this->getContextDefinition()->getDataType();
   

   Same as before.

6. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinitionInterface.php
   @@ -0,0 +1,25 @@
   +use \Drupal\Component\Plugin\Context\ContextDefinitionInterface as ComponentContextDefinitionInterface;
   

   Redundant leading backslash.

7. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
   @@ -132,6 +132,7 @@ public function applyContextMapping(ContextAwarePluginInterface $plugin, $contex
   +    //drupal_set_message('<pre>' . print_r($contexts, TRUE) . '</pre>');
   

   :)

Ok then, how about now?

Eclipse

The previous tests expected required contexts to return a NULL when they are not set, however only optional contexts perform this way. Updated the existing test for the proper response and added a plugin definition & test for the different optional context logic.

Eclipse

Really good. RTBC from me ...

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
@@ -7,7 +7,7 @@
+use \Drupal\Component\Plugin\Context\ContextInterface;

Redundant leading backslash.

Removed redundant leading backslash in this patch. Please review.

Fubhy pointed out a couple of doc issues. I took care of them. Just a docs issue so the RTBC should stand.

Eclipse

Rerolled for HEAD

Eclipse

Yeah, the setters still exist and the constructor values are sane defaults, so this shouldn't be any more difficult to use before I added the parameters to the constructor. I'll file a follow up to change the order.

Eclipse

Did that real quick ...

This should fix the broken tests.

Eclipse

After a bit of Discussion it was determined that we didn't need a Change Notice for this, so I think this is ready to go in any time. I'm working on adding docs to the plugin handbook currently.

Eclipse

Committed and pushed to 8.x. Thanks!

Sigh. :P Always the last patch of the day. :P

Thanks!!!

Hi, me late as usual, following the track of TypedDataTrait that magically popped up in TypedData...

It looks like this one is adding a new (poorly named btw) trait that adds "magic" *public* methods to classes and are not part of any interface....

Also it seems there's a new interface that mostly duplicates DataDefinitionInterface interface instead of maybe fixing / extending it...

Now I'm thinking about:

a) Reopening this one to properly document why / what for we've got two new interfaces, why we cannot reuse existing ones, etc... Does this look like enough explanation for any of them?

/**
 * @file
 * Contains \Drupal\Component\Plugin\Context\ContextDefinitionInterface.
 */

/**
 * Interface for context definitions.
 */
interface ContextDefinitionInterface {

/**
 * @file
 * Contains \Drupal\Core\Plugin\Context\ContextDefinitionInterface.
 */

/**
 * Interface for context definitions.
 */
interface ContextDefinitionInterface extends ComponentContextDefinitionInterface {

Or is there any follow up for that?.

b) Making that new trait's methods part of some existing interfaces to be able to address issues like #2053415: Allow typed data plugins to receive injected dependencies
Anyone working on / interested on / is there already any other related issue I'm missing..?

Before we filed this issue, I was leveraging DataDefinitionInterface for all of this stuff. Fago, Tim Plunkett and myself had long discussions over fago's approach which is essentially what we've adopted here. I too wanted to find a common interface for these, but what we have here is a problem of dependencies. Plugins (and context is part of plugins) are a stand alone php component. Typed Data is not (yet). Building a dependency here was not feasible, and as fago will tell you, despite the marked similarities of these two systems (ContextDefinitions and DataDefinitions) they are in fact separate things for separate uses. At the end of the day I am pleased to not have to spend weeks bikeshedding & detangling the intricacies of trying to combine those two systems. I sympathize with your position, it's actually how I approached this issue initially (before it was filed) but in retrospect, I think what's in core now was a very good idea and really the only way to make forward progress on the very very tight time line we have now.

Eclipse

@EclipseGc,

Really, thanks for the explanation.

I can understand that because we are, for typed configuration, trying to reuse typed data stuff and right, it is a major headache.

Thanks guys for driving this home. Sadly you forgot to credit me for writing all the original code though :-/

I did not mean to change defining context definitions in this issue (yet), as this change is not complete (context definitions are not parsed into plugin definitions) and deserve a separate issue - so the committed change of making use a ContextDefinition annotation class is the right thing to do here (as discussed in Austin afaik)

Looking at the patch I noticed two left overs of the context definition statics, created a quick follow-up: #2305569: Follow-up to context definitions: Remove unused, left-over static contextDefinitions() methods

Besides that, I dislike the add ContextDefinition constructor with many arguments. I opened #2305573: Use setters instead of many ContextDefinition constructor arguments for discussing and hopefully resolving that.

I apologize for your name not being on this fago. Got tunnel vision on getting the patch in. My sincerest apologies. I'll check your follow ups for the other issues you have here.

Eclipse

thanks, no biggie!