DefaultSingleLazyPluginCollection should not attempt to instantiate a NULL instance ID

Ran into this while working on #2831274: Bring Media entity module to core as Media module (see my comment #305 on that issue for more info).

Patch looks good to me, but would be great if someone that is more familiar with the plugin system would check it too.

This makes sense for me.

In DefaultSingleLazyPluginCollection, the PHPDoc for $instanceId; is @var string, and for the $instance_id parameter in __construct() is @param string $instance_id. So I think if we want to allow for NULL, we should update those docs. Alternatively, maybe we should throw an exception in __construct() and/or addInstanceId() if it's NULL?

Looks good to me, except for:

+++ b/core/lib/Drupal/Core/Plugin/DefaultSingleLazyPluginCollection.php
@@ -31,7 +31,7 @@ class DefaultSingleLazyPluginCollection extends LazyPluginCollection {
-  protected $configuration;
+  protected $configuration = [];

This doesn't look like a related change.

Thanks for the explanation!

In which case, I'd say this is ready.

+++ b/core/lib/Drupal/Core/Plugin/DefaultSingleLazyPluginCollection.php
@@ -52,7 +52,12 @@ class DefaultSingleLazyPluginCollection extends LazyPluginCollection {
-    $this->addInstanceId($instance_id, $configuration);
+
+    // @todo Replace this check with a deprecation error:
+    //   https://www.drupal.org/node/2575081.
+    if ($instance_id !== NULL) {
+      $this->addInstanceId($instance_id, $configuration);
+    }

I think given that we're planning to put a deprecation notice here we should consider doing that in this issue. Because with this change now addInstance is not called on every instantiation. In core this doesn't cause a problem because we only we really on it to initialise $configuration but the same might not be true in contrib. Without the @trigger_error they might not know that something has changed. Well, even with this is possible but at least we're being helpful.

Also since this is a change in behaviour in a class that is an extension point we should have a CR.

Rerolled and updated deprecation message.

Ignored #27

Also took a shot at the change record. So removing that tag.

Thanks for reviving this one.

+++ b/core/lib/Drupal/Core/Plugin/DefaultSingleLazyPluginCollection.php
@@ -52,7 +52,13 @@ class DefaultSingleLazyPluginCollection extends LazyPluginCollection {
+      @trigger_error(sprintf('Instantiating %s with a NULL instance ID is deprecated in Drupal 10.1.0 and support will be removed before Drupal 11.0.0.', __CLASS__), E_USER_DEPRECATED);

Deprecation messages need to match the standard format: "... is deprecated in drupal:10.1.0 and is removed from drupal:11.1.0. See https://www.drupal.org/node/3302915"

Updated based on #30

1. +++ b/core/lib/Drupal/Core/Plugin/DefaultSingleLazyPluginCollection.php
   @@ -52,7 +52,13 @@ class DefaultSingleLazyPluginCollection extends LazyPluginCollection {
   +      @trigger_error(sprintf('Instantiating %s with a NULL instance ID is deprecated in Drupal 10.1.0 and is removed from drupal:11.1.0. See https://www.drupal.org/node/3302915', __CLASS__), E_USER_DEPRECATED);
   

   it should be `drupal:10.1.0` and `drupal:11.0.0`

   wondered why sniffers skipped it

2. +++ b/core/modules/block/tests/src/Kernel/BlockStorageUnitTest.php
   @@ -2,11 +2,11 @@
   +use Drupal\Component\Plugin\Exception\PluginException;
   ...
   -use Drupal\Component\Plugin\Exception\PluginException;
   

   unrelated change, please remove

Updated based on feedback in #33

> wondered why sniffers skipped it

Because of sprintf() call - only exact strings are checked, too many false positives with dynamic strings.

The last change contains a lot of theme related things as well, is that on purpose? Change request looks good.

Yeah the theme related changes look like they come from a Bartik removal patch, out of scope here. NW to undo those.

Sorry about that.

+++ b/core/lib/Drupal/Core/Plugin/DefaultSingleLazyPluginCollection.php
@@ -52,7 +52,13 @@ class DefaultSingleLazyPluginCollection extends LazyPluginCollection {
+      @trigger_error(sprintf('Instantiating %s with a NULL instance ID is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. See https://www.drupal.org/node/3302915', __CLASS__), E_USER_DEPRECATED);

please replace "sprintf" to allow sniffers to catch format

+++ b/core/lib/Drupal/Core/Plugin/DefaultSingleLazyPluginCollection.php
@@ -52,7 +52,13 @@ class DefaultSingleLazyPluginCollection extends LazyPluginCollection {
+      @trigger_error(sprintf('Instantiating %s with a NULL instance ID is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. See https://www.drupal.org/node/3302915', __CLASS__), E_USER_DEPRECATED);

Do we need to do a trigger_error() here, or could we throw an exception?

IIRC, at the moment, if $instance_id is NULL it crashes.

So if we throw an exception here, we're changing people's code from one crash to another.

Updated patch based on #39

++ to #40, changed wording and using exactly a tested class

Filed child to remove deprecation in 11.x #3303176: DefaultSingleLazyPluginCollection throw exception to instantiate a NULL instance ID

IIRC, at the moment, if $instance_id is NULL it crashes.

would be great to get a real example (maybe contrib) to prove it, otherwise follow-up is #2579743: Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed

I don't about contrib, but I saw this in a project last week where a mistake in config caused DefaultSingleLazyPluginCollectionTest to be used with a NULL.

To reproduce in a test, in DefaultSingleLazyPluginCollectionTest::setupPluginCollection(), just add:

    $this->defaultPluginCollection = new DefaultSingleLazyPluginCollection($this->pluginManager, NULL, ['id' => 'apple', 'key' => 'value']);

run the test and it fails with:

Undefined array key ""

/Users/joachim/Sites/drupal-core-composer/repos/drupal/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php:163
/Users/joachim/Sites/drupal-core-composer/repos/drupal/core/tests/Drupal/Tests/Core/Plugin/DefaultSingleLazyPluginCollectionTest.php:28
/Users/joachim/Sites/drupal-core-composer/vendor/phpunit/phpunit/src/Framework/MockObject/Stub/ReturnCallback.php:33
/Users/joachim/Sites/drupal-core-composer/vendor/phpunit/phpunit/src/Framework/MockObject/Matcher.php:153
/Users/joachim/Sites/drupal-core-composer/vendor/phpunit/phpunit/src/Framework/MockObject/InvocationHandler.php:122
/Users/joachim/Sites/drupal-core-composer/repos/drupal/core/lib/Drupal/Core/Plugin/DefaultSingleLazyPluginCollection.php:62
/Users/joachim/Sites/drupal-core-composer/repos/drupal/core/lib/Drupal/Component/Plugin/LazyPluginCollection.php:80
/Users/joachim/Sites/drupal-core-composer/repos/drupal/core/lib/Drupal/Core/Plugin/DefaultSingleLazyPluginCollection.php:83
/Users/joachim/Sites/drupal-core-composer/repos/drupal/core/lib/Drupal/Core/Plugin/DefaultSingleLazyPluginCollection.php:99
/Users/joachim/Sites/drupal-core-composer/repos/drupal/core/lib/Drupal/Core/Plugin/DefaultSingleLazyPluginCollection.php:55
/Users/joachim/Sites/drupal-core-composer/repos/drupal/core/tests/Drupal/Tests/Core/Plugin/DefaultSingleLazyPluginCollectionTest.php:31
/Users/joachim/Sites/drupal-core-composer/repos/drupal/core/tests/Drupal/Tests/Core/Plugin/DefaultSingleLazyPluginCollectionTest.php:41

The line of the test which makes it fail is this in the anonymous class:

    $this->configuration = $configuration + $this->defaultConfiguration();

as bug

Change record is simple and makes sense.

Code has coverage.

Think this is good.

+++ b/core/lib/Drupal/Core/Plugin/DefaultSingleLazyPluginCollection.php
@@ -52,7 +52,13 @@ class DefaultSingleLazyPluginCollection extends LazyPluginCollection {
+
+    if ($instance_id === NULL) {
+      @trigger_error('Instantiating %s with a NULL instance ID is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. See https://www.drupal.org/node/3302915', E_USER_DEPRECATED);
+    }
+    else {
+      $this->addInstanceId($instance_id, $configuration);
+    }

A bit nitpicky, but it won't be removed in Drupal 11, it's either going to be a type error if we explicitly type hint with string, or it'll throw an exception. Can we maybe say 'and must be a string in Drupal 11' instead?

Changed according to the suggestion given in #49

Fixed PHP CBF errors from the earlier patch.

Fixed PHP CBF errors from the earlier patch.

Trying to Fix CCF errors.

clean-up unrelated changes

Fix remaining message

Think I'm able to review this as the patch has gone through several changes since I last worked on it 7 months ago.

Deprecation looks good, with change record
Tests look good and fail as expected without the fix.

Think this is good.

This is still a problem.

Also, I just ran into a sibling issue: #3404023: DefaultSingleLazyPluginCollection::setConfiguration() accepts NULL but ConfigurableInterface::setConfiguration() does not.