Allow object-based plugin definitions to be processed in DefaultPluginManager::findDefinitions()

Ok, this seems pretty sane... Test coverage?

Eclipse

Looks sensible and useful to me!

Eclipse

+++ b/core/lib/Drupal/Component/Plugin/Definition/PluginDefinitionProviderInterface.php
@@ -0,0 +1,21 @@
+/**
+ * @todo Move to \Drupal\Component\Plugin\Definition\PluginDefinitionInterface.
+ */

This needs a 9.0.x issue and also should be more than an @todo. Also this issue should have a change record.

This looks absolutely great.

+++ b/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.php
@@ -453,3 +480,49 @@ public function submitConfigurationForm(array &$form, FormStateInterface $form_s
+class ObjectDefinition implements PluginDefinitionInterface {

Would be nice to provide this as an actual PluginDefinition class in a non-test namespace for object-based definitions to extend. That should be a follow-up, let's get this in first.

Also added change record, so this is good to go, IMO.

This change looks very sensible. The one concern I have is for contrib that has implemented \Drupal\Component\Plugin\Definition\PluginDefinitionInterface. The comment in #10 allays a few of the concerns but I think the issue summary needs an update to reflect the changes made by this issue and to mkae the case for this change in 8.3.x. I'm not convinced that the plugins section on https://www.drupal.org/core/d8-bc-policy actually covers this case.

#13 addressed in #15 so back to RTBC.

The one concern I have is for contrib that has implemented \Drupal\Component\Plugin\Definition\PluginDefinitionInterface. The comment in #10 allays a few of the concerns but I think the issue summary needs an update to reflect the changes made by this issue and to mkae the case for this change in 8.3.x. I'm not convinced that the plugins section on https://www.drupal.org/core/d8-bc-policy actually covers this case.

The plugins section doesn't, but I think that https://www.drupal.org/core/d8-bc-policy#interfaces does. From there:

When implementing the interface, module authors are encouraged to either:
Where there is a 1-1 relationship between a class and an interface, inherit from the class. Where a base class or trait is provided, use those. This way your class should inherit new methods from the class or trait when they're added.
Where the interface is implemented directly for other reasons, be prepared to add support for new methods in minor releases

I think it's a shame that we didn't add a PluginDefinitionTrait when we added PluginDefinitionInterface. But that's because we added PluginDefinitionInterface prior to the above BC policy clarification. I think it would be good though to provide that trait now, so that even though we inconvenience some contrib authors with this change, at least they'll be able to opt in to the trait and then not be inconvenienced again when we add another method in the future. I'm not necessarily opposed to punting the trait to a follow-up, but I'd feel better with committing it as part of this patch, so that the CR can instruct people about it.

Would be nice to provide this as an actual PluginDefinition class in a non-test namespace for object-based definitions to extend.

That might be more sensible than a trait in this case. Since LayoutDefinition, EntityType, etc. do have a "is a" relationship to plugin definitions, so a base class is logically appropriate here, I think.

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
@@ -298,6 +295,31 @@ protected function findDefinitions() {
+    // Attempt to convert the plugin definition to an array.
+    if (is_object($plugin_definition)) {
+      $plugin_definition = (array) $plugin_definition;
+    }

I think instead of typcasting to array either use Reflection or check for toArray and as third choice convert typecast to array.

Fair enough!

+++ b/core/lib/Drupal/Component/Plugin/Definition/PluginDefinitionInterface.php
@@ -12,6 +12,14 @@
   /**
+   * Gets the unique identifier of the plugin definition.
+   *
+   * @return string
+   *   The unique identifier of the plugin definition.
+   */
+  public function id();

This is confusing. Usually this is the plugin ID, the plugin definition itself does not generally have a distinct ID. The documentation needs to be updated to reflect that.

Also this should be getId(), not id(). I realize that the latter is what EntityType uses, but this is a new interface so we should be using best practice naming here, IMO.

Marking needs work for the first point at least, I realize the last point is potentially contentious.

Thanks, awesome that we already have dedicated issue for the getId() vs. id() thing. Docs look perfect now.

Patch looks great to me. Ticking some credit boxes.

Pushed to 8.4.x and cherry picked to 8.3.x. Let's update the CR to mention the id() method and base class and then publish it.

publish the change record