Create a trait and base class to implement \Drupal\Component\Plugin\ConfigurableInterface

In PHP 5 there are weird issues with traits redefining properties.
In order to be bulletproof, the trait should have a protected $configuration;, but it will conflict with \Drupal\Component\Plugin\PluginBase::$configuration

\Drupal\Component\Utility\NestedArray::mergeDeepArray() has an interesting side-effect on the sorting.
If you pass it two arrays, it uses the order of the keys of the first array, but the values of the second array.
Only one test is affected by this, and it doesn't really need to care about the strict ordering of visibility conditions at runtime.

Building off the PHP5 patch from above since it works with both.

yeah, wouldn't it be nice to live in a world where PLs don't promise ordering of a hashmap, to be able to optimize its speed potentially?

+++ b/core/lib/Drupal/Core/Plugin/ConfigurablePluginTrait.php
@@ -0,0 +1,42 @@
+trait ConfigurablePluginTrait {
...
+  public function calculateDependencies() {

It is a little bit weird to put calculateDependencies() in there, but I get why, this code is needed everytime you have some configuration which might depend on some external stuff.

ConfigurablePluginInterface (incorrectly, IMO) extends DependentPluginInterface. But we could just pretend that isn't true, and only provide methods on the trait for the ones we care about...

also, #2579743: Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed intends to expand DependentPluginInterface...

Works for me. I guess a CR so people are aware of it would be great.

Regarding #8, looks like https://www.drupal.org/node/2852190 is already attached here; it probably just will need an update for this issue.

I almost assigned this issue to alexpott for review before I realized he filed it.

Attaching a few related issues.

@alexpott and I agreed on adding a new CR that is clear about this specific change since this is something developers will want to know about. @tim.plunkett is working on it now.

Added a new CR: https://www.drupal.org/node/2853355

+++ b/core/lib/Drupal/Core/Plugin/ConfigurablePluginTrait.php
@@ -0,0 +1,35 @@
+/**
+ * Implements \Drupal\Component\Plugin\ConfigurablePluginInterface.
+ *
+ * @ingroup Plugin
+ */
+trait ConfigurablePluginTrait {

Okay sorry, one more piece of feedback. "Implements FooInterface" does not tell us what it is for. I'd add a paragraph to this docblock explaining that configurable plugins can use the trait to ensure their default configuration is merged correctly and avoid the need for boilerplate implementations.

Other than that this looks great to me. 9 files changed, 55 insertions, 113 deletions is a happy diffstat.

Oh also what about AggregatorPluginSettingsBase and other aggregator stuff? What distinguishes the implementations converted in this patch?

I missed AggregatorPluginSettingsBase because it doesn't define a stub [gs]etConfiguration() method, which is what I was grepping for.
However, it's not an appropriate use for it, because nothing else is responsible for the plugin's config (which is bad), so the implementations are forced to use configFactory directly!

So I went back through every plugin type and found a couple more that can use it.

The only other one I left alone was FilterBase, because it provides implementations of all 3 methods in the trait, and does so very differently than the trait. And it's filter, so definitely not changing things unless necessary.

Also expanded the docs per #12

Yay more less things!

+1 for not tinkering with Filter. Looks like the aggregator implementation dates to #1957330: Make possible for parsers and fetchers to expose configuration through plugins and some even older stuff that dates back to the original config entity discussion in Munich! Should we have a followup to do... something... for aggregator?

Updated docs look good to me too.

This looks good, but why a trait rather than a base class?

Providing a base class would also cover this bit of boilerplate that's needed in __construct():

  /**
   * {@inheritdoc}
   */
  public function __construct(array $configuration, $plugin_id, $plugin_definition) {
    parent::__construct($configuration, $plugin_id, $plugin_definition);
    $this->setConfiguration($configuration);
  }

(Closing #2900418: provide a plugin base class for ConfigurablePluginInterface as potential duplicate.)

Suppose you want to be context-aware AND configurable? Lack of multiple inheritance means not everything can be a base class.

Fair point, but I can only see test classes in core that extend ContextAwarePluginBase, and in contrib there's something in CTools and probably Rules. (The class hierarchy on api.d.org seems to have a lot of false positives: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Plug...)

There are more things that implement ConfigurablePluginInterface, and certainly in my work with plugins so far, configurable plugins are much more prominent than context aware plugins.

Could we have the best of both worlds, and create a trait, and also a base class which imports the trait and adds the constructor?

Maybe coordinate that trait which is supper useful to remove boilerplate code
with #1764380: Merge PluginSettingsInterface into ConfigurableInterface - base class that will use #2579743: Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed

@andypost could you expand more on that please -- specifically, what are you suggesting should happen here? I can't make any sense of #2852463: Create a trait and base class to implement \Drupal\Component\Plugin\ConfigurableInterface, as its summary dates back from 2012 and the early development of the plugin system.

Also, any feedback on my proposal in #19? Should I drop it / leave it to a follow-up to prevent creep in this issue?

Linking #2937177: Migrate plugin base classes should implement ConfigurablePluginInterface

I don't think it makes sense to create the trait without the corresponding base class. The trait makes sense for classes that implement ConfigurablePluginBase that don't extend directly off PluginBase, but, I can see people trying to use the trait without realizing they need to override the constructor and add $this->setConfiguration($configuration) so that their default configuration is pulled in. At the very least classes that extend PluginBase and implement ConfigurablePluginInterface can make use of a base class, along with everything that extends off of them.

The more I think about it, I'd rather just have the base class over the trait. I feel like traits should be self contained and I'm not a fan of having one that requires additional constructor code to make it actually work right. Adding to the issue, trait methods are replaced, rather than overridden, so if the goal is to make sure setConfiguration merges the incoming and default configurations correctly, and a class needs to tweak the incoming configuration slightly before merging(say to add a BC layer for a deprecated config key), it can't override setConfiguration, and then call parent::setConfiguration($configuration) to have the proper merge.

So the base class is far more helpful than the trait, but due to lack of multiple inheritance, it can't always be used, and in the cases where it can't be used, the trait isn't enough on it's own, so maybe it should indicate that in its name, ConfigurablePluginDefaultTrait or something like that so it doesn't feel like it is everything you need to implement the interface correctly.

A random thought, and maybe not a good one, but if PluginBase is going to have an opinion about where ConfigurablePluginInterface is storing its configuration, maybe it could predict the method to use too. I know ConfigurablePluginInterface doesn't extend off any interface that PluginBase uses, and in that sense this makes no sense, but in reality this would avoid the need for the extra constructor work in any reasonable situation with practically no overhead and no negative effects.

Okay, more negative effects than I would have thought...

Attempting to re-roll this patch against 8.5.x and adding in a base class along with the trait.

Bah, Ignore that one, I put ConfigurablePluginBase in the wrong spot.

It would seem that at some point between 8.4 and 8.4 WorkflowTypeBase changed it's setConfiguration from NestedArray::mergeDeep to simple array addition, and trying to change that back results in a dozen failed tests.

I'm going to postpone this on #2946122: Deprecate ConfigurablePluginInterface and replace with an interface that doesn't extend DependentPluginInterface for now.

I look into this issue more later, for now I add the tag "Contributed project blocker" because this issue blocks #2937177: Migrate plugin base classes should implement ConfigurablePluginInterface, which blocks Feeds Migrate. Feeds Migrate aims to provide an UI for Migrate plugins and therefore it needs Migrate plugins that have configuration be officially declared as configurable.

So I have looked through the comments of this issue and here are my thoughts about this issue:

• The trait does not contain a $configuration property. According to comment #2, this will cause issues with PHP 5. But at the time Drupal 8.7 is released, PHP 5 is no longer supported. So I suppose that property could be added anyway?
• A base class for configurable plugins sounds like a good idea to me to avoid the need of boilerplate code noted in #17.
• I think I get why this issue is postponed on #2946122: Deprecate ConfigurablePluginInterface and replace with an interface that doesn't extend DependentPluginInterface. The base class implements the redundant method calculateDependencies(). If fixing this issue first, I think we cannot delete that method from the base class at the time that other issue lands as then contrib could potentially rely on it?
  An other reason I can think of why this issue is postponed, is that both issues touch the same classes and that doing a 'cleanup' first makes the code changes in this issue more clear. I see that if we could do it the other way around, we need to add code here that would be removed again in the other issue.
  Are there any other reasons of why this issue is postponed?
• I think that if we would postpone adding the base class to a follow-up, this issue doesn't have to wait for #2946122: Deprecate ConfigurablePluginInterface and replace with an interface that doesn't extend DependentPluginInterface to land. I'm not sure though if #2937177: Migrate plugin base classes should implement ConfigurablePluginInterface needs the base class. If so, then we would win nothing with postponing the base class to a follow-up. If #2937177: Migrate plugin base classes should implement ConfigurablePluginInterface does not need the base class per se, we have an opportunity to unblock configurable plugins in Migrate + developing an UI for these plugins in the Feeds Migrate project much sooner.

No matter what, #2937177: Migrate plugin base classes should implement ConfigurablePluginInterface is blocked on #2946122: Deprecate ConfigurablePluginInterface and replace with an interface that doesn't extend DependentPluginInterface because if we implement ConfigurablePluginInterface in migrate plugins now, we can't unimplement it later to implement ConfigurableInterface.

I've actually started to think that instead of a base class we could just adjust the component PluginBase Constructor from
$this->configuration = $configuration
to

if ($this instanceof ConfigurableInterface) {
  $this->setConfiguration($configuration)
}
else {
$this->configuration = $configuration
}

That could be done as part of this issue.

Both this and the other issue touch the same lines of the same dozen files. It makes sense to finish the other issue first and resolve the BC implications before focusing on this trait. None of this would qualify for a backport to 8.6 though, so we do have time to make sure it will be sorted before the 8.7 alpha in March.

#36, I like the type checking. Let's do it after this gets unblocked.

Issue is unblocked. Needs to add type checking in plugin base class.

First attempt against the testbot. It renamed everything, so no interdiff.

I may need a couple iterations of this, my starting point was a bit outdated..

The last few test failures stem from moving the setConfiguration() into the constructor of PluginBase. Moving it to the end of the constructor after the id and definition were set helped with most of the issues, as they had custom setConfigurations which added in the pluginId and definition into the configuration. Aggregator Plugins are an odd one, they seem to have custom getConfiguration and setConfiguration to use a config backend. There's a TODO in the code suggesting that they should be refactored to be actual config entities, which makes more sense to me than trying to highjack ConfigurableInterface for a config backend. The specific issue here is that I replaced the `$this->configuration` with $this->setConfiguration($configuration)` if the plugin is configurable, but the custom setConfiguration that the Aggregator plugins use don't actually set the $configuration member, they count on PluginBase to do it, which it no longer does. The attached patch solves the issue by setting $this->configuration directly in pluginBase, and then calling setConfiguration() afterwards, in most cases just setting $this->configuration again. It's redundant, but most closely follows what the old code path would have done, with PluginBase assigning $this->configuration, and the actual plugin overwriting it with a $this->setConfiguration() call in it's own constructor.

The 'correct' thing to do would seem to have all the aggregator plugins with custom setConfigurations() call parent::setConfiguration() to set the class member, but I can't really enforce that in a BC way. Another alternative would be to set $this->configuration in an AggregatorPluginSettingsBase constructor to fix the behavior of that one set of plugins until such time as they are refactored into config entities.

As expected, fails in 5.6 for duplicating the $configuration member in the trait. We aren't quite ready to break php 5 yet, so we will leave it out. I did some general cleanup, got rid of some patch noise, tweaked the docblocks. I think this is ready for review now.

Okay, now it's ready for review...

+++ b/core/lib/Drupal/Component/Plugin/PluginBase.php
@@ -54,6 +54,9 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
     $this->configuration = $configuration;
     $this->pluginId = $plugin_id;
     $this->pluginDefinition = $plugin_definition;
+    if ($this->isConfigurable()) {
+      $this->setConfiguration($configuration);
+    }

This seems like it should be reformulated a bit:

if ($this->isConfigurable()) {
  $this->setConfiguration($configuration);
}
else {
  $this->configuration = $configuration;
}

Plugins are heavily tested in core all over the place, explicitly and implicitly, configurable and non-configurable. If this trait were breaking something, I'm sure we'd know about it. For this reason, I'm not sure this patch needs explicit tests. But, I'm tagging in the framework managers to either confirm or disconfirm that idea.

Patch review

+++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
@@ -78,7 +78,7 @@ protected function setUp() {
-    $this->assertSame($visibility, $block->getVisibility());
+    $this->assertEquals($visibility, $block->getVisibility());

I'd just as soon change the expected array to have the correct new order than switch to the weaker assertion. The placement of negate changes when switching from += to NestedArray::mergeDeep. But keeping the strict assertion can help catch type errors later.

I dislike leaving off protected $configuration; but I know why we have to. Can you file a follow-up to add it once we drop PHP5, and put that link as an @todo on the trait?

#52
See some of the earlier fails in #44/#45 and the explanation given in #48.
TL;DR is that aggregator is bad, see also #14.

#53
I agree here but I'm not a FM. Any test of this trait would be a test of the concept of traits as well as of the functionality of NestedArray.

I was going to address a few comments on that one, and I remembered that I wrote a whole unit test against the version of the trait that we were going to make specific to the migrate module, so I thought I’d refactor it and use it. When I tried I realized that in the migrate issue we were using NestedArray::mergeDeepArray and in the current core issue we are using NestedArray::mergeDeep. The two handle indexed keys differently, and the notes on migrate say that we wanted mergeDeepArray because we sometimes use integer keys. The notes from @timplunkett (he/him) on the core issue note that using mergeDeepArray affects ordering on one test only, and the ordering doesn’t matter in that case. ’m wondering if we should be using mergeDeepArray in the Core trait, or leave it as mergeDeep and override it in migrate plugin base classes to mergeDeepArray if we need to.

I have this feel/thought I've seen it important in non Migrate cases. But those don't come to mind at this moment.

I've addressed the feedback. I added a comment in PluginBase constructor explaining why we set $configuration twice. I added a followup and @todo for adding the $configuration member variable. I switched to mergeDeepArray, this may require some additional test adjustments, but hopefully not. I added an explicit test class, testing the merge strategy. I fixed the order of the block test expectations so we can keep the strict test.

doh.. posted the patch twice and not the diff....

No testbot issues with the switch to mergeDeepArray, so that is good, After discussing with @heddn and @tim.plunkett in slack, I believe that's the correct choice.

This should be ready for Framework manager review.

Looks great!

1. +++ b/core/lib/Drupal/Component/Plugin/ConfigurableTrait.php
   @@ -0,0 +1,59 @@
   + * @todo add protected $configuration member when PHP5 is no longer supported.
   + *
   + * @see https://www.drupal.org/project/drupal/issues/3029004
   

   I think these need to be combined.

2. +++ b/core/lib/Drupal/Component/Plugin/ConfigurableTrait.php
   @@ -0,0 +1,59 @@
   +   * @see \Drupal\Component\Plugin\ConfigurableInterface::getConfiguration()
   ...
   +  public function defaultConfiguration() {
   

   Bad copy/paste

3. +++ b/core/lib/Drupal/Component/Plugin/PluginBase.php
   @@ -54,6 +54,14 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
        $this->configuration = $configuration;
   ...
   +    // We may be resetting the $configuration member that we just set above,
   +    // but it's necessary to set this twice to preserve previous behavior, as
   +    // certain plugins use setConfiguration() to set external configuration
   +    // storage but still expect PluginBase to set the $configuration member
   +    // directly.
   +    if ($this->isConfigurable()) {
   +      $this->setConfiguration($configuration);
   +    }
   

   Is there any way you can think of to test for the reliance on this double setting, and to deprecate reliance on it?

4. +++ b/core/tests/Drupal/Tests/Component/Plugin/ConfigurableTraitTest.php
   @@ -0,0 +1,191 @@
   +class ConfigurableTraitTest extends TestCase {
   

   As I cautioned in #54, this test looks very much like a test of NestedArray's logic.

   Also it is not split up into typical unit test methods (one per method tested) with @covers and whatnot.

   I'd recommend removing it entirely, but I defer to a framework manager.

#1 I combined the @todo and @see. I've always been a bit confused with @see, in The coding standards it says to group the tags, and leave a space between the groups, but it seems like @sees need to be sprinkled through, next to whatever they are providing documentation for. Of course, I had them backwards by that standard anyway..

#2 fixed

#3 I can't think of any way to check for the behavior. Plugins might do something funky in setConfiguration and not even need to have $configuration set. The only place I saw the issue in core is the Aggregator plugins. We could certainly reset PluginBase to an if/else and put the $configuration assignment in the aggregator plugin base class constructor without apology. The problem would then go away if/when aggregator plugins get refactored into config entities. Like I mentioned, not setting `$this->configuration = $configuration` in the PluginBase constructor for configurable plugins is a change in behavior, it's just tough to tell what out their in the wild might depend on it.

#4 I kept the test class for now, but refactored it a bit to better align with unit testing. We can take it out if the FMs think it's redundant.

#1 I meant like this

diff --git a/core/lib/Drupal/Component/Plugin/ConfigurableTrait.php b/core/lib/Drupal/Component/Plugin/ConfigurableTrait.php
index 30122b1780..a156dd12cb 100644
--- a/core/lib/Drupal/Component/Plugin/ConfigurableTrait.php
+++ b/core/lib/Drupal/Component/Plugin/ConfigurableTrait.php
@@ -14,8 +14,8 @@
  *
  * @ingroup Plugin
  *
- * @todo add protected $configuration member when PHP5 is no longer supported.
- * @see https://www.drupal.org/project/drupal/issues/3029004
+ * @todo Add protected $configuration property when PHP 5 is no longer
+ *   supported. See https://www.drupal.org/project/drupal/issues/3029004.
  */
 trait ConfigurableTrait {
 

#3 fair enough. I saw the test fails for aggregator but from a quick glance can't see why that broke it...

#4 this looks much better, if still redundant

Specifically the test breaks because of the form in the test processor:

    $form['processors'][$info['id']]['dummy_length'] = [
      '#title' => t('Dummy length setting'),
      '#type' => 'number',
      '#min' => 1,
      '#max' => 1000,
      '#default_value' => $this->configuration['items']['dummy_length'],
    ];

It's wrong, the plugin should be accessing the default value through ->getConfiguration(), as $this->configuration is not how the plugin stores it's configuration, but it worked as long as PluginBase was setting $this->configuration in the constructor. When it stopped doing so, the test broke. I could easily fix the test by fixing the bug in the test processor, but what I don't know is what else in contribland might break by changing that behavior.

I moved the comment added in #57/#58 to be an @todo pointing at #3029077: Deprecate legacy handling of configuration in plugins that override getConfiguration/setConfiguration as a follow-up to #61.3.
This way when we fix the double setting of configuration, we already have the if/else in place. It also helps for the @todo to be as clear as possible about which code it refers to.

That's gonna fail, I mixed in some code from the follow-up. Here's the real patch.

Looks like a solid solution. I didn't see anything to complain about. LGTM. I'm assuming we can have the FM review at time of commit. This is for #53: should there be explicit testing coverage. There is.

I had 30 minutes to do a patch review. This is what I found so far:

1. +++ b/core/modules/image/src/ImageEffectBase.php
   @@ -19,6 +20,7 @@
   +  use ConfigurableTrait;
   

   Nitpick: newline missing after "use ConfigurableTrait".

2. +++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
   @@ -18,6 +19,7 @@
   @@ -25,14 +27,6 @@ abstract class WorkflowTypeBase extends PluginBase implements WorkflowTypeInterf
   

   WorkflowTypeBase still has its own setConfiguration() method that doesn't use NestedArray:mergeDeepArray(), but still $configuration + $this->defaultConfiguration();.
   I think setConfiguration() should be removed from WorkflowTypeBase.

3. +++ b/core/tests/Drupal/Tests/Core/Plugin/Fixtures/TestConfigurablePlugin.php
   @@ -5,29 +5,15 @@
    
   ...
    
   ...
   +  use ConfigurableTrait;
   

   Nitpick: one newline too many before "use ConfigurableTrait;".

4. +++ b/core/lib/Drupal/Core/Condition/ConditionPluginBase.php
   @@ -93,14 +86,6 @@ public function getConfiguration() {
   -  public function setConfiguration(array $configuration) {
   -    $this->configuration = $configuration + $this->defaultConfiguration();
   -    return $this;
   -  }
   
   +++ b/core/lib/Drupal/Core/Display/VariantBase.php
   @@ -74,14 +67,6 @@ public function getConfiguration() {
   -  public function setConfiguration(array $configuration) {
   -    $this->configuration = $configuration + $this->defaultConfiguration();
   -    return $this;
   -  }
   

   Some classes returned $this for the method setConfiguration(). The trait does not. Is this considered an API change? Should the trait just return $this for setConfiguration()?

Should the trait just return $this for setConfiguration()?

I would be in favor of this. For plugins that returned void from setConfiguration(), there is effectively no change, since it's unlikely they'll be assigning the return value to a variable. For plugins which used a fluent version, setConfiguration() will continue to work as before.

Agreed on all counts.

RE: workflowbase, see #31

I don't mind returning $this, but it doesn't seem necessary. Fluent setters make sense when you are setting a single key, so that you can chain them, (i.e. $object->set('key1', 'value')->set('key2', 'value2)) When set configuration sets the entire configuration array at once, chaining doesn't really make any sense. Regardless, ConfigurableInterface specifies no return value, we can't really add a return value to the interface without breaking BC, and I do worry we will run into trouble as we continue towards strict typing in php if we add a return value to the trait that doesn't exist in the interface.

Restored the custom setConfiguration for workflows.

I don't see any test of NestedArray::mergeDeepArray([$this->defaultConfiguration(), $configuration], TRUE);. Could we add a scenario?

Do we still need a framework manager review of this as well? Or did we get that review earlier with the last issue that added all these interfaces?

Had a chat with Mike at midcamp. Seems like I missed the test for mergeDeepArray. So, ignore me. And the FW manager review can happen during commit. So on to RTBC.

I was looking at this, too, and did not notice that @heddn and @mikelutz just updated it. In case it is useful for other readers, here are my notes on the point raised in #77:

The difference between NestedArray::mergeDeepArray([$this->defaultConfiguration(), $configuration], TRUE) and NestedArray::mergeDeep($this->defaultConfiguration(), $configuration) is how they handle integer array keys. Looking at the previous discussion, that seems to be the point that needs testing.

Looking at ConfigurableTraitTest.php, in setConfigurationDataProvider(), I see

      'indexed_override' => [
        'default_configuration' => [
          'config_value_1',
          'config_value_2',
          'config_value_3',
        ],
        'test_configuration' => [
          'override_value_1',
          'override_value_2',
        ],
        'final_configuration' => [
          'override_value_1',
          'override_value_2',
          'config_value_3',
        ],
      ],
      'indexed_override_complex' => [
        'default_configuration' => [
          'config_value_1',
          'config_value_2',
          'config_value_3',
        ],
        'test_configuration' => [
          0 => 'override_value_1',
          2 => 'override_value_3',
        ],
        'final_configuration' => [
          'override_value_1',
          'config_value_2',
          'override_value_3',
        ],
      ],

That looks to me like two tests of merging with integer array keys.

Just to be clear, this method is the data provider for setConfiguration(), which is where NestedArray::mergeDeepArray() is used.

Heads up, this issue broke Commerce: #3053575: [Drupal 8.8] Error: Call to a member function getSteps() on null in CheckoutPaneBase because setConfiguration() is now invoked before the plugin constructor has done its job.
Any plugin that uses a service in defaultConfiguration() will crash.

+1 to reverting and creating ConfigurablePluginBase for classes to extend that can. If you need to extend another PluginBase, or if you have an odd dependency in defaultConfiguration that requires you to place your setConfiguration() call in a specific place in your constructor, then you can implement the trait and call setConfiguration() manually in your constructor without worrying about PluginBase doing it before you are ready.

Except we already have ContextAwarePluginBase, and no equivalent trait. Can't use two base classes.

Going to be tricky.

Is this stalled out because it is tricky?

It stalled because I got busy with other things. I can rewrite it at mwds sprints this weekend.

Here we go.

I missed a file, plus fixing some use statements.

This should solve @bojanz issue. We won't change the constructor for PluginBase. The solution in contrib with the previous patch would be to set the service members before calling parent::construct, and I still recommend switching to the new base class and doing that for CheckoutPaneBase at some point, but now it won't break before you choose to make that change.

So now you can extend ConfigurablePluginBase, and you don't need to worry about the constructor, or you can just use the trait, in which case, you are responsible for setting the configuration in your constructor in a manner of your choosing.

No additional tests, as there was no additional logic written, we've just moved things around. The previous tests were updated to test the new logic locations.

This ended up being pretty trivial, just redundant. One small question. Leaving in NR for it. Otherwise, this looks ready to go if the testbot agrees.

1. +++ b/core/lib/Drupal/Component/Plugin/ConfigurableTrait.php
   @@ -0,0 +1,61 @@
   + * @todo Add protected $configuration property when PHP 5 is no longer
   + *   supported. See https://www.drupal.org/project/drupal/issues/3029004.
   

   Depending on when this lands, this could be done now, right?

2. +++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
   @@ -131,10 +131,10 @@ public function testBlockMigration() {
   +        'negate' => FALSE,
   

   Why was this needed? Remind me. Right #54.

We certainly can do that now.. This originally went into 8.8 before 8.7.0 was released, and we were waiting for the full 8.7.0 release before committing anything incompatible with php5. We are passed that point now.

yes, #54

Also added a sentence to document why you might want to use the base class vs the trait.

Also fixed the test correctly, missed that that file was a component test, lol.

Seems like this needs a reroll.

That sounds like an excellent novice task :)

:-( I dropped the ball on this one and totally forgot about it. I wish I had pushed to get it into 8.8.

Added a reroll.

Fixed the PHP error.

Lots of failures in that last run:

1) Drupal\Tests\simpletest\Functional\SimpletestUiTest::testTestingThroughUI
PHPUnit\Framework\Exception: Fatal error: Trait 'Drupal\Core\Action\ConfigurableTrait' not found in /var/www/html/core/lib/Drupal/Core/Action/ConfigurableActionBase.php on line 16

Let's see if this re-roll works more better.

1. +++ b/core/lib/Drupal/Component/Plugin/ConfigurableTrait.php
   @@ -0,0 +1,65 @@
   +    $this->configuration = NestedArray::mergeDeepArray([$this->defaultConfiguration(), $configuration], TRUE);
   
   +++ b/core/lib/Drupal/Core/Action/ConfigurableActionBase.php
   @@ -22,27 +25,6 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   -    $this->configuration = $configuration + $this->defaultConfiguration();
   
   +++ b/core/lib/Drupal/Core/Condition/ConditionPluginBase.php
   @@ -93,14 +95,6 @@ public function getConfiguration() {
   -    $this->configuration = $configuration + $this->defaultConfiguration();
   
   +++ b/core/lib/Drupal/Core/Display/VariantBase.php
   @@ -74,14 +65,6 @@ public function getConfiguration() {
   -    $this->configuration = $configuration + $this->defaultConfiguration();
   

   🤔 This is a crucial change in logic. I reviewed it carefully, and AFAICT the old behavior is preserved, except for when the value for a key is an array in both the default configuration and the passed in configuration.

   As long as @Action and @Condition (etc.) plugins weren't setting arrays as values for configuration keys, there is no behavior change.

   How do we know that for certain?

2. +++ b/core/lib/Drupal/Core/Layout/LayoutDefault.php
   @@ -47,20 +39,6 @@ public function build(array $regions) {
   -  public function setConfiguration(array $configuration) {
   -    $this->configuration = NestedArray::mergeDeep($this->defaultConfiguration(), $configuration);
   -  }
   
   +++ b/core/modules/search/src/Plugin/ConfigurableSearchPluginBase.php
   @@ -26,27 +28,6 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   -    $this->configuration = NestedArray::mergeDeep($this->defaultConfiguration(), $configuration);
   

   These OTOH were already using the merging logic that is now being standardized.

This is a crucial change in logic. I reviewed it carefully, and AFAICT the old behavior is preserved, except for when the value for a key is an array in both the default configuration and the passed in configuration.

On Slack, I asked Wim Leers if I correctly understood what he meant with this:

$a = [
  'foo',
  'bar' => [
    'qux' => 'qux',
  ],
];

$b = [
  'foo',
  'bar' => [
    'baz' => 'baz',
  ],
];

$c = [
  'foo',
  'bar' => [
    'qux' => 'qux',
    'baz' => 'baz',
  ],
];

From the code example above, after the patch is applied, array $a and array $b will now merge into $c.

As long as @Action and @Condition (etc.) plugins weren't setting arrays as values for configuration keys, there is no behavior change.

How do we know that for certain?

In my opinion, $this->configuration = $configuration + $this->defaultConfiguration(); is incomplete, just because default values living in multidimensional arrays were not properly merged.
But it is indeed possible that modules in contrib are relying on the current behavior (thus that defaults are not properly merged), so there's a chance that these modules would break with this change and need to update. The question is if that's acceptible. Is a BC break acceptible for modules relying on buggy behavior? Because I think I would classify it as a bug that default configuration is not completely merged when using multidimensional arrays in it.

Can we search contrib code for action and condition plugins and inspect how many of them are using multidimensional arrays in the default configuration? Or would it be enough to just create a change record for this change?

I found one condition plugin so far in contrib that uses a multidimensional array in the default configuration, but based on the rest of the code it seems like an unfinished plugin because the configuration isn't used in evaluate().

From \Drupal\rng\Plugin\Condition\EventOperation:

/**
 * {@inheritdoc}
 */
public function defaultConfiguration() {
  return [
    'operations' => ['manage event' => TRUE],
  ] + parent::defaultConfiguration();
}

/**
 * {@inheritdoc}
 */
public function evaluate() {
  $operation = 'manage event';
  /* @var \Drupal\user\UserInterface $user */
  $user = $this->getContextValue('user');
  /* @var \Drupal\Core\Entity\EntityInterface $event */
  $event = $this->getContextValue('event');
  return $event->access($operation);
}

87 contrib projects are used on the site where I searched for @Action and @Condition.

What I found in my contrib projects is that the buggy behaviour of $this->configuration = $configuration + $this->defaultConfiguration() didn't bite me initially. But then it did and caused some very hard to diagnose issues. So I'd be in favor of "fixing" this for them.

Because the alternatives are much, much worse. Meaning in the base plugin we utilize $this->configuration = $configuration + $this->defaultConfiguration(). But that will break lots of people, including core itself. Alternatively we don't add that to the base plugin at all and basically gut this entire issue's purpose. I don't advocate either of these, because it means a really bad developer experience or broken code to solve a questionably buggy implementation for some non-likely scenarios.

I concur with @heddn and @MegaChriz to implement the following:

$this->configuration = NestedArray::mergeDeep($this->defaultConfiguration(), $configuration);

In doing so this lines up nicely with other plugin implementations in core (e.g. views etc...) and will reduce inconsistent behavior with merging config going forward. While this could break potential configurations with nested keys, as @MegaChriz pointed out, the number of contrib modules out there seems limited. Since we are looking at 9.x for this patch, and the fact we are fixing bad behavior/bug, I am advocating for the solution in #106. Thanks everyone for the work on this.

It looks 9.1.x material

I guess this can be worked on now.

Hey, lets try doing this again. :-). Here's a straight up re-roll to start.

I am reviewing this issue.

I was hoping that the lines I highlighted in the MR review would be copied here along with the comments. I guess you will have to read the MR itself.

I searched core/lib/ and core/modules/ (but not core/tests/) for "setConfiguration(". There were only a few dozen matches (under 100), so I paged through them all rather than try to identify which ones might be derived from ConditionPluginBase or VariantBase.

Good news: the return value is never used (neither assigned nor chained ... I paged through them a second time because I only checked for chaining the first time through).

I think it is OK to remove the return value from setConfiguration() for these two classes. Since they both implement ConfigurableInterface, any (contrib or custom) code that relies on a return value should be updated. We should mention this in the change record.

I am setting the status to NW for the comments I made on the MR and for updates to the CR. The issue summary also needs a release-notes snippet. (Maybe not, but let's provide one and let the release managers decide.) I also suggested at least one follow-up issue, so I am adding the tag for that.

TODO: I reviewed the new test, but I have not yet reviewed the changes to the existing tests.

This is blocking #2937177: Migrate plugin base classes should implement ConfigurablePluginInterface

@benjifisher
I've commented on your review.

I think we've these tasks still to resolve:

1. Mention the interface instead of the trait in ConfigurablePluginBase.
2. In ConfigurableTrait, document that the $configuration property is also defined \Drupal\Component\Plugin\PluginBase.
3. Decide on if ConfigurableTraitTest needs adjustments.
4. Make sure that the changes in base classes for certain plugin types are mentioned in the release notes.
5. (optional) Check if setConfiguration() still needs to be overridden in WorkflowTypeBase.
6. Rebase the merge request.

I'm hiding the patches since we're dealing with a merge request now.

There has been work on the MR since I last checked, so I am marking this issue NR. I am also removing the assignment.

I've rebased the existing branch against 9.2 and made a new branch & new MR on 10.1.

> implement \Drupal\Component\Plugin\ConfigurableTrait directly

Is that the right wording? Does one 'implement a trait'? I'd have said 'use' or 'import'.

Some of the things in #123 still need doing by the looks of it, for instance:

> In ConfigurableTrait, document that the $configuration property is also defined \Drupal\Component\Plugin\PluginBase.

Swapping the Drupal Global Contribution Weekend 2023 tag with the correct one.

Add missing return statement and fix CCF.
Rebased to latest head.

#132 and #123 still need discussion.

Alright, I think I've resurrected this from the ashes. New merge request on 11.x, fixed issues in #123and #132

123.1 Mention the interface instead of the trait in ConfigurablePluginBase.

I mention both, since the base class uses the trait anyway.

123.2 In ConfigurableTrait, document that the $configuration property is also defined \Drupal\Component\Plugin\PluginBase.

done

123.3 Decide on if ConfigurableTraitTest needs adjustments.

It did, though opposite from the previous suggestions of moving all tests to getMockForTrait, as getMockForTrait is getting deprecated in phpunit, so I converted all tests to use the same concrete test class that uses the trait.

123.4 Make sure that the changes in base classes for certain plugin types are mentioned in the release notes.

I added the list to the release notes snippet and the CR

123.5 (optional) Check if setConfiguration() still needs to be overridden in WorkflowTypeBase.

It's an issue of overriding it and keeping tests, or resetting it to the standard and adjusting all the tests to handle the reordered returned array. I also removed use of the trait entirely from ConditionPluginBase for the same reasons, updating it to use mergeDeepArray broke a few tests and will require adjusting the order of items in umami config to pass tests (as they would be exported in a different order, causing a test fail) It's better to explore converting both of those pluginbases to use mergeDeepArray in a followup, if at all, where we can review the test changes and potential BC issues more thoroughly in isolation from the trait and base class work being provided here.

123.6 Rebase the merge request.

done.

132 > implement \Drupal\Component\Plugin\ConfigurableTrait directly

Is that the right wording? Does one 'implement a trait'? I'd have said 'use' or 'import'.

I agree, switched the wording to use `use`.

In addition, I added SelectionPluginBase to the plugin bases that we convert (it was added after the original MR was created here), and removed ConditionPluginBase for reasons described above.

I moved ConfigurableTrait from Component/Plugin to Core/Plugin. In the original patch, we didn't notice that having the trait in Component meant we added a undocumented dependency on Component/Utility to Component/Plugin, and we shouldn't do that. The interface can live in the Component, the trait can live in Core next to the base class (which was already there because that needed to extend Core's PluginBase and not Component's. Despite the fact that the interface lives in Component, it makes more sense to have the implementations live in Core.

I updated the Change Record (and unpublished it, as it was never unpublished after we reverted years ago.

I fixed a few coding standards/phpstan/phpunit10 issues that have changed things over the years.

I think this should be committable now, or close to it hopefully.

Opened #3469714: Determine if we can remove logger from ImageEffectBase to research if we can remove logger.

All I did was rebase (200+ commits back) and added some typehints to the test classes.

Reviewed the rest of the changes and looks cleaner. Didn't see anything off so lets see if we can land this old one.

+1 on RTBC

I read the IS, the comments and the MR. There are suggestions in the MR for the comments and they do need someone working on this issue to review, so back to NW for that.

And a follow up is needed for this part of #145

It's better to explore converting both of those pluginbases to use mergeDeepArray in a followup, if at all, where we can review the test changes and potential BC issues more thoroughly in isolation from the trait and base class work being provided here.

I've updated the documentation with @quietone's suggestions. Setting back to review. We still need to consider if we want followups for the plugin bases that use different merge strategies or if they are fine as they are.

Feedback appears to be addressed

Opened #3520751: Research - If pluginbases from 2852463 can use mergeDeepArray for the follow up, please add more detail if you have time.

I think that this removes blocker to further work on Migrate Plus Feeds module https://www.drupal.org/project/feeds_migrate