getInstance() fatals on almost all plugin managers

IMHO we could implement a basic implementation which calls out to createInstance() if possible (when the ID is set).

See also #1894130: Make \Drupal\Core\Plugin\Mapper\MapperInterface optional for plugin managers

IMHO we could implement a basic implementation which calls out to createInstance() if possible (when the ID is set).

This is not doable. We don't know anything about the structure of $options. Writing code that pretends we know this, is just giving a bad example.

This patch implements @chx's original suggestion, and slightly improves the documentation on PluginManagerBase.

Sure, I think that's an improvement. But it's not a major bug.

+++ b/core/lib/Drupal/Component/Plugin/PluginManagerBase.php
@@ -94,7 +94,7 @@ public function createInstance($plugin_id, array $configuration = array()) {
+    throw new \Exception(sprintf('This method is not implemented. It must remain on %s for backwards compatibility. See %s.', PluginManagerInterface::class, 'https://www.drupal.org/node/1894130'));

static::class could work, but I assume you meant PluginManagerInterface::class because that's where the docs live?

Marked #2579235: Resource plugin manager needlessly calls wrong method to instantiate plugins and #2503355: Remove ResourcePluginManager::getInstance() as duplicates of this. Note that we should also remove \Drupal\rest\Plugin\Type\ResourcePluginManager::getInstance() as part of this, i.e. the changes in #2579235: Resource plugin manager needlessly calls wrong method to instantiate plugins should be included here.

Still a problem.

PluginManagerInterface::class, what about get_called_class() ?

static::class could work, but I assume you meant PluginManagerInterface::class because that's where the docs live?

I hardcoded the class name exactly for that reason. I have updated the message a little to clear up the confusion.

Marked #2579235: Resource plugin manager needlessly calls wrong method to instantiate plugins and #2503355: Remove ResourcePluginManager::getInstance() as duplicates of this. Note that we should also remove \Drupal\rest\Plugin\Type\ResourcePluginManager::getInstance() as part of this, i.e. the changes in #2579235: Resource plugin manager needlessly calls wrong method to instantiate plugins should be included here.

@Wim Leers: in #2503355-16: Remove ResourcePluginManager::getInstance() you said removing it breaks BC, which I agree with, so I think we should keep it as it is.

Should we use BadMethodCallException?

IN my world it seems that getInstance is something which would be deprecated and removed from the generic interface in D9. Conceptually getInstance() requires domain specific knowledge and just, as we see here, be implemented in a generic way. Given that, adding domain specific methods onto plugin managers seems to be the next logical step in a future design.

@dawehner: Are you suggesting we do that in follow-up issues?

I'm not sure if we should add the exception in code like that or we should add a trigger error like we usually do. This is something that's really broken.

+++ b/core/lib/Drupal/Component/Plugin/PluginManagerBase.php
@@ -89,7 +89,7 @@ public function createInstance($plugin_id, array $configuration = array()) {
   public function getInstance(array $options) {
-    return $this->mapper->getInstance($options);
+    throw new \Exception(sprintf('This method is not implemented. It must remain on %s for backwards compatibility with %s. See %s.', static::class, PluginManagerInterface::class, 'https://www.drupal.org/node/1894130'));
   }
 

Shouldn't we check the mapper first?

NW for #21 and #22.

I'm not sure if we should add the exception in code like that or we should add a trigger error like we usually do. This is something that's really broken.

Is that different from just allowing the current code to trigger a fatal error? Do we want to allow continued code execution?

This patch implements the feedback from #13 and #22. I also replaced the URL to the other issue with the URL to the change record I just created. I used the term "discouraged" since I don't think we can fully deprecate this (on account of MapperInterface being meh, but not actually deprecated), but perhaps we can improve the wording over time :)

That's a reasonable explenation, this is a lot better than the generic error that previously happened.

Re-testing since this looks like an infra failure.

The exception can be tested and then we can be sure the the helpful information of which class is pointed out. Also linking to an issue in the exception message feels odd. Is there not something better we can say / link to?

+++ b/core/tests/Drupal/Tests/Component/Plugin/PluginManagerBaseTest.php
@@ -90,4 +93,37 @@ public function testCreateInstanceFallback() {
+      ->wilLReturn($instance);

Shift key error in wilLReturn.

+++ b/core/tests/Drupal/Tests/Component/Plugin/PluginManagerBaseTest.php
@@ -90,4 +93,37 @@ public function testCreateInstanceFallback() {
+   * @expectedException \BadMethodCallException

We don't use this annotation any more.

Fixed issues raised from #31 and #32

Add missing PhpunitCompatibilityTrait from 33.

Finally dawned on me, that test errors are due to issue with the way I created the patch, where the new file with StubPluginManagerBaseWithMapper was missing in the diff, should hopefully work now.

All issues that were raised are fixed now, to RTBC this goes.

1. +++ b/core/lib/Drupal/Component/Plugin/PluginManagerBase.php
   @@ -104,6 +104,9 @@ protected function handlePluginNotFound($plugin_id, array $configuration) {
   +      throw new \BadMethodCallException(sprintf('%s does not implement this method, unless %s::$mapper is set.', static::class, static::class));
   

   Hmmm re-reading this exception message gives me pause. The method is definitely implemented. I think better language would be does not support this method

2. +++ b/core/tests/Drupal/Tests/Component/Plugin/PluginManagerBaseTest.php
   @@ -11,6 +15,8 @@
   +  use PhpunitCompatibilityTrait;
   

   Let's not use this trait in component code. Every where in component tests we do an if. This introduces a dependency on Drupal's test code that we don't want.

The remarks in #39 were fixed.

+1 Would have done this myself, just not had the time for it. This should be RTBC.

Committed and pushed 56554f5da5 to 8.7.x and e863ce259a to 8.6.x. Thanks!

Backported to 8.6.x since, whilst this does throw a new exception, it is only in the case that would have errored already and the new error is more helpful.

diff --git a/core/tests/Drupal/Tests/Component/Plugin/PluginManagerBaseTest.php b/core/tests/Drupal/Tests/Component/Plugin/PluginManagerBaseTest.php
index 3bf99abb5b..2bad780dbc 100644
--- a/core/tests/Drupal/Tests/Component/Plugin/PluginManagerBaseTest.php
+++ b/core/tests/Drupal/Tests/Component/Plugin/PluginManagerBaseTest.php
@@ -4,7 +4,6 @@
 
 use Drupal\Component\Plugin\Exception\PluginNotFoundException;
 use Drupal\Component\Plugin\Mapper\MapperInterface;
-use Drupal\Component\Plugin\PluginInspectionInterface;
 use Drupal\Component\Plugin\PluginManagerBase;
 use PHPUnit\Framework\TestCase;
 

Fixed unused use on commit.

publish change record