Provide a standard way to check a plugin is an instance of the required interface

Here is a start.

I think this is a great idea.

I expanded this fully, which required adding 2 interfaces. Also, I pulled the code into another method on DefaultPluginManager, so that classes that don't use interfaces can opt out.

Well let's actually address the issue summary.

While trying to debug the fails, I realized the exception wasn't enough information. Expanded that, and fixed views join.

Ahh, very interesting. This code now triggers autoloading, which reveals some interesting assumptions/bugs in taxonomy.

Well, we should make it optional to check the interface.

This time done properly.

Okay, I agree it should be opt-in.

Oh wow!

1. +++ b/core/modules/system/src/Tests/Plugin/Discovery/AnnotatedClassDiscoveryTest.php
   @@ -43,6 +43,13 @@ protected function setUp() {
   +        'class' => 'Drupal\plugin_test\Plugin\plugin_test\fruit\Kale',
   

   I'm pretty sure Kale is a vegetable...

2. +++ b/core/modules/taxonomy/taxonomy.module
   @@ -689,8 +689,21 @@ function taxonomy_implode_tags($tags, $vid = NULL) {
   +  if (isset($info['options_select'])) {
   +    $info['options_select']['field_types'][] = 'taxonomy_term_reference';
   +  }
   +  if (isset($info['options_buttons'])) {
   +    $info['options_buttons']['field_types'][] = 'taxonomy_term_reference';
   +  }
   +}
   +
   +/**
   + * Implements hook_field_formatter_info_alter().
   + */
   +function taxonomy_field_formatter_info_alter(array &$info) {
   +  if (!\Drupal::moduleHandler()->moduleExists('entity_reference')) {
   +    unset($info['entity_reference_rss_category']);
   +  }
   

   Is this related to this patch? or did it sneak in from elsewhere?

In general, this looks pretty good. My one big question here is actually around adding this functionality to the DefaultPluginManager in Drupal\Core. Two points:

1. I think this could have easily been added at the Component layer. If we feel like that's not worth while I totally understand, but it's worth mentioning.
2. We are currently checking definitions during discovery. I see this is done as the last step of findDefinitions(), so it's pretty safe, but I had always envisioned an actual check in the createInstance(). Either way, I just wanted to get this off my chest. This still looks pretty safe for what we intend.

I guess the last couple things are just the aggregator's plugin manager looks completely weird compared to all the other here. I dunno why fetchers/parsers/processors are all dealt with in a single manager when they're doing such different things. I kind of thought we'd end up passing an interface through a constructor variable like we do directory and annotation class. Is there a reason we chose to hard code it on each manager instead of injecting it?

Eclipse

I'm pretty sure Kale is a vegetable...

Fixed.

Is this related to this patch? or did it sneak in from elsewhere?

See #2318281-7: Provide a standard way to check a plugin is an instance of the required interface

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
@@ -237,8 +246,37 @@ protected function findDefinitions() {
+    throw new PluginException(String::format('Plugin "@plugin_id" (@class) in @provider should implement interface @interface', array(
+      '@plugin_id' => $plugin_id,
+      '@class' => $plugin_definition['class'],
+      '@provider' => $plugin_definition['provider'],
+      '@interface' => is_array($this->interface) ? implode(', ', $this->interface) : $this->interface,
+    )));

We can probably follow up on this because we don't really do this anywhere else but I feel like we should have an explicit exception. Exceptions in the plugin system have always been a weak point.

+++ b/core/modules/aggregator/src/Plugin/AggregatorPluginManager.php
@@ -26,6 +26,15 @@
+   * {@inheritdoc}
+   */
+  protected $interface = array(
+    'Drupal\aggregator\Plugin\FetcherInterface',
+    'Drupal\aggregator\Plugin\ParserInterface',
+    'Drupal\aggregator\Plugin\ProcessorInterface',
+  );
+
+  /**

+++ b/core/modules/migrate/src/Plugin/MigratePluginManager.php
@@ -30,6 +30,17 @@
+   * {@inheritdoc}
+   */
+  protected $interface = array(
+    'Drupal\migrate\Plugin\MigrateDestinationInterface',
+    'Drupal\migrate\Plugin\MigrateProcessInterface',
+    'Drupal\migrate\Plugin\MigrateSourceInterface',
+    'Drupal\migrate\Plugin\MigrateIdMapInterface',
+    'Drupal\migrate\Plugin\MigrateEntityDestinationFieldInterface',
+  );
+
+  /**

Maybe this is exposing a problem with the implementing systems but this smells. An array for a singular variable? Multiple "valid" interfaces?

re #16
Code comments:
1) I _think_ that's the point. It doesn't implement the fruit interface.
2) In #7 Tim mentions some "interesting assumptions/bugs in taxonomy" and added this but I don't understand what this is fixing either.

Other comments:
1) We can follow up and discuss.
2) I'm actually a little uncomfortable with this. We explicitly go to lengths to not load code during definition discovery. While this doesn't load code outside of the specific plugin system, its a regression. On the other hand, allowing a user to select a plugin that is actually invalid is bad. On the other hand, this magic makes for a bit of a dev hurdle in "why doesn't my plugin show up?"

We originally didn't include this because of this autoload requirement and I'm curious what the use case is that discovery is going to find plugins that aren't actually plugins? We came to the conclusion originally that this wouldn't be something we would see.

Ha! Yes, kale is a vegetable. Hence it doesn't implement FruitInterface!

I'm worried about the autoloading too, but not sure if we should continue here.

Ha! Yes, kale is a vegetable. Hence it doesn't implement FruitInterface!

Oh right! Just ignore my previous patch then.

I'm worried about the autoloading too, but not sure if we should continue here.

Well, we could at least move this functionality onto the factory, which still should be okay, don't you think so?

Yes, I do think that's ok. It'll throw a nice big fat error and the dev will go "Why doesn't this implement that interface?... better fix it."

I know it's not perfect from a UI perspective because technically people COULD implement a plugin that doesn't implement the interface and it could be selected to be run and and and, but re: discovery, I've always used createInstance() as my example for how to enforce this. I'd be a lot more comfortable with that, and I also still think we should pass the interface through the constructor so that we have a 1 to 1 relationship of service ids to interface expectations. Unless we have a real legitimate use for a single service dealing with multiple interfaces. (Note: I say service to denote the difference between a manager that services multiple types under different service ids vs one that COULD service multiple from a single id... which I hope we don't do)

Eclipse

I'll look into both of the last suggestions.

I agree with both suggestions in #22. I also agree with #16.2.1: why not move all of the DefaultPluginManager additions to PluginManagerBase instead? #17 says to punt that to a followup, but I don't see why.

xpost

Something like this?

Ahem.

Stupid mistake.

Didn't retesting used to mark back to needs review? Oh well, passes now.

1. +++ b/core/lib/Drupal/Component/Plugin/PluginManagerBase.php
   @@ -55,6 +76,7 @@ public function getDefinitions() {
   +    $this->enforcePluginInterface($plugin_id);
        return $this->factory->createInstance($plugin_id, $configuration);
   

   Here is a question: why is that not part of the factory? Just curious. Technically, both obviously work.

2. +++ b/core/lib/Drupal/Component/Plugin/PluginManagerBase.php
   @@ -65,4 +87,24 @@ public function getInstance(array $options) {
   +    if ($this->interface && !is_subclass_of($plugin_definition['class'], $this->interface)) {
   

   I am fine if you want to kill me: did you considered to not introduce the views handler interface but use the base class instead? Technically is_subclass_of covers both
   and we could actually have a proper discussion about the interface in contrast to just sneaking it in.

3. +++ b/core/lib/Drupal/Component/Plugin/PluginManagerBase.php
   @@ -65,4 +87,24 @@ public function getInstance(array $options) {
   +      throw new PluginException(String::format('Plugin "@plugin_id" (@class) in @provider should implement interface @interface', array(
   

   I am not a native speaker, but at least the why I understand "should" is optional. So maybe replace it with "has to".

4. +++ b/core/lib/Drupal/Component/Plugin/PluginManagerBase.php
   @@ -65,4 +87,24 @@ public function getInstance(array $options) {
   +        '@interface' => is_array($this->interface) ? implode(', ', $this->interface) : $this->interface,
   

   Maybe implement with " or "?

5. +++ b/core/lib/Drupal/Core/Field/WidgetPluginManager.php
   @@ -47,7 +47,7 @@ class WidgetPluginManager extends DefaultPluginManager {
   +    parent::__construct('Plugin/Field/FieldWidget', $namespaces, $module_handler, 'Drupal\Core\Field\WidgetInterface', 'Drupal\Core\Field\Annotation\FieldWidget');
   
   +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitManager.php
   @@ -63,7 +63,7 @@ class ImageToolkitManager extends DefaultPluginManager {
   +    parent::__construct('Plugin/ImageToolkit', $namespaces, $module_handler, 'Drupal\Core\ImageToolkit\ImageToolkitInterface', 'Drupal\Core\ImageToolkit\Annotation\ImageToolkit');
   
   +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationManager.php
   @@ -46,7 +46,7 @@ class ImageToolkitOperationManager extends DefaultPluginManager implements Image
   +    parent::__construct('Plugin/ImageToolkit/Operation', $namespaces, $module_handler, 'Drupal\Core\ImageToolkit\ImageToolkitOperationInterface', 'Drupal\Core\ImageToolkit\Annotation\ImageToolkitOperation');
   
   +++ b/core/lib/Drupal/Core/Menu/ContextualLinkManager.php
   @@ -28,6 +28,11 @@
   +   * {@inheritdoc}
   +   */
   +  protected $interface = '\Drupal\Core\Menu\ContextualLinkInterface';
   +
   +  /**
   
   +++ b/core/lib/Drupal/Core/Menu/LocalActionManager.php
   @@ -31,6 +31,11 @@
      /**
   +   * {@inheritdoc}
   +   */
   +  protected $interface = '\Drupal\Core\Menu\LocalActionInterface';
   +
   
   +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -35,6 +35,11 @@ class LocalTaskManager extends DefaultPluginManager {
       */
   +  protected $interface = '\Drupal\Core\Menu\LocalTaskInterface';
   +
   

   it is a bit sad that we have two "ways" to specify it. Maybe we could just always use the property on the class? That way feels a bit more readable tbh.

6. +++ b/core/modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/fruit/FruitInterface.php
   similarity index 50%
   copy from core/modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/fruit/Apple.php
   
   copy from core/modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/fruit/Apple.php
   copy to core/modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/fruit/Kale.php
   
   copy to core/modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/fruit/Kale.php
   index 9aabeca..9c69b15 100644
   
   index 9aabeca..9c69b15 100644
   --- a/core/modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/fruit/Apple.php
   
   --- a/core/modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/fruit/Apple.php
   +++ b/core/modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/fruit/Kale.php
   

   oh git.

Working on moving to the default factory tomorrow. This is just a reroll.

This could pass.

Also expanded the test coverage a bit.

+++ b/core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php
@@ -70,6 +88,10 @@ public static function getPluginClass($plugin_id, $plugin_definition = NULL) {
+      throw new PluginException(sprintf('Plugin "%s" (%s) in %s should implement interface %s.', $plugin_id, $plugin_definition['class'], $plugin_definition['provider'], is_array($required_interface) ? implode(', ', $required_interface) : $required_interface));

We can ditch the is_array part, that stopped being valid after the last rewrite. It will always be a string here.

It turns out you cannot use @covers for static methods. Running the coverage reports failed for me.

The fail was just a missing bit in ArchiverManager, it happens to be the only manager I can see that both calls parent::__construct AND overrides createInstance().

I really think this is ready now. Anyone wants to step forward?

@tim.plunkett
Did you tried creating an issue against phpunit? Maybe in the future this could be fixed.

+++ b/core/modules/migrate_drupal/src/Plugin/MigratePluginManager.php
@@ -0,0 +1,34 @@
+ * Contains \Drupal\migrate\Plugin\MigratePluginManager.

Wrong Contains text. Needs to be migrate_drupal.

Other than this, I am 100% on board with this issue. Needs work for that comment, RTBC subsequent to that.

Eclipse

Fixed that and removed use statements made obsolete by this patch. RTBC per #44.

Yay, it works! Finding plugins that aren't extending their interface FTW.

Just reaffirming the RTBC on this.

Eclipse

Context conflict with #2096703: Image toolkits should use PluginFormInterface and ContainerFactoryPluginInterface

Nice patch - Committed 840cd4c and pushed to 8.0.x. Thanks!

This changed the order of arguments in DefaultPluginManager::construct(). I'm not sure why it wasn't just added last, but this means that all existing (contrib) plugin managers are now dead and need to be updated. So it should have a change record and we also need to update a bunch of existing change records I think.

Cool. I'm also wondering about the argument order. I wrote draft change record at https://www.drupal.org/node/2338561.

Also updated some older records mentioning DefaultPluginManager:
https://www.drupal.org/node/2087153/revisions/view/6979427/7634271
https://www.drupal.org/node/2034535/revisions/view/7443805/7634275

Cookies to anyone who can find another record that needs updating because of this change!

@Arla thanks for the draft change record. Here is an example of an issue that is impacted by this: #2336309: Devel Generate cannot be used due to Plugin discovery problems.

I think this has been addressed, if not please open a new issue.