Resource plugin manager needlessly calls wrong method to instantiate plugins

wtf?

This looks like a really big win, is ridiculously simple and also a better usage of Plugins in general.

Eclipse

This looks like a task tbh and not a bug. Also can't see why this is a big win - yes less code is nice but there is no bug in the current code it is just clunkier. Afaics this doesn't meet https://www.drupal.org/core/beta-changes. Postponing to a patch release

Duplicate of #2625554: getInstance() fatals on almost all plugin managers.

How is this a duplicate? That issue specifies that getInstance() fatals on most plugin managers... this plugin manager actually implements getInstance() needlessly. Two very different things, no?

Eclipse

I would agree. This is just a minor cleanup not related to #2625554: getInstance() fatals on almost all plugin managers.

+++ b/core/modules/rest/src/Plugin/Type/ResourcePluginManager.php
@@ -33,13 +33,4 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
-  public function getInstance(array $options){
-    if (isset($options['id'])) {
-      return $this->createInstance($options['id']);
-    }
-  }

Imo we should be marking this as deprecated for Drupal 9... since removing this now is unnecessary and will break code that currently works.

All the other changes are fine but the change outlined in #28 is not patch or minor release safe.

Restored ::getInstance() method and marked it as deprecated. Back to RTBC

+++ b/core/modules/rest/src/Plugin/Type/ResourcePluginManager.php
@@ -36,6 +36,10 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
   /**
    * {@inheritdoc}
+   *
+   * @deprecated in Drupal 8.2.0, will be removed before Drupal 9.0.0.
+   *   Use Drupal\rest\Plugin\Type\ResourcePluginManager::createInstance()
+   *   instead.
    */

I'm not sure what it means to deprecate an interface method. This is from MapperInterface which is implemented by DefaultPluginManager

If we remove the override in 9.x but don't do anything else, then it'll still have the method, just inherited from DefaultPluginManager - will that not work then?

I'm not sure what it means to deprecate an interface method. This is from MapperInterface which is implemented by DefaultPluginManager

We simply want to encourage developers to use the proper method to create an instance of resource plugins instead of relying on the unnecessary implementation of ResourcePluginManager::::getInstance().

If we remove the override in 9.x but don't do anything else, then it'll still have the method, just inherited from DefaultPluginManager - will that not work then?

it wont work, exactly like other plugin managers that not implement ::getInstance() see #2625554: getInstance() fatals on almost all plugin managers (but as said in #18 by Eclipse Gc the two issues are not a duplicate)

IOW: ResourcePluginManager mistakenly abuses \Drupal\Component\Plugin\Mapper\MapperInterface::getInstance() for the purpose that \Drupal\Component\Plugin\Factory\FactoryInterface::createInstance() exists for.

Also see #1894130: Make \Drupal\Core\Plugin\Mapper\MapperInterface optional for plugin managers, which explains:

\Drupal\Core\Plugin\MapperInterface is a general-purpose interface to get plugin 'singletons' based on arbitrary 'options'. It is by no means a proper API, and it's only used by a handful of plugin managers.

Because the $options parameter is so generic, and different plugin managers behave differently based on the array's contents, the interface is useless by definition.

+++ b/core/modules/rest/src/Plugin/Type/ResourcePluginManager.php
@@ -36,6 +36,10 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
+   * @deprecated in Drupal 8.2.0, will be removed before Drupal 9.0.0.
+   *   Use Drupal\rest\Plugin\Type\ResourcePluginManager::createInstance()
+   *   instead.

I think this needs to be more nuanced. As @catch points out this method might still exist in Drupal 9. What we're really doing here is deprecating its usage for creating new instances.

What about simply using: * @deprecated in Drupal 8.2.0
+ * Use Drupal\rest\Plugin\Type\ResourcePluginManager::createInstance()
+ * instead.


Committed/pushed to 8.2.x, thanks!

YAY, finally closing this one :)