DefaultSingleLazyPluginCollection retains stale instance IDs

1. +++ b/core/lib/Drupal/Core/Plugin/DefaultSingleLazyPluginCollection.php
   @@ -52,10 +52,7 @@ class DefaultSingleLazyPluginCollection extends LazyPluginCollection {
      public function __construct(PluginManagerInterface $manager, $instance_id, array $configuration) {
        $this->manager = $manager;
   -    $this->instanceId = $instance_id;
   -    // This is still needed by the parent LazyPluginCollection class.
   -    $this->instanceIDs = array($instance_id => $instance_id);
   -    $this->configuration = $configuration;
   +    $this->addInstanceId($instance_id, $configuration);
      }
   

   It is always nice to not bypass APIs in constructors. The only tricky bit is that this might cause BC problems. Imagine the following: A subclass having a constructor with one additional dependency. This dependency is used in the addInstanceId method. Now this new call leads to a call to NULL

2. +++ b/core/tests/Drupal/Tests/Core/Plugin/DefaultSingleLazyPluginCollectionTest.php
   @@ -58,6 +58,17 @@ public function testAddInstanceId() {
   +  /**
   +   * @covers ::getInstanceIds
   +   */
   +  public function testGetInstanceIds() {
   +    $this->setupPluginCollection($this->any());
   +    $this->assertEquals(['apple' => 'apple'], $this->defaultPluginCollection->getInstanceIds());
   +
   +    $this->defaultPluginCollection->addInstanceId('banana', ['id' => 'banana', 'key' => 'other_value']);
   +    $this->assertEquals(['banana' => 'banana'], $this->defaultPluginCollection->getInstanceIds());
   +  }
   

   Nice and good to understand test coverage.

New test made the review easier. Is it worth it to add test only patch?

+++ b/core/modules/search/src/Plugin/ConfigurableSearchPluginBase.php
@@ -23,7 +23,7 @@
   public function __construct(array $configuration, $plugin_id, $plugin_definition) {
     parent::__construct($configuration, $plugin_id, $plugin_definition);
 
-    $this->configuration = NestedArray::mergeDeep($this->defaultConfiguration(), $this->configuration);
+    $this->setConfiguration($configuration);
   }
 
   /**
@@ -44,7 +44,7 @@ public function getConfiguration() {

@@ -44,7 +44,7 @@ public function getConfiguration() {
    * {@inheritdoc}
    */
   public function setConfiguration(array $configuration) {
-    $this->configuration = $configuration;
+    $this->configuration = NestedArray::mergeDeep($this->defaultConfiguration(), $configuration);
   }

What impacts will this have on contrib?

Discussed about the implications for contrib with @tim.plunkett in iRC. I think we need a change record to inform contrib about the need to make changes like:

+++ b/core/lib/Drupal/Core/Action/ConfigurableActionBase.php
@@ -17,7 +17,7 @@
   public function __construct(array $configuration, $plugin_id, $plugin_definition) {
     parent::__construct($configuration, $plugin_id, $plugin_definition);
 
-    $this->configuration += $this->defaultConfiguration();
+    $this->setConfiguration($configuration);
   }
 
   /**
@@ -38,7 +38,7 @@ public function getConfiguration() {

@@ -38,7 +38,7 @@ public function getConfiguration() {
    * {@inheritdoc}
    */
   public function setConfiguration(array $configuration) {
-    $this->configuration = $configuration;
+    $this->configuration = $configuration + $this->defaultConfiguration();
   }

If we get this early into 8.4.x then contrib has plenty of time to apply and these changes will work for 8.3.x we should be all good. According to @tim.plunkett if they don;t make the changes then the worst that will occur is some PHP notices but the site should not be broken.

I'm wondering whether we should have a protected function called setConfigurationNested and allow contrib modules to use it in their setConfiguration implementation. This would then allow all contrib modules to have the new method already and then we could maybe switch over at some point.

#17 is answered in #18 so back to RTBC

Can we get a followup to investigate adding a trait for \Drupal\Component\Plugin\ConfigurablePluginInterface and for all core base plugin classes to use it where possible. The differences between ConfigurableSearchPluginBase and others confuse me and probably others.

Once the followup exists this looks ready to commit to 8.4.x.

Created #2852463: Create a trait and base class to implement \Drupal\Component\Plugin\ConfigurableInterface

Committed 037932d and pushed to 8.4.x. Thanks!

The fact that DefaultLazyPluginCollection is no longer lazy broke Commerce hard.
Caught and fixed (need to start testing against future branches), but it feels like the core BC process was unusually lax here.

This broke Entityqueue as well, it needed the same fix as Commerce: #2903149: Fatal error when updating to Drupal 8.4