Make contexts immutable

To a certain degree I feel like we're using MORE TypedData here, we're just not asking Drupal to do the work, but instead expecting the developer to do it. The main objective here was to prevent calls to the TypedDataManager during set*() methods and during __construct(). This resulted in the removal of all set*() methods on the ContextInterface. TypedData objects themselves have a setValue() method, so to a certain degree this improvement is moot because of that, but at least we've removed another layer of the problem space and placed the greater problem squarely with TypedData. TypedDataInterface itself might benefit from a similar refactor, but that's a separate issue.

This patch needs a bit of love yet since I added TypedDataInterface dependencies into Plugin Components, but I want to see how this fairs with tests.

Eclipse

Cleaning up the tests, some of these have very strange errors in testbot. The component/core issue I mentioned earlier apparently has a test (cool!) I've not tried to fix that yet, will do that once the rest of it appears to be passing.

Eclipse

Interdiff?

Sorry Wim, my bad. I've added interdiffs from 3-5 and 5-8 now. I EXPECT this patch to pass. If it does, I'd love to see if this makes a performance difference.

Eclipse

Nice, green! :)

Probably major given the performance impact. But we don't have numbers yet, so definitely not critical.

Could we get an IS update, please?

Reroll

Some of the conflicts break the immutability of context, but we can fix that after we see how test bot feels.

Eclipse

Fixing an error in the reroll and attempting to make sure that this takes advantage of cacheable metadata.

Eclipse

Thank you berdir for pointing out that I deleted NodeRouteContext.

Eclipse

Attempting to fix the core ContextTest and maintain the immutability of the Context object.

Eclipse

No more need for a reroll, but now that we have a green patch: very much needs profiling.

And an IS update of _what_ this does and why and what the impact on core / contrib is.

#2354889: Make block context faster by removing onBlock event and replace it with loading from a ContextManager landed (yay, progress!), this needs to be rerolled.

After profiling is it a critical or not?

Not critical at all.

Got pointed here by @dsnopek after my comment in #2375695-141: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed

Copy pasting from over there :

The cacheability metadata that got added to Context seems to depend on the history of values the context received over time. - e.g changing a context value from node 1 to node 2 causes the context to have both cache tags node:1 and node:2. while a fresh context created directly with node 2 will only have node:2.

This feels like the kind of histeresis we usually try to avoid ? I guess changing a Context value is not a very common situation, but then again the setContextValue() method is public, so we can't pretend contexts are immutable ?

So it seems that is another good reason to push through with the approach here of making contexts actually immutable :-)

(Sorry for the noise if that was a concerted plan from the get go. The two issues didn't seem to explicitly relate to each other, nor did #2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed mentioned the question of immutability, so at least this is an attempt to make it explicit somewhere)

yched++

Yes, +1 to immutable context objects.

Re-classifying as a bug.

What I am unclear is:

- What API changes does this make?
- What is the impact on already existing contrib modules?

Worked on this issue and here is the new patch.

Rerolled from comment #19

@swathin, @sravya

You need to provide interdiffs with your patches so that people can keep up with your work without having to reread the whole patch.

@Fabianx

The thing it primarily affects is the context providers. Core (as you well know) does a little of this, and Page_manager and Rules. Neither Page_manager nor Rules are far enough along that this would be a huge issue for this. I tend to think (as someone helping develop on of the modules most heavily impacted by this) that it's unlikely to cause huge issue.

Eclipse

#35: It would be great if you could update the issue summary and add a beta evaluation.

I have already pasted the template in.

---

I know it is a lot of effort to take care of issue summaries and beta evaluations, but it really helps others understand the issues and such they can be both more effective in review and helping. Also usually those issues get to be committed much faster.

filling in beta evaluation template.

Restoring some of the issue tags that were lost in #33. Also, I attempted to make some interdiffs (attached) for the patches in #31 and #33, but no promises that they're correct. :-)

And needs a reroll!

Er, 2nd attempt at adding the tags back. :-/

Er, nevermind, doesn't need a reroll! I was on the wrong branch left over from generating the interdiffs. Sorry for all the comments so quickly!

Beta evaluation was added, IS still needs an update AFAICT.

There is a left-over TODO -- but its very close!

Okay first shot re-roll of 21.

Waiting for @timplunkett to give his snarky review. ;)

My snarky review from yesterday got eaten because I took too long to post. Here are the important parts, sans snark.

1. +++ b/core/lib/Drupal/Core/Language/ContextProvider/CurrentLanguageContext.php
   @@ -57,8 +59,9 @@ public function getRuntimeContexts(array $unqualified_context_ids) {
   -        $context = new Context(new ContextDefinition('language', $info[$type_key]['name']));
   -        $context->setContextValue($this->languageManager->getCurrentLanguage($type_key));
   +        $context_value = Language::createInstance(DataDefinition::createFromDataType('language'));
   +        $context_value->setValue($this->languageManager->getCurrentLanguage($type_key));
   +        $context = new Context(new ContextDefinition('language', $info[$type_key]['name']), $context_value);
   

   This kinda sucks as an API change. Why force us to think about TypedData more than we have to?

2. +++ b/core/modules/node/src/ContextProvider/NodeRouteContext.php
   @@ -44,12 +45,12 @@ public function getRuntimeContexts(array $unqualified_context_ids) {
   -      $context->setContextValue(Node::create(array('type' => $node_type->id())));
   +      $context = new Context(new ContextDefinition('entity:node'), EntityAdapter::createFromEntity(Node::create(array('type' => $node_type->id()))));
   

   new new create create a b start

   That's a LOT of things to know. From an Entity to TypedData to ContextDefinition to Context? Why should I have to know that much?

3. +++ b/core/modules/user/src/Tests/Condition/UserRoleConditionTest.php
   @@ -133,7 +134,7 @@ public function testConditions() {
   -    $condition->setContextValue('user', $this->authenticated);
   +    $condition->setContextValue('user', EntityAdapter::createFromEntity($this->authenticated));
   

   This sort of change isn't as bad, but it's still extra work

If NodeRouteContext had its code rearranged slightly, the change I called out in #49.2 would be this:

-      $context->setContextValue($node);
+      $context = new Context(new ContextDefinition('entity:node'), EntityAdapter::createFromEntity($node));

That's pretty awful.

+++ b/core/modules/node/src/ContextProvider/NodeRouteContext.php
@@ -44,12 +45,12 @@ public function getRuntimeContexts(array $unqualified_context_ids) {
     $context = new Context(new ContextDefinition('entity:node', NULL, FALSE));

Also this should probably be $context = NULL; and then after the other calls: if (!$context) { this line.

new new create create a b start

Does that code give you 30 lives in Super IssueQueuetra (tm)?

-      $context->setContextValue($node);
+      $context = new Context(new ContextDefinition('entity:node'), EntityAdapter::createFromEntity($node));

That's pretty awful.

In a functional programming language (or using functional programming style in any language) that would be totally normal! But point taken in Drupal-land.

Would a helper function be enough? For example, something like:

$context->cloneWithValue($node);

I dunno if anyone is still working on this, but I may take a stab at fixing the tests after I finish catching up on issues...

So... I like the immutability of the Context object in this patch.

But assuming I'm understanding this, the performance improvement is from not calling TypedDataManager::create() and requiring the developer to convert the context values to typed data themselves. I'm not terribly crazy about that at all. :-/

I totally misunderstood what @tim.plunkett was complaining about above in #49 and #50. I thought it was about have to initialize so many objects, but it's about the extra knowledge that the developer has to have.

The performance improvement would be nice, but not at this high of a DX cost. Maybe we could split this into two issues: (1) for the immutability of the Context objects, and (2) for removing calls to TypedDataManager::create()?

Anyway, here's a new patch that may or may not pass more tests...

This is what I really meant. :-)

Talked with @japerry and @EclipseGc and agreed we'll just work on the immutability, and remove the TypedData changes.

Cool! Then I have a half finished patch which adds a $context->cloneWithValue($value) and even a few conversions (or revert-sions?). It still needs lots of work (most of the typed data changes are still there), but it's a start.

Just curious what textbook will say

Working on this now...

Ok! This should just be changes to make the context immutable. Let's see what the testbot thinks!

+++ b/core/modules/node/src/ContextProvider/NodeRouteContext.php
@@ -44,12 +44,12 @@ public function getRuntimeContexts(array $unqualified_context_ids) {
-        $context->setContextValue($node);
+        $context = $context->cloneWithValue($node);

I need to think about this naming, because it doesn't feel right, but I don't yet have a better name.

Otherwise, this is looking great! Thanks @dsnopek

Cool, thanks! Unassigning for now...

Okay, a long talk with @japerry, @EclipseGc, and @dsnopek, this is what we came up with.

\Drupal\Core\Plugin\Context\ContextInterface will have this:

  public static function createFromContext(ContextInterface $old_context, $value);

\Drupal\Component\Plugin\Context\ContextInterface does not need a dedicated method, since it is not concerned with preserving cacheability. It can just use the constructor directly.

I think this patch looks great! Assuming tests pass, this is RTBC as far as I'm concerned. :-) Unfortunately, since I wrote a bunch of this code, I can't RTBC it.

As pointed out by @dsnopek, \Drupal\Component\Plugin\ContextAwarePluginBase::__construct() hardcoded the Component Context class. This allows us to override it.

Interdiff looks good! Hopefully this one'll pass tests. :-)

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginBase.php
@@ -41,19 +41,33 @@
+
+    if (isset($configuration['context'])) {
+      $this->contexts = $this->extractContextFromConfiguration($configuration);
+      unset($this->configuration['context']);

This should happen before the parent::__construct() call. The removal of context from configuration is to keep config at the parent level from having to deal with this at all ever.

Otherwise this seems fine.

Eclipse

Skipping #78 for the moment, just want to see the failure numbers.

Here is my closest approximation of what I think @EclipseGc wants.

We cannot simply move the __construct later, since we call getContextDefinition, which needs $this->getPluginDefinition().

+10000000

Looks great from my end, who can rtbc this?

Eclipse

I can. In reading this I think I get what your shooting for.

IS still needs update!

Done

1. +++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
   @@ -31,20 +31,16 @@ class Context implements ContextInterface {
   +   * @param mixed $context_value|NULL
   
   +++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
   @@ -43,11 +43,19 @@ class Context extends ComponentContext implements ContextInterface {
   +   * @param mixed $context_value|NULL
   

   Nit: mixed|null $context_value

2. +++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginBase.php
   @@ -41,17 +41,30 @@
   +   * Converts any context mappings from configuration into context objects.
   ...
   +  protected function createContextFromConfiguration(array $context_configuration) {
   

   Nit: "Converts" in the doc, vs "create" in the method name. Let's be consistent.

3. +++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginBase.php
   @@ -41,17 +41,30 @@
   +   *   An array of context objects.
   

   Nit: "[…], keyed by …." — but this also doesn't happen elsewhere, so probably we don't want this either?

4. +++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
   @@ -170,4 +174,14 @@ public function getCacheMaxAge() {
   +    $context = new static($old_context->getContextDefinition(), $value);
   +    $context->addCacheableDependency($old_context);
   +    $context->setTypedDataManager($old_context->getTypedDataManager());
   +    return $context;
   

   Nit: In principle this could all be chained.

5. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextInterface.php
   @@ -17,6 +17,14 @@
   +   *   The defining characteristic representation of the context.
   

   What is a "defining characteristic representation"?

6. +++ b/core/modules/system/tests/modules/condition_test/src/Tests/OptionalContextConditionTest.php
   @@ -58,6 +59,8 @@ protected function testContextNoValue() {
   +    $type = NodeType::create(['type' => 'example', 'name' => 'Example']);
   +    $type->save();
   

   Nit: NodeType::create([…])->save()

1) Yep
2) Sure
3) Leaving as is.
4) Yes, but leaving as is
5) That's c/p from the parent, using inheritdoc instead (we're only defining this to override the @return docblock)
6) Sure.

RTBC++

+++ b/core/lib/Drupal/Core/Plugin/Context/ContextInterface.php
@@ -17,10 +17,9 @@
   /**
-   * Gets the provided definition that the context must conform to.
+   * {@inheritdoc}
    *
    * @return \Drupal\Core\Plugin\Context\ContextDefinitionInterface
-   *   The defining characteristic representation of the context.
    */
   public function getContextDefinition();

Yay for not spreading a crazy doc :P

But why keep the @return?

Because of the crazy split between
\Drupal\Component\Plugin\Context\ContextInterface
and
\Drupal\Core\Plugin\Context\ContextInterface

It helps immensely to typehint correctly between Component and Core.

Okay, thanks for the explanation!

1. I think given that context is passed around through layers of the application making it immutable is very sensible and will be prevent a whole class of bugs in the D8 life cycle. I'm +1 for this change as reducing fragility and unblocking contrib.
2. This is an API change and therefore needs a CR

3. +++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
   @@ -170,4 +174,14 @@ public function getCacheMaxAge() {
   +    $context->setTypedDataManager($old_context->getTypedDataManager());
   

   The getTypedDataManager method is not on the interface. Is that ok?

4. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextInterface.php
   @@ -24,16 +31,6 @@
   -  public function setContextData(TypedDataInterface $data);
   

   Can remove the use TypedDataInterface...

5. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php
   @@ -15,6 +15,7 @@
   +use Drupal\Core\TypedData\TypedDataInterface;
   

   Not used.

6. +++ b/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
   @@ -8,9 +8,12 @@
   +use Drupal\Core\Plugin\Context\Context;
   

   Not used.

2) Added #2575711: Context objects are now immutable
3) It's not on any interface, so we have no way to check that. But that is a pre-existing condition in HEAD. As such, I'm including a method_exists call here, but not adding any interfaces.
4) No, it's used elsewhere in the file
5) Yep
6) Yep

The patch in #92 appears to fix the concerns in #91, thus marking RTBC to get back into Alex's visibility.

I'm committing this API change under the core committer discretion clause of the beta policy since I think it will eliminate a whole class of likely problem in contrib. Committed 91b4ae5 and pushed to 8.0.x. Thanks!

diff --git a/core/lib/Drupal/Core/Plugin/Context/ContextInterface.php b/core/lib/Drupal/Core/Plugin/Context/ContextInterface.php
index 967819a..067e76a 100644
--- a/core/lib/Drupal/Core/Plugin/Context/ContextInterface.php
+++ b/core/lib/Drupal/Core/Plugin/Context/ContextInterface.php
@@ -9,7 +9,6 @@
 
 use Drupal\Component\Plugin\Context\ContextInterface as ComponentContextInterface;
 use Drupal\Core\Cache\CacheableDependencyInterface;
-use Drupal\Core\TypedData\TypedDataInterface;
 
 /**
  * Interface for context.

Removing the unused use from #91.4 on commit.

It would appear this commit broke Rules 8.x-3.x pretty hard. I'm not up enough on this issue to know the details of this issue, but I suspect fago et al may need to be involved here.

That's not surprising, it broke page_manager too, but they just need to stop using the setters and start using the constructor. Should be a pretty straight forward fix.

Eclipse

@Torenware: The change record describes how code needs to be updated https://www.drupal.org/node/2575711

@dsnopek,

Thanks. I've looked at that now and spent a day applying that to the Rules code base.

One thing I've having trouble with: Rules has a concept of a set context that has a value of NULL. Post this patch, this is busted, and I don't see a valid way to have a set context with a NULL value. Setting it NULL directly won't work, and I don't see a way to create a TypedData object that gets around the problem either.

Is there some way of doing this I'm not seeing, or does the fix for this issue disallow a context with a NULL value?

Follow-up: #2583135: Context::getContextData() sometimes returns NULL which violates ContextInterface