Allow context definitions to specify default values

We should use getDefaultValue() and associated setter methods as we do for field definitions for other definition objects as well. We want them on ContextDefinitions also though.

Should we add getDefaultValue() to DataDefinitionInterface at this point? Or should we stick to getSetting('default_value') as in the patch to make API changes? Should we only support default values for primitive data types such as strings? Or should we also support entities with default values (example: "5" as default value for a node would load node 5)? Where do we put the test cases for this? Should we add to TypedDataTest or try PHPUnit for this?

klausi opened a new pull request for this issue.

So I followed the approach to use the settings place and added a test case for that. So no interface change for DataDefinitionInterface. I added a test case for that.

Then I also started with ContextDefinitionInterface and implemented the same settings approach there. No test cases for that yet.

>Should we add getDefaultValue() to DataDefinitionInterface at this point?
Yes, as said. We need to complete the API here - I don't think the API change is an issue as the base-class will not change. It does not make any sense to cripple the class in order to achieve BC for not existing devs working with this right now.

When adding the default value to the interfaces, it will be there for all data types. However, it has to be compatible with our usage in fields what is problematic because of it's entity parameter :/
Thinking about it, the typed data definition default value is a nice addition, which is not required for d8rules but only aids easing creating field types or other data structures with properties having some default values. Given that I'd suggesting changing the issue scope of this to ContextDefinitions only which should be a simple straight-forward get/set of default values.

+++ b/core/lib/Drupal/Component/Plugin/Context/ContextDefinitionInterface.php
@@ -158,4 +158,23 @@ public function addConstraint($constraint_name, $options = NULL);
+  public function getSettings();

I don't think adding settings to context definition makes sense - there are no custom classes which could make use of custom settings. So everything supported, should be in the interface.

Ok, started from scratch adding the getDefaultValue() method to DataDefinitionInterface and ContextDefinitionInterface.

TODO: test case for context definitions.

Oh, so we have a serious incompatibility with FieldDefinitionInterface now which has a required parameter on the getDefaultValue() method. Not sure how we should solve that.

that I talked about in my last comment: ;)

However, it has to be compatible with our usage in fields what is problematic because of it's entity parameter :/
Thinking about it, the typed data definition default value is a nice addition, which is not required for d8rules but only aids easing creating field types or other data structures with properties having some default values. Given that I'd suggesting changing the issue scope of this to ContextDefinitions only which should be a simple straight-forward get/set of default values.

Thus, as suggested removed typed data definition from the scope. I think we should probably look into doing #2268049: Decouple field definitions from typed data definitions for being able to solve this and #2329937: Allow definition objects to provide options - where we ran into the same issue.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Ok, removed the TypedData changes, let's only do ContextDefinition then.

Thanks, patch looks good. However, moreover we should make the default values work, not? :-)
I.e., the context class should return the default value instead of NULL if it's unset.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Right, implemented that for both context classes and also added test cases.

Thanks, here some small remarks:

1. +++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
   @@ -54,11 +54,13 @@ public function getContextValue() {
   +      if ($default_value === NULL && $definition->isRequired()) {
   

   minor: we usually use !isset() for checking NULL

2. +++ b/core/lib/Drupal/Component/Plugin/Context/ContextDefinitionInterface.php
   @@ -110,6 +110,24 @@ public function isRequired();
   +   *   The default value to be set.
   

   or NULL to remove it?

3. +++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
   @@ -72,7 +74,14 @@ public function getConstraints() {
   +      return $this->getTypedDataManager()->create($definiton->getDataDefinition(), $default_value);
   

   To speed up further usages, should we keep a reference on our typed data object and set it in the context?

4. +++ b/core/tests/Drupal/Tests/Core/Plugin/Context/ContextTest.php
   @@ -0,0 +1,71 @@
   +  public function testDefaultValue() {
   +    $mock_definition = $this->getMockBuilder('Drupal\Core\Plugin\Context\ContextDefinitionInterface')
   

   Any reasons for replicating this test method in core when it is already in component?

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Fixed the comment and the isset() check.

3: I don't think we should introduce more complexity here and the optimization of caching the typed data object is necessary.

4: The test case is the same, but the core Context class overrides the method, so we should also make sure that the overridden method works as expected to avoid regressions.

Thanks.

ad: 4. k, makes sense

ad 3:

+++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
@@ -72,7 +74,14 @@ public function getConstraints() {
+    if (isset($this->contextData)) {
+      return $this->contextData;
...
+    if (isset($default_value)) {
+      return $this->getTypedDataManager()->create($definiton->getDataDefinition(), $default_value);
+    }

Actually, I figured there is already $this->contextData - so the returned value should be written to the property? I do not see a point in re-creating it every time it's called and the variable to keep it is already there, so we should use it to avoid having an inconsistency inside the class.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Thanks, that looks good now. Also adding tag as this is needed by rules.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Just a reroll, let's see if this still works.

Thanks. Took another look, patch is still good.

1. Looks like we need to update the documentation in \Drupal\Core\Annotation\ContextDefinition - specifically @defgroup plugin_context Annotation for context definition

2. +++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
   @@ -72,7 +79,17 @@ public function getConstraints() {
   -    return $this->contextData;
   +    if (isset($this->contextData)) {
   +      return $this->contextData;
   +    }
   +    $definiton = $this->getContextDefinition();
   +    $default_value = $definiton->getDefaultValue();
   +    if (isset($default_value)) {
   +      // Keep the default value here so that subsequent calls don't have to look
   +      // it up again.
   +      $this->contextData = $this->getTypedDataManager()->create($definiton->getDataDefinition(), $default_value);
   +      return $this->contextData;
   +    }
      }
   

   How about structuring this so there aren't two returns? Eg.

     /**
      * {@inheritdoc}
      */
     public function getContextData() {
       if (!isset($this->contextData)) {
         $definition = $this->getContextDefinition();
         $default_value = $definition->getDefaultValue();
         if (isset($default_value)) {
           // Store the default value so that subsequent calls don't have to look
           // it up again.
           $this->contextData = $this->getTypedDataManager()->create($definition->getDataDefinition(), $default_value);
         }
       }
       return $this->contextData;
     }
   

3. +++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
   @@ -72,7 +79,17 @@ public function getConstraints() {
   +    $definiton = $this->getContextDefinition();
   +    $default_value = $definiton->getDefaultValue();
   +    if (isset($default_value)) {
   +      // Keep the default value here so that subsequent calls don't have to look
   +      // it up again.
   +      $this->contextData = $this->getTypedDataManager()->create($definiton->getDataDefinition(), $default_value);
   

   $definiton is misspelt.

4. +++ b/core/tests/Drupal/Tests/Core/Plugin/Context/ContextTest.php
   @@ -0,0 +1,91 @@
   +   * @var \Drupal\Core\Plugin\Context\ContextDefinitionInterface|PHPUnit_Framework_MockObject_MockObject
   ...
   +   * @var \Drupal\Core\TypedData\TypedDataManager|PHPUnit_Framework_MockObject_MockObject
   ...
   +   * @var \Drupal\Core\TypedData\TypedDataInterface|PHPUnit_Framework_MockObject_MockObject
   

   PHPUnit_Framework_MockObject_MockObject is not properly namespaced

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Fixed all comments by alexpott, interdiff attached.

Good remarks. They have been all addressed - thus back to RTBC.

The testbots are having troubles, Im working on one that was running this last patch, so if it comes back failed, I would try retesting it. (I had to reboot mysql on the bot)

Sorry for the inconvenience.

Was only a testbot fluke, back to RTBC.

This issue is a normal bug fix, and its disruption is limited and it unblocks contrib, so it is allowed per https://www.drupal.org/core/beta-changes.Committed c5e5ad4 and pushed to 8.0.x. Thanks!