Convert ContextAwarePluginBase to traits

The difference is really confusing, +1

Just an idea.

Do we still want this? I noticed we now have a \Drupal\Component\Plugin\ContextAwarePluginBase too.

Unassigning stale issue. Hopefully someone else will pursue this.

We've always had a Component version of this class. I wrote that first.

I don't know how I missed this. I support this in principle, but the code that's there isn't going to work, we'll need to abstract the component version first and then figure out how we want to handle the TypedData specifics that the core implementation provides.

Eclipse

1. I've reviewed the patch in #33 and it looks great! I added an additional deprecation notice to \Drupal\Core\Condition\ConditionManager::createInstance() for passing context values to plugins via configuration.
2. I've begun a change record here. Let me know if any changes should be made (it's my first change record).
3. I've submitted a patch attached to this comment to finish-up the remaining inline documentation.

Looks good, I just removed an unused use statement in one of the test cases to fix the code style warning that was reported.

Sorry for stepping on your toes on the @return overrides; didn't notice the distinction there!

What's the plan for \Drupal\Component\Plugin\ContextAwarePluginInterface once we hit Drupal 10? I noticed that the only two places it's used are in \Drupal\Component\Plugin\ContextAwarePluginBase (which is now deprecated for removal in 10.x) and \Drupal\Core\Plugin\ContextAwarePluginInterface.

The reason I ask is that if we intend to remove the component interface in favor of the core interface, we should probably note that in the inline documentation, no? I believe a deprecation notice via trigger_error() would be a bit heavy handed here, especially since it's still the ancestor of the core interface.

I'm not sure if this warrants a reversion of RTBC; I'll leave that to your better judgement @tim.plunkett.

Backporting patch to 8.9.x branch for additional testing.

Fixing failing tests.

9.1.x issue now, which means we need to update the deprecation message at least.

Updating backport patch for testing on 8.9.x.

Updating main patch. Change record has also been updated to reflect the new target.

Please advise whether RTBC should be reverted on the reported test error:

fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-2723.xml:
PHPunit Test failed to complete; Error: PHPUnit 8.5.2 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Scripts\TestSiteApplicationTest
FFI...I 7 / 7 (100%)

Time: 26.58 seconds, Memory: 4.00 MB

There were 2 failures:

1) Drupal\Tests\Scripts\TestSiteApplicationTest::testInstallWithNonExistingFile
No additional tables created in the database
Failed asserting that actual size 895 matches expected size 858.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:116
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:54
/var/www/html/vendor/phpunit/phpunit/src/Framework/Assert.php:2887
/var/www/html/vendor/phpunit/phpunit/src/Framework/Assert.php:492
/var/www/html/core/tests/Drupal/Tests/Scripts/TestSiteApplicationTest.php:63
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:691

2) Drupal\Tests\Scripts\TestSiteApplicationTest::testInstallWithFileWithNoClass
No additional tables created in the database
Failed asserting that actual size 956 matches expected size 926.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:116
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:54
/var/www/html/vendor/phpunit/phpunit/src/Framework/Assert.php:2887
/var/www/html/vendor/phpunit/phpunit/src/Framework/Assert.php:492
/var/www/html/core/tests/Drupal/Tests/Scripts/TestSiteApplicationTest.php:81
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:691

FAILURES!
Tests: 7, Assertions: 37, Failures: 2, Incomplete: 2.

A discussion via Slack on #46 has yielded that the test failure is from 9.1.x/HEAD thus the most recent patch can be considered ready for review.

Now that the tests have passed for the most recent patch, I wanted to highlight a question that may have been missed earlier in the thread for the reviewer to consider:

What's the plan for \Drupal\Component\Plugin\ContextAwarePluginInterface once we hit Drupal 10? I noticed that the only two places it's used are in \Drupal\Component\Plugin\ContextAwarePluginBase (which is now deprecated for removal in 10.x) and \Drupal\Core\Plugin\ContextAwarePluginInterface.

The reason I ask is that if we intend to remove the component interface in favor of the core interface, we should probably note that in the inline documentation, no? I believe a deprecation notice via trigger_error() would be a bit heavy handed here, especially since it's still the ancestor of the core interface.

Hopefully this fixes the patch. Seems as though there was a conflict introduced in a test case.

1. +++ b/core/tests/Drupal/KernelTests/Core/Plugin/Context/ContextAwarePluginTraitTest.php
   @@ -0,0 +1,134 @@
   +  public function setUp() {
   

   Should be protected function setUp(): void here. #3107732: Add return typehints to setUp/tearDown methods in concrete test classes just got committed

2. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginTrait.php
   @@ -0,0 +1,241 @@
   +      $this->prepareContext();
   

   Deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Instead, call ::setContextValue() on the plugin itself. @see https://www.drupal.org/node/3120980
   

3. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginTrait.php
   @@ -0,0 +1,241 @@
   +    throw new ContextException(sprintf("The %s context is not a valid context.", $name));
   

   Not sure, whether sprintf() is still encouraged to use. See #3118957: Evaluate replacing all usage of sprintf by dot concatenation or variable inside string

4. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginTrait.php
   @@ -0,0 +1,241 @@
   +    if ($definition instanceof ContextAwarePluginDefinitionInterface) {
   +      return $definition->getContextDefinitions();
   +    }
   +    else {
   +      return !empty($definition['context_definitions']) ? $definition['context_definitions'] : [];
   +    }
   ...
   +        return $definition->getContextDefinition($name);
   ...
   +    elseif (!empty($definition['context_definitions'][$name])) {
   +      return $definition['context_definitions'][$name];
   +    }
   

   And I would like to kill elses, for instance:

   function foo ($bar) {
     if ($bar) {
        return "baz";
     }
     else {
       return "qua";
     }
   }

   can be

   function foo ($bar) {
     if ($bar) {
        return "baz";
     }
     return "qua";
   }

   As inside if, if the condition meets, the function returns. else killed.

1. Done.
2. I believe the intention was to remove this call altogether once the target method has been removed (this is mimicking the now-deprecated logic at the end of ContextAwarePluginBase::__construct()).
3. I've replaced this with string concatenation in the files that this patch was already modifying, but this still leaves behind an sprintf() in \Drupal\Component\Plugin\Context\Context. Let me know if this file should be updated too.
4. I agree for the first case of this, but the logic flow in the second case could be broken if a ContextAwarePluginDefinitionInterface is encountered without a matching context definition. I recommend that we leave the second case alone. Correct me if I'm wrong on this.

@clayfreeman thanks!

2. My bad. did not realize it's the CR associated with this issue
3. I'm ok with that. as it's still undecided yet.
4. Yes, kill else, not elseif. :)

1. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginTrait.php
   @@ -0,0 +1,240 @@
   +trait ContextAwarePluginTrait {
   ...
   +   * {@inheritdoc}
   ...
   +   * {@inheritdoc}
   ...
   +   * {@inheritdoc}
   ...
   +   * {@inheritdoc}
   ...
   +   * {@inheritdoc}
   ...
   +   * {@inheritdoc}
   ...
   +   * {@inheritdoc}
   ...
   +   * {@inheritdoc}
   ...
   +   * {@inheritdoc}
   ...
   +   * {@inheritdoc}
   ...
   +   * {@inheritdoc}
   ...
   +   * {@inheritdoc}
   ...
   +   * {@inheritdoc}
   ...
   +   * {@inheritdoc}
   

   {@inheritdoc}s should be replaced with real docs. A trait can not inherit docs.

2. The question in #49 might be missed and should be answered, maybe, @tim.plunkett is the right person to answer it :)

   What's the plan for \Drupal\Component\Plugin\ContextAwarePluginInterface once we hit Drupal 10? I noticed that the only two places it's used are in \Drupal\Component\Plugin\ContextAwarePluginBase (which is now deprecated for removal in 10.x) and \Drupal\Core\Plugin\ContextAwarePluginInterface.

   The reason I ask is that if we intend to remove the component interface in favor of the core interface, we should probably note that in the inline documentation, no? I believe a deprecation notice via trigger_error() would be a bit heavy handed here, especially since it's still the ancestor of the core interface.

   Meanwhile, \Drupal\Component\Plugin\ContextAwarePluginInterface vs \Drupal\Core\Plugin\ContextAwarePluginInterface, it's CONFUSING too. Just like \Drupal\Component\Plugin\ContextAwarePluginBase vs \Drupal\Core\Plugin\ContextAwarePluginBase

3. Furthermore, a similar question, what about ContextInterface and Context?

   • \Drupal\Component\Plugin\Context\ContextInterface vs \Drupal\Core\Plugin\Context\ContextInterface
   • \Drupal\Component\Plugin\Context\Context vs \Drupal\Core\Plugin\Context\Context

NW for #1. Separate issue(s) for #2 and #3?

1. Can you cite a requirement for this? I found multiple other examples of {@inheritdoc} being used in traits that are paired with interfaces that the referencing class is expected to implement (e.g. Drupal\Core\Cache\CacheableDependencyTrait and Drupal\Core\Cache\CacheableDependencyInterface). If this is something that needs to be done, I'm more than happy to do it, but we should also create a "clean-up" ticket for other occurrences as well.
2. Agree, the component / core namespacing is a bit odd and confusing in the other areas that you highlight. I'd love @tim.plunkett's insight into this as well as @eclipseGc and @effulgentsia since those three maintain the plugin subsystem. I've started a discussion for this on Slack.
3. My opinion is that we should limit the scope of this issue to just make the traits and we can handle the disambiguation clean-up work in another issue.

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginBase.php
@@ -89,7 +94,7 @@ public function getContextDefinition($name) {
+    throw new ContextException('The ' . $name . ' context is not a valid context.');

Seems completely unrelated and also not as nice or as safe as what is there today. Reading the linked issue above, I don't really buy the logic for removing sprintf() in general. Let's remove this from the patch.

More generally, there are a few things here that pain me.

• First and foremost, I think I need a compelling argument to break down the walls between the Core/Component split. I personally still hope that as Drupal becomes more Composer centric we will eventually evaluate the work done in the Component sections of our code base and begin to make them available as libraries. If we've given up all together on that, I probably need to be pointed at issues where that conversation is/has happened and a decision has been made in favor of not writing platform generic code.
• Secondly, the patch feels a bit heavy handed. We're stamping out base classes in favor of traits rather than moving base class methods into traits and exclusively updating the base class. It feels like this would radically simplify the patch. If we want to follow up by removing the base class from existing implementations later, that feels like follow up territory to me.
• Finally, there are decisions around no longer supporting constructor injection of contexts in the patch. My read of this (right/wrong/or otherwise) is that traits can't have constructors so we created the prepareContext() method, but this became ugly because we simultaneously eliminated the base class. If we kept the base class, its constructor could call the trait method and the whole thing is transparent to downstream implementations.

This feels like it needs some more work yet, and ideally more discussion. I +1'd the idea many years ago and am still +1 to the trait option, but I think I'd like to see a slightly different implementation here.

Also, while I'm not certain what other traits are doing with regard to inheritdoc, I'm ok with it so long as we have an @see to the intended interface implementation. Again I know we have standards around this, so we should figure out what that standard is and follow it. I'm just espousing an opinion here.

Eclipse

Uploading this for preliminary discussion and test bot; not expected to be worthy of being the final patch.

I agree that any clean up should be in a follow up ticket if we want to squash the base class, but I don't believe we'll even want to.

I opted not to include the ::prepareContext() method in the trait since it seemed extraneous to me; all that you have to do in your constructor if you're not extending ContextAwarePluginBase is the following:

  public function __construct(array $configuration, $plugin_id, $plugin_definition) {
    $context_configuration = isset($configuration['context']) ? $configuration['context'] : [];
    unset($configuration['context']);

    parent::__construct($configuration, $plugin_id, $plugin_definition);

    $this->context = $this->createContextFromConfiguration($context_configuration);
  }

Obviously, this also has the added benefit of allowing one to customize the approach that they take for sourcing the context configuration; cleaner & better in my opinion.

I also added an @see to the trait that references the interface that should be implemented.

Let's see if tests pass for this...

About {@inheritdoc}s in ContextAwarePluginTrait, I have not fond the standard about it yet.. As there is an @see above the Trait definition and PHPStorm on my local can understand it. I am totally ok to keep them.

And about the sprint() thing, yes, it’s unrelated, and I already commented "3. I'm ok with that. as it's still undecided yet." in #55.

As per discussion with @EclipseGc and @tim.plunkett via Slack call, we decided to middleground the issue:

1. Component class will be deprecated in a separate issue; we can't deprecate in this issue due to an out of scope change required in testing infrastructure to be able to accomplish this.
2. Context configuration will still be deprecated as ::setContextValue() should be used instead to avoid the initialization that we can't do in traits due to prohibition on constructors in them. Implicit initialization is performed via ::prepareContext() until the deprecated functionality is removed.
3. Reverted back to using sprintf() since that has yet to be decided and is presently out of scope of this issue.

This means that ContextAwarePluginBase will be deprecated for contrib and replaced with PluginBase implements ContextAwarePluginInterface and use ContextAwarePluginTrait; for all core code.

Interdiff posted for this is from #54.

+++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginTrait.php
@@ -0,0 +1,240 @@
+      throw new ContextException("Passed $name context must be an instance of \\Drupal\\Core\\Plugin\\Context\\ContextInterface");

So, we have a variable in the exception, we need it in sprintf(). $name isn't typehinted, so no guarantees what that is. I know this is how it was previously, but let's fix it. It should never have gone in this way.

Otherwise this looks great.

Eclipse

Attached patch better demonstrates (with diff.renames) that #64 wasn't introduced by the patch in #62. The interdiff is a bit odd due to the config change, so I've omitted it; git apply 65.patch && git apply -R 61.patch should result in a clean working tree.

Ok, then I retract my statement about the exception. This looks good to me. RTBC pending passing tests.

Eclipse

Expect a test failure; citation from 9.1.x that seems to indicate an intermittent failure: https://www.drupal.org/pift-ci-job/1730324

The fact that #62 passed and there are no changes from that patch seems to line up with my suspicion.

Yeah WidgetUploadTest is a very frequent random fail in HEAD.

Meanwhile, patch review:

1. +++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginBase.php
   @@ -42,6 +42,9 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
        parent::__construct($configuration, $plugin_id, $plugin_definition);
    
   +    if ($context_configuration) {
   +      @trigger_error('Passing context values to plugins via configuration is deprecated in drupal:9.1.0 and will be removed before drupal:10.0.0. Instead, call ::setContextValue() on the plugin itself. See https://www.drupal.org/node/3120980', E_USER_DEPRECATED);
   +    }
        $this->context = $this->createContextFromConfiguration($context_configuration);
      }
   
   +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php
   @@ -3,21 +3,20 @@
   -use Drupal\Component\Plugin\Context\ContextInterface as ComponentContextInterface;
   -use Drupal\Core\Plugin\Context\ContextInterface;
   +
   +@trigger_error(__NAMESPACE__ . '\ContextAwarePluginBase is deprecated in drupal:9.1.0 and will be removed before drupal:10.0.0. Instead, use \Drupal\Core\Plugin\ContextAwarePluginTrait. See https://www.drupal.org/node/3120980', E_USER_DEPRECATED);
    
    /**
     * Base class for plugins that are context aware.
     */
   

   • So, the whole of Drupal\Core\Plugin\ContextAwarePluginBase is being deprecated, but not the whole of Drupal\Component\Plugin\ContextAwarePluginBase. Only the "passing values via configuration" part is deprecated.
   • The Drupal\Core one extends the Drupal\Component one.
   • The things like BlockBase that extend the Drupal\Core one are being changed to extend straight-up PluginBaseand implementing the inteface, rather than extending the component one, despite the component one not being deprecated.

   What's the scoop on all that?

2. copy from core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php
   copy to core/lib/Drupal/Core/Plugin/ContextAwarePluginTrait.php
   
   +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginTrait.php
   @@ -103,7 +122,12 @@ public function setContextMapping(array $context_mapping) {
   +    $definition = $this->getPluginDefinition();
   

   This doesn't look to be a method on the trait; how can we be sure it exists wherever the trait is used?

3. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginTrait.php
   @@ -2,52 +2,53 @@
   +use Drupal\Component\Plugin\Context\ContextInterface as ComponentContextInterface;
   

   Remind me again why we have one copy of all this stuff in Drupal\Core and another in Drupal\Component? (I knew the answer to this at some point, but have since forgotten.) The docblock of the Drupal\Core one just says:

   An override of ContextAwarePluginInterface for documentation purposes.

   Uh, wha?

4. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginTrait.php
   @@ -2,52 +2,53 @@
   +    if (!$this->context) {
   +      $this->prepareContext();
   +    }
   

   I guess this is because we can't rely on its existence because there's no constructor to set it.

5. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginTrait.php
   @@ -112,16 +136,56 @@ public function getContextDefinitions() {
   +    // @todo: Implement symfony validator API to let the validator traverse
   +    // and set property paths accordingly.
   

   No @todo without followups, please. Also, there should not be a colon after @todo, and our standard is to add two spaces to indent subsequent lines of the to-do (after the // for inline comments).

6. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginTrait.php
   @@ -112,16 +136,56 @@ public function getContextDefinitions() {
   +   * Initializes context values from configuration.
       *
   -   * @return \Drupal\Core\Plugin\Context\ContextHandlerInterface
   +   * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Instead,
   +   *   call ::setContextValue() on the plugin itself.
   +   * @see https://www.drupal.org/node/3120980
       */
   -  protected function contextHandler() {
   -    return \Drupal::service('context.handler');
   +  private function prepareContext() {
   

   I'm confused by this deprecation. I couldn't find a prepareContext() method on either the core or component base classes. But if it's not coming from the parent, then why the combination of private and deprecated? Especially with a deprecation message that says what the caller should do, when it's... private. What's the story with this?

Ah, here's the answer to one of my questions at least:

Component class will be deprecated in a separate issue; we can't deprecate in this issue due to an out of scope change required in testing infrastructure to be able to accomplish this.

Can we get an @todo and a followup for that in the code? Because it's WTFy as-is.

Thanks @xjm for the review; I'm happy with the progress that we've made on this today. Here are my answers to your questions as best as I understand them:

1. The current state of component/core is a mess WRT context-aware plugins. From my understanding, component is going to be killed with fire in a separate ticket, as you observed in #70; I can update the patch to include a @todo for this. Ideally when everything is said and done, only core will exist and only as an interface / trait (once base class is removed after deprecation).
2. Good question. Input on this from the subsystem maintainers is welcome. I imagine since there's a "weak requirement" on ContextAwarePluginInterface, we can assume that it's available, but I'm not sure if that's best practice.
3. We're having to type juggle between component and core, so that won't be an issue once component is killed. We have approval to kill component from @alexpott in #26.
4. Correct. We have to lazy-initialize the context from configuration until it's removed in 10.x.
5. I think this @todo may have come from component code; not sure that we're responsible for that being there in this ticket.
6. ::prepareContext() is the temporary way that we're handling laxy initialization of context from configuration in the new trait. It will be removed in 10.x when support for context via configuration is removed. This is why it's defined as private and marked as deprecated.

@xjm

re: 2

ContextAwarePluginInterface (which this trait implements) extends the Component interface of the same name which extends \Drupal\Component\Plugin\PluginInspectionInterface which is where this method is found. So, in so far as a trait COULD be used anywhere, this one is explicitly meant to satisfy the Core/ContextAwarePluginInterface and so long as it is used as intended, the PluginInspectionInterface::getPluginDefinition() method should 100% always be available.

re: 6

This was how Tim encapsulated the code that was originally in the constructor for config context injection purposes. Since it's private but in a trait, any class that uses the trait will have access to the private method. So putting a deprecation notice in it from day one (since we're not going to support constructor context injection) seems sensible. Here's a quick code explanation. This is valid:

<?php

trait foo {
	
	private function myfunc() {
		return 'foo::myfunc';
	}
}

class Bar {
	use foo;
	
	public function test() {
		return $this->myfunc();
	}
}

$bar = new Bar();
print $bar->test();
// will print "foo::myfunc"

So the patch is just trying to prevent anyone from relying on that method.

Eclipse

Updating the patch to include an issue for the dangling @todo in ::validateContexts(). Also adding a @todo for deprecating \Drupal\Component\Plugin\ContextAwarePluginBase and \Drupal\Component\Plugin\ContextAwarePluginInterface.

I think we're back to RTBC since only the todo portion of xjm's review seemed actually actionable. I'll add a little more commentary here to respond to that review.

re: 4

Yes :-)

re: 3

The Drupal\Component namespace was intended, from day one, to be a place we could subtree split and make our components more widely available to the PHP community. A trojan horse for both spreading Drupal code outside of Drupal as well as pulling non-Drupal PHPists into the Drupal community bit by bit. While this goal still has not manifest in any actionable way, I 100% support it still and I also support Plugins as a more widely available component. That being said (and admittedly, tangential to this issue) there are issues with the Plugin system in Component.

1. People are consistently confused by the Core/Component dichotomy. I'm not entirely sure why, but I probably don't get a vote since I helped write it. In short, what's in Component is intended for use in ANY PHP project, and what's in Core is a Drupal specific override mostly for TypedData/Entity as contexts purposes.
2. I still support Plugins as a Component, however I don't do much in the way of maintaining Plugins in the issue queue these days (a trend I'm trying to break). The whole thing would likely be easier to maintain if the Component portion of it didn't exist. Others are doing that work by and large these days and want the split gone. I support their efforts more than my historic efforts at Componentization of Plugins.
3. Removing the Component portion will mean that our Core code base grows, but the final git commit for this will likely be net-negative lines of code.

I think I'm delving into "remove component" follow up territory at this point, but I hope that helps give context to why the core/component split exists.

Eclipse

All review criteria should be addressed by this patch. #3153956: Remove code related to "context from configuration" that was deprecated in Drupal 9 was created to remove deprecations from this issue when Drupal 10.0.x hits.

Re: @see inside @todo

We should probably have someone update the screenshot in this section of standards to reflect this suggestion.

Looks good to me.

I can definitely ++ this logic.

1. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginTrait.php
   @@ -2,52 +2,71 @@
   +    // @todo Remove this entire block in Drupal 10.0.x.
   +    //   See https://www.drupal.org/project/drupal/issues/3153956.
   +    if (!$this->context) {
   +      if ($this instanceof ConfigurableInterface) {
   +        $configuration = $this->getConfiguration();
   +      }
   +      else {
   +        $reflection = new \ReflectionProperty($this, 'configuration');
   +        $reflection->setAccessible(TRUE);
   +        $configuration = $reflection->getValue($this);
   +      }
   +
   +      if (isset($configuration['context'])) {
   +        @trigger_error('Passing context values to plugins via configuration is deprecated in drupal:9.1.0 and will be removed before drupal:10.0.0. Instead, call ::setContextValue() on the plugin itself. See https://www.drupal.org/node/3120980', E_USER_DEPRECATED);
   +        foreach ($configuration['context'] as $key => $value) {
   +          $context_definition = $this->getContextDefinition($key);
   +          $this->context[$key] = new Context($context_definition, $value);
   +        }
   +      }
   +    }
   

   Is it possible that this is called multiple times on a request - and end up doing the reflection multiple times?

   Should we initialise $this->context to NULL and only do this if it is NULL and then set it to an empty array in here?

   Is there test coverage of both code paths? I think TestContextAwarePlugin tests the reflection path but I'm not sure about the other.

2. +++ b/core/modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/mock_block/MockComplexContextBlock.php
   @@ -2,14 +2,17 @@
   -class MockComplexContextBlock extends ContextAwarePluginBase {
   +class MockComplexContextBlock extends PluginBase {
   
   +++ b/core/modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/mock_block/MockUserNameBlock.php
   @@ -2,14 +2,17 @@
   -class MockUserNameBlock extends ContextAwarePluginBase {
   +class MockUserNameBlock extends PluginBase {
   
   +++ b/core/modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/mock_block/TypedDataStringBlock.php
   @@ -10,7 +11,9 @@
   -class TypedDataStringBlock extends ContextAwarePluginBase {
   +class TypedDataStringBlock extends PluginBase {
   

   Are we okay with these not implementing the ContextAwarePluginInterface interface - other test classes that now use the trait do this.

1. I decided it would be best to have an internal trait property, $initializedContextConfig, used to track the state of initialization; if we initialize $context to NULL, we could run into potential type safety issues, or BC concerns where implementing classes depend on the NULL value instead of an empty array. Plus, it was much easier to account for an additional boolean flag instead of changing a bunch of code in other places.
2. I've added the interface, but haven't tested this specific change locally. I don't anticipate a test failure here anyway, so I'm fine waiting for testbot to find any issues that may present themselves.

Looks like a random failure. Re-queued.

NR for https://www.drupal.org/node/3176667

here's

Committed bc0cbe8 and pushed to 9.1.x. Thanks!

+++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php
@@ -3,21 +3,20 @@
-use Drupal\Component\Plugin\Context\ContextInterface as ComponentContextInterface;
-use Drupal\Core\Plugin\Context\ContextInterface;
+
+@trigger_error(__NAMESPACE__ . '\ContextAwarePluginBase is deprecated in drupal:9.1.0 and will be removed before drupal:10.0.0. Instead, use \Drupal\Core\Plugin\ContextAwarePluginTrait. See https://www.drupal.org/node/3120980', E_USER_DEPRECATED);
 
 /**
  * Base class for plugins that are context aware.
  */
 abstract class ContextAwarePluginBase extends ComponentContextAwarePluginBase implements ContextAwarePluginInterface, CacheableDependencyInterface {
+
+  use ContextAwarePluginTrait;

Why wasn't the class flagged as deprecated? All we have is a runtime deprecation. This cannot be caught by PHPStan.