DefaultFactory cannot deal with objects as plugin definitions

+++ b/core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php
@@ -77,11 +77,24 @@ public function createInstance($plugin_id, array $configuration = array()) {
+    elseif (is_object($plugin_definition) && method_exists($plugin_definition, 'getClass')) {

Magic method access is not a solution, but a workaround. I know we're in a tight spot, but let's not 'fix' things the wrong way.

I guess xano wants to have an interface ... #2458789: Introduce PluginDefinitionInterface

Xano would like to see an interface, but he would even more so like to see we stop magically figuring out which properties or methods to call, like #2494719: ConfigFormBaseTrait is confusing. Such solutions only make things worse.

Care to share how you think it should be? ;-)

Ideally an interface, of course. I'm aware of the fact we are in code freeze right now, but that's no reason to commit workarounds that utilize very bad practices. We *can* still break BC to fix severe fundamental problems, which is why I don't want to rule out solving this issue (and several others) by adding the interface.
If this problem cannot be solved by introducing a new interface, then I have no potential fix in mind right now.

#2458789: Introduce PluginDefinitionInterface now passes all tests and is ready for manual review. If that patch gets committed, fixing this issue will be really easy.

Tagging for framework manager review as #2458789: Introduce PluginDefinitionInterface was pushed back.

To re-iterate because this issue was referenced elsewhere: the patch is no workaround, but a hack. The new code is much more difficult to debug and will only work when object definitions work according to hardcoded, undocumented assumptions. I understand why it was proposed, but code like this should never be used.

Bump.

We shouldn't be using magic method access, even if it works in core's current configuration. See #11.

The only good solution involves working with an interface on entity/object definitions (reasons in #11). Seeing as this bug lives in \Drupal\Component, we cannot use EntityTypeInterface::getClass(), unfortunately, as that lives in \Drupal\Core. We can either introduce a new small generic interface, or pursue #2458789: Introduce PluginDefinitionInterface.

Here's a solution without magic or BC breaks. It works with entity types as well. I also fixed some left-over array access.

I forgot to remove one more interface method.

1. +++ b/core/lib/Drupal/Component/Plugin/Definition/PluginDefinitionInterface.php
   @@ -0,0 +1,38 @@
   +/**
   + * Defines a plugin definition.
   + *
   + * @ingroup Plugin
   + */
   +interface PluginDefinitionInterface {
   

   IMHO we should explain when to use it

2. +++ b/core/lib/Drupal/Component/Plugin/Definition/PluginDefinitionInterface.php
   @@ -0,0 +1,38 @@
   +   *   If the class is invalid.
   

   What defines an invalid class?

What defines an invalid class?

Good question. It depends on the plugin type, as most of them have interfaces that must be implemented, or base classes to be extended. I'll have to think about how to phrase this. Suggestions are welcome.

Changed SHOULD to MUST as per @dawehner's recommendation, and added tests.

So I screwed up the patch after playing with Composer. Here's a clean one.

This kinda reverts the original idea of #2005716: Promote EntityType to a domain object which was about:

This patch removes Entity type's dependency on Plugins among other things.

Entity types have depended on plugins for as long as I can remember, at that issue didn't change that. I don't disagree that decoupling these two systems may potentially be a good idea, but the situation at the moment is that entity types are discovered through the plugin system, but that we don't really support object definitions properly. We're not going to change this dependency, as we don't only use plugin system classes, but have tied the entity API to the plugin system by at least making EntityManagerInterface extend PluginManagerInterface and CachedDiscoveryInterface. Changing this would be a BC break.

TLDR; entity API depends on the plugin system and will do so until Drupal 8's EOL. Let's make sure this dependency works better than it does now.

The change looks nice compared to #2458789: Introduce PluginDefinitionInterface which caused quite bad BC breaks. In the patch, there is quite a good test coverage being added for the interface. We still need beta-evaluation, sign off from the framework manager and feedback about #24/#25

+++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
@@ -6,6 +6,7 @@
 namespace Drupal\Core\Entity;
+use Drupal\Component\Plugin\Definition\PluginDefinitionInterface;

Minor detail; missing line change between namespace and use statement

Addressing #26.

Re-roll.

Everything has been addressed from #26

Let's fix it now. The committers have more important things to work on than fixing typos :-)

We can use the newly added interface to fix #2458769: Derivative plugin definitions contain base plugin IDs instead of derivative IDs in the same way we fixed the bug here.

The size of patch increased quite a lot..

I had forgotten to rebase. Here's the correct patch, plus interdiff (same as the previous one).

Let's actually not require a fluent interface, to allow definitions to be immutable. EntityType::setId() can continue to return $this, as that is covariant with static.

That looked like a random test failure.

Random migration test fail.

Random HTTP query fail?

That was an aborted re-test. RTBC and triggering a re-test.

+++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
@@ -15,7 +17,7 @@
  */
-interface EntityTypeInterface {
+interface EntityTypeInterface extends PluginDefinitionInterface {
 

Life is sad, but we should be honest. Entities are plugins, given that we use plugin manager interfaces. This coupling doesn't make things worse, it just shows it you directly.

I think this is a good compromise - we should be honest that we did truly decouple plugins and entity type discovery and this is a very minimal surface area. It also will make plugin managers that use objects for plugin definitions are more robust. I'm committing this under the committer discretion provision of the beta policy - but let's be honest - this is not a bug.

Committed 125cda4 and pushed to 8.0.x. Thanks!