Introduce PluginDefinitionInterface

Why is this a task and not something like a feature request?

1. +++ b/core/lib/Drupal/Component/Plugin/PluginDefinition.php
   @@ -0,0 +1,229 @@
   +  public function setClass($class) {
   +    if (!is_subclass_of($class, 'Drupal\Component\Plugin\PluginInspectionInterface')) {
   ...
   +  public function setDeriverClass($class) {
   +    if (!is_subclass_of($class, 'Drupal\Component\Plugin\Derivative\DeriverInterface')) {
   

   type hint?

2. +++ b/core/lib/Drupal/Component/Plugin/PluginDefinitionInterface.php
   @@ -0,0 +1,117 @@
   +  /**
   +   * Sets the plugin provider.
   +   *
   +   * @param string $provider
   +   *
   +   * @return $this
   +   */
   +  public function setProvider($provider);
   +
   +  /**
   +   * Gets the plugin provider.
   +   *
   +   * @return string
   +   */
   +  public function getProvider();
   

   If we're going to do this we need to document somewhere inside the plugin system what a provider is. As it stands its a defacto component provided outside of plugins by \Drupal\Component\Annotation and \Drupal\Component\Discovery

3. +++ b/core/lib/Drupal/Component/Plugin/PluginDefinitionInterface.php
   @@ -0,0 +1,117 @@
   +  /**
   +   * Sets the deriver class.
   +   *
   +   * @param string $class
   +   *   A class that implements
   +   *   \Drupal\Component\Plugin\Derivative\DeriverInterface.
   +   *
   +   * @return $this
   +   *
   +   * @throws \InvalidArgumentException
   +   */
   +  public function setDeriverClass($class);
   +
   +  /**
   +   * Gets the deriver class.
   +   *
   +   * @return string|null
   +   *   A class that implements
   +   *   \Drupal\Component\Plugin\Derivative\DeriverInterface or null.
   +   */
   +  public function getDeriverClass();
   +
   

   Do we want to put this on _all_ plugins? I haven't really thought too hard about the implications but gut reaction I'd vote for its own interface.

+++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
@@ -609,6 +585,7 @@ public function getRevisionTable();
+<<<<<<< HEAD

@@ -618,6 +595,9 @@ public function getLabel();
+=======
...
+>>>>>>> Comment 1

Oops :)

Didn't get all the way through, but here we go:

1. +++ b/core/lib/Drupal/Component/Plugin/PluginDefinitionInterface.php
   @@ -0,0 +1,123 @@
   + * For backwards compatibility with array-based plugin definitions, this
   + * interface implements \ArrayAccess. The required array keys and their
   ...
   +interface PluginDefinitionInterface extends \ArrayAccess {
   

   I can see why the default implementation implements \ArrayAccess but I don't see any reason for the interface to implement it. Dropping it (from the interface) would make things a bit easier in the future.

2. +++ b/core/lib/Drupal/Component/Plugin/PluginDefinitionInterface.php
   @@ -0,0 +1,123 @@
   + * - label: static::setLabel() and static::getLabel()
   + * - deriver: static::setDeriverClass() and static::getDeriverClass()
   

   I think label and deriver should be dropped, as they are not used for all plugin types. A lot of the Views plugins in particular extend the PluginId annotation which by design does not have a label. Also we should not enforce having a deriver.

   At the very least let's drop this for now and discuss this in a follow-up.

3. +++ b/core/lib/Drupal/Component/Plugin/PluginDefinitionInterface.php
   @@ -0,0 +1,123 @@
   +   * @param string $id
   +   *
   ...
   +   * @return string
   +   */
   ...
   +   * @param string $provider
   +   *
   ...
   +   * @return string
   +   */
   ...
   +   * @param string $label
   +   *
   ...
   +   * @return string|null
   +   */
   

   Unfortunately this won't fly. (Missing one-line docs)

4. +++ b/core/lib/Drupal/Component/Plugin/PluginDefinitionInterface.php
   @@ -0,0 +1,123 @@
   +   *   A class that implements
   +   *   \Drupal\Component\Plugin\PluginInspectionInterface.
   ...
   +   * @throws \InvalidArgumentException
   ...
   +   * @return string
   +   *   A class that implements
   +   *   \Drupal\Component\Plugin\PluginInspectionInterface.
   

   Why is PluginInspectionInterface required? I see why it should be recommended, but we should enforce it. In fact it is one of the primary principles of the plugin system that any class will do.

5. +++ b/core/lib/Drupal/Component/Plugin/PluginDefinitionInterface.php
   @@ -0,0 +1,123 @@
   +   * The provider is an arbitrary string that identifies the
   +   * package/component/module/extension that provides the plugin.
   

   I think it would be beneficial to mention that with the default plugin manager only module names + core + component are allowed.

Because #2485513: DefaultFactory cannot deal with objects as plugin definitions is blocked on this, this is now a bug.

If @EclipseGc and @neclimdul are on board I think this is good to go. Can we upate IS after #20? We probably need beta eval as well. We definitely need a change notice to let contrib know about this change.

1. +++ b/core/lib/Drupal/Component/Plugin/PluginDefinition.php
   @@ -0,0 +1,229 @@
   +   * @param string
   

   @var :)

2. +++ b/core/lib/Drupal/Component/Plugin/PluginDefinition.php
   @@ -0,0 +1,229 @@
   +   *   A class that implements
   +   *   \Drupal\Component\Plugin\PluginInspectionInterface.
   ...
   +   *   A class that implements
   +   *   \Drupal\Component\Plugin\Derivative\DeriverInterface or null.
   

   Theses should be one line 80 char limit doesn't apply to description.

3. +++ b/core/lib/Drupal/Component/Plugin/PluginDefinition.php
   @@ -0,0 +1,229 @@
   +    return $this->label;
   
   +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -675,13 +645,6 @@ public function getDataTable() {
   -    return (string) $this->label;
   

   We are not typecasting here so would that fine?

I would like to see PluginDefinitionInterface not implement \ArrayAccess. It allows us to abstract the PluginDefinitionInterface into smaller interfaces without causing major DX regression. I like the idea of using ArrayPluginDefinitionDecorator to still support Array type of plugin definitions.

This needs way more justification on the why's and hows, but as much as I understand it, the problem is that object definitions can't be written to, but need to allow writes for certain properties like 'class', 'id', 'provider', etc. at run time.

The idea is to use a common Interface for that.

My thoughts:

- Do not have PluginDefinitionInterface extend ArrayAccess - That is a no go and just will lead straight to hell ;).

- Use a Decorator, but extend it from ArrayObject, which is faster than ArrayAccess - though a little slower than native arrays.

If you _need_ a reference to the original array to be able to change it too, then you need to use exchangeArray, which works by reference. (I used that trick in my original wrap twig arrays in objects patch to get twig into core).

e.g.

new PluginDefinitionDecorator(array $definition);

Or

$definition_object = new PluginDefinitionDecorator();
$definition_object->exchangeArray($definition); // definition was passed by reference, that is a side-effect that however has not changed since ages.

( see http://php.net/manual/de/arrayobject.exchangearray.php)

Then use within the class:

setClass($class) {
$this->offsetSet('class', $class);
}

etc.

Just a note that this patch is nuking testbots with lots of must implement interface Drupal\Core\Plugin\Definition\PluginDefinitionInterface in the logs, so please avoid retesting the patches above and ideally run some tests locally before uploading a new one. Sorry for the invconvenience; the infrastructure team is also looking into how to make it so the bot is less brittle.

Congrats

+++ b/core/lib/Drupal/Component/Plugin/CategorizingPluginManagerInterface.php
index 0000000..c2b58b3
--- /dev/null

--- /dev/null
+++ b/core/lib/Drupal/Component/Plugin/Definition/ArrayPluginDefinition.php

+++ b/core/lib/Drupal/Component/Plugin/Definition/ArrayPluginDefinition.php
@@ -0,0 +1,238 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function setContextDefinitions(array $context_definitions) {
+    $this->validateContextDefinitions($context_definitions);
+
+    $this->arrayDefinition['context'] = $context_definitions;
+
+    return $this;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function getContextDefinitions() {
+    return isset($this->arrayDefinition['context']) ? $this->arrayDefinition['context'] : [];
+  }
...
+  /**
+   * {@inheritdoc}
+   */
+  public function setContextDefinition($name, ContextDefinitionInterface $context_definition) {
+    $this->arrayDefinition['context'][$name] = $context_definition;
+
+    return $this;

Mh I'm not sure why the generic one should care about contexts, given that most plugin definition give a shit about context

Didn't really review the approach taken on the patch that much but for me its sane. I did take a look on the code styles though and found only few nits from the patch

1. +++ b/core/lib/Drupal/Component/Plugin/Definition/PluginDefinition.php
   @@ -0,0 +1,256 @@
   \ No newline at end of file
   +++ b/core/tests/Drupal/Tests/Component/Plugin/Definition/PluginDefinitionTest.php
   @@ -0,0 +1,188 @@
   \ No newline at end of file
   +++ b/core/tests/Drupal/Tests/Core/Plugin/Definition/PluginDefinitionTest.php
   @@ -0,0 +1,98 @@
   \ No newline at end of file
   

   No newline at end of file

2. +++ b/core/lib/Drupal/Component/Plugin/Definition/PluginDefinitionBase.php
   @@ -0,0 +1,113 @@
   +  /**
   +   *
   +   */
   ...
   +  /**
   +   *
   +   */
   

   Empty docblock

Only got about half way through, but here's what I got:

1. +++ b/core/lib/Drupal/Component/Annotation/Plugin.php
   @@ -73,10 +86,12 @@ protected function parse(array $values) {
   -   * {@inheritdoc}
   +   * Gets the value of this annotation.
   

   We now allow @inheritdoc with additional stuff, so I think that should stay.

2. +++ b/core/lib/Drupal/Component/Plugin/Definition/ArrayPluginDefinition.php
   @@ -0,0 +1,263 @@
   +  /**
   +   * The array definition.
   +   *
   +   * @var mixed[]
   +   */
   +  protected $arrayDefinition = [];
   

   I think the "array" is not needed. This is the internal property so "The definition" is completely sufficient.

   The same goes for the variable name. I.e. I would suggest $this->definition instead of $this->arrayDefinition

   If you use array instead of mixed[] that would additionally reinforce that this is an array.

3. +++ b/core/lib/Drupal/Component/Plugin/Definition/ArrayPluginDefinition.php
   @@ -0,0 +1,263 @@
   +  public function getArrayDefinition() {
   

   I think in other places we're calling this toArray() and that would make sense here as well, IMO.

4. +++ b/core/lib/Drupal/Component/Plugin/Definition/ArrayPluginDefinition.php
   @@ -0,0 +1,263 @@
   +   * Merges another array definition into this one, using the other for defaults.
   ...
   +  public function mergeDefaultArrayDefinition(array $other_definition) {
   ...
   +   * Merges another array definition into this one, using the other for overrides.
   ...
   +  public function mergeOverrideArrayDefinition(array $other_definition) {
   

   I think the verbosity in this case actually makes things less clear, especially what the difference between the two is. I had to actually look at the code.

   I don't know what others think but what about mergeDefaults($defaults) and mergeOverrides($Overrides)

5. +++ b/core/lib/Drupal/Component/Plugin/Definition/ArrayPluginDefinition.php
   @@ -0,0 +1,263 @@
   +  public function doMergeDefaultDefinition(PluginDefinitionInterface $other_definition) {
   +    /** @var \Drupal\Component\Plugin\Definition\ArrayPluginDefinition $other_definition */
   +    $this->mergeDefaultArrayDefinition($other_definition->arrayDefinition);
   +  }
   ...
   +  public function doMergeOverrideDefinition(PluginDefinitionInterface $other_definition) {
   +    /** @var \Drupal\Component\Plugin\Definition\ArrayPluginDefinition $other_definition */
   +    $this->mergeOverrideArrayDefinition($other_definition->arrayDefinition);
   +  }
   

   If this is required it proves that IMO we're doing something wrong. Suggestion:
   1. Mark getArrayDefinition()/toArray() no longer deprecated and have mergeDefaultArrayDefinition()/mergeDefaults() and mergeOverrideArrayDefinition()mergeOverrides() use that in case they are passed a PluginDefinitionInterface (and remove the array typehints). Then document that both arrays and PluginDefinitionInterfaces are accepted with a @todo D9 to remove array support.

6. +++ b/core/lib/Drupal/Component/Plugin/Definition/PluginDefinitionBase.php
   @@ -0,0 +1,113 @@
   +  protected function doMergeDefaultDefinition(PluginDefinitionInterface $other_definition) {
   +    // Child classes can override this to perform an actual merge.
   +  }
   ...
   +  protected function doMergeOverrideDefinition(PluginDefinitionInterface $other_definition) {
   +    // Child classes can override this to perform an actual merge.
   +  }
   

   Should this not be abstract?

   Also ArrayPluginDefinition redeclares those as public, which is a bit strange.

7. +++ b/core/lib/Drupal/Component/Plugin/Definition/PluginDefinitionBase.php
   @@ -0,0 +1,113 @@
   +  protected function isDefinitionCompatible(PluginDefinitionInterface $other_definition) {
   +    if (!($other_definition instanceof $this)) {
   +      throw new \InvalidArgumentException(sprintf('$other_definition must be an instance of %s, but %s was given.', get_class($this), get_class($other_definition)));
   +    }
   +  }
   

   IMHO this is over-abstraction. I would rather have this logic in the two implementing classes by proper type-hinting, see my suggestion above.

8. +++ b/core/lib/Drupal/Component/Plugin/Definition/PluginDefinitionInterface.php
   @@ -0,0 +1,193 @@
   +interface PluginDefinitionInterface extends \ArrayAccess {
   

   Again, I fail to see how it would be an API change to remove this from the interface to the actual implementing classes.

   Also, I saw in the previous patches that properly splitting up the implementations in terms of optional features such as derivers etc. is a lot of work. But I don't see why we can't at least split up the interface and then have the base implementation implement all the interfaces.

9. +++ b/core/lib/Drupal/Core/Annotation/PluginID.php
   @@ -0,0 +1,43 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function get() {
   +    return $this->createDefinitionFromArray(array(
   +      'id' => $this->value,
   +      'class' => $this->class,
   +      'provider' => $this->provider,
   +    ));
   +  }
   

   Why is this method override needed?

10. +++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
    @@ -110,12 +115,12 @@ public function buildDataDefinition(array $definition, $value, $name = NULL, $pa
    -    foreach ($definition as $key => $value) {
    +    foreach ($definition->getArrayDefinition() as $key => $value) {
    

    I don't think we have to do this API change, if we have ArrayPluginDefinition implement IteratorAggregate and then do getIterator { return new \ArrayIterator($this->arrayDefinition); }

11. +++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
    @@ -141,20 +146,18 @@ public function getDefinition($base_plugin_id, $exception_on_invalid = TRUE) {
    +    if (isset($definition['type']) && !isset($definition->_merged)) {
    +      $definition->_merged = TRUE;
    

    The _merged business at the very least needs a code comment if we can't get around it somehow.

12. +++ b/core/lib/Drupal/Core/Entity/Annotation/ContentEntityType.php
    @@ -36,10 +36,8 @@ class ContentEntityType extends EntityType {
    -  public function get() {
    ...
    +  protected function preprocessArrayDefinition(array &$definition) {
    

    Seems a bit over-powered to introduce a whole method for that, no?

    Can't ConfigEntityType and ContentEntityType just override createDefinitionFromArray()?

13. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -350,28 +323,26 @@ public function id() {
    +    $this->validateClass($class);
    +    if (!is_subclass_of($class, 'Drupal\Core\Entity\EntityInterface')) {
    +      throw new \InvalidArgumentException('Entity classes must implement \Drupal\Core\Entity\EntityInterface.');
    +    }
    

    Seems like validateClass() should be overridden here?

14. +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionSubscriber.php
    @@ -129,6 +129,7 @@ protected function onHtml(GetResponseForExceptionEvent $event) {
    +    $content = $event->getException()->getMessage() . $event->getException()->getFile() . $event->getException()->getLine();
    

    Leftover debug?

15. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
    @@ -266,10 +268,14 @@ protected function cacheSet($cid, $data, $expire = Cache::PERMANENT, array $tags
    +      $definition->mergeDefaultArrayDefinition($this->defaults);
    

    If this is the only use of mergeDefaultArrayDefinition() why can we not do new ArrayPluginDefinition($this->defaults) first and then do a proper merge?

I tried to review the whole idea of the issue and for it sounds sane to make the plugin definitions more consistent. Internally this is quite an API change and I'm not very good to evaluate whether the disruption which is very high is lower than what we achieve by doing this. Anyway by looking the issue it is very easy to start implementing the new API which is positive thing.

1. +++ b/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php
   @@ -96,6 +91,10 @@ public function getDefinitions() {
   +   * @param \Drupal\Component\Plugin\Definition\PluginDeriverDefinitionInterface[] $base_plugin_definitions
   

   This is missing the param comment

2. +++ b/core/lib/Drupal/Component/Plugin/Discovery/StaticDiscovery.php
   @@ -6,6 +6,7 @@
    namespace Drupal\Component\Plugin\Discovery;
   +use Drupal\Component\Plugin\Definition\PluginDefinitionInterface;
   

   Missing space before use statement

3. +++ b/core/lib/Drupal/Core/Config/Schema/Mapping.php
   @@ -6,6 +6,7 @@
    namespace Drupal\Core\Config\Schema;
   +use Drupal\Core\Plugin\Definition\ArrayPluginDefinition;
   
   +++ b/core/lib/Drupal/Core/Config/Schema/Sequence.php
   @@ -6,6 +6,7 @@
    namespace Drupal\Core\Config\Schema;
   +use Drupal\Core\Plugin\Definition\ArrayPluginDefinition;
   

   Missing line before use statement

4. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
   @@ -6,6 +6,7 @@
    namespace Drupal\Core\Menu;
   +use Drupal\Core\Plugin\Definition\PluginDefinitionInterface;
   

   Missing line before use statement

5. +++ b/core/modules/views/tests/src/Unit/Plugin/views/field/EntityOperationsUnitTest.php
   @@ -7,7 +7,8 @@
   +  use Drupal\Core\Plugin\Definition\ArrayPluginDefinition;
   +  use Drupal\Tests\UnitTestCase;
    use Drupal\views\Plugin\views\field\EntityOperations;
    use Drupal\views\ResultRow;
   

   Indenting looks inconsistent

6. +++ b/core/tests/Drupal/Tests/Core/Plugin/Discovery/DerivativeDiscoveryDecoratorTest.php
   @@ -53,93 +53,19 @@ public function testGetDerivativeFetcher() {
   +  public function xtestExistingDerivative() {
   

   Probably xtest for debugging?

Thanks.

Yeah I think this is really not something we can still do, but sure, feel free to try to fight this.

:)

@alexpott and I discussed this and due to both the BC break and the scope of the change, unfortunately, this issue is not in scope for Drupal 8. We agreed the issue needed to be postponed to Drupal 9.

Thanks @Xano for your continued diligence on this patch. I'm sad that we can't make this change at this point. For future improvements of this scope, I'd recommend getting feedback from both framework and release managers early on, especially when we are not in the development phase of the release cycle.

Also, recategorizing as a major task.

I am sorry for you Xano. You have put a lot of effort into this issue.

What about #2458769: Derivative plugin definitions contain base plugin IDs instead of derivative IDs & #2485513: DefaultFactory cannot deal with objects as plugin definitions now? Those are bugs so how are we planning to fix those?

Before postponing this we need a clear plan how to fix the problems we currently have.

I don't know why this was changed from Bug report to Task but this whole problem space is speckled with bugs all around. Solving those bugs without any API breaks might not be possible in all cases and will certainly come with increased complexity and code debt in comparison to this solution. So before pushing this back we need to actually assess the situation and know what we are buying into with doing this. Anything else would be just lying to ourselves.

To not piss off people any more than necessary I fear is inevitable, let me be absolutely clear: Even though I personally disagree with the decision, it's totally fine if you, @xjm and @alexpott, decide that this should be postponed. And I'm quite certain that you made a informed decision about that. However, #103 completely lacks reasoning when taking into account all the different issues that are involved with this.

Edit: Better choice of words. I of course do not actually want to piss off anyone.

Here's an effort to clarify and provide some reasoning for the previous comment.

The situation (as I see it) is as follows (I am trying to put this as objectively as possible, but I am of course aware that at some point things will be arguable):

The plugin system allows for both array-based plugin definitions and object-based plugin definitions and both are used in core. Some code in the plugin system needs to interact generically with plugins, so it cannot know which definitions are array-based and which are object based. There is already some code to account for the two cases but first of all that code is by definition very messy with type-checks, casting, and all sorts of "safety checks" galore. More importantly, as @Xano and others have more or less recently uncovered it is by far insufficient to be able to use the plugin system generically in any meaningful way. This is as simple as retrieving the plugin class from the definition in order to instantiate a plugin instance as discovered in #2485513: DefaultFactory cannot deal with objects as plugin definitions. This alone makes using the plugin system with object-based definitions impossible for many use-cases. Plugin derivatives also do not work because they currently rely on a notion of array merging which does not exist as such for objects. That is why we explicitly removed derivative support from entity types (which use object-based definitions) even though that support was explicitly baked in, to support ECK in Drupal 8. Such a thing is not possible (without hacks) in Drupal 8 at the moment. We do not have a generic plugin mapper implementation in core, but if we did or if someone tries to write one in contrib, she will hit the same wall. In other words everything in the Plugin system but the discovery (i.e. the retrieving of the definitions itself) - namely factories, mappers, and derivatives - are broken in some way (surely not broken as in completely-broken-WSOD-fatal-in-every-single-use-case broken, but still broken in some way). So much for the status quo.

If we were not in release-mode the easy answer to all this would be to simply require definitions to be proper objects implementing a PluginDefinitionInterface wholesale without caring about backwards compatibility. Needless to say, this is not even remotely an option because it would be a huge API change for anyone already using plugins.

Because PHP provides the \ArrayAccess crutch plus Countable and Traversable, however, we are able to do this conversion with a much smaller set of API changes. We just need a base plugin definition class which implements all the "fake array" logic and most of consumer code will not see the difference. One notable exception to this is the merging of arrays, which cannot be "faked" in any way. For that reason explicit methods are provided for this, so code that needs to merge plugin definitions does need to be changed. Because generic code that interacts generically with plugin definitions now expects to receive objects implementing PluginDefinitionInterface (as that is the whole point: to make the input parameters reliable, so the behavior can be reliable and (more importantly) working), code that creates plugin definitions needs to be updated. This is something we cannot avoid, regardless of the specific implementation of this patch. However, most plugin types in core use the generic discovery mechanisms, so this change is is completely transparent to them.

There are also some other API changes in the current patch but I would argue that some of them could be avoided, if we wanted to:
1. The creation of the definition objects from annotations is done in Drupal\Core\Annotation instead of Drupal\Component\Annotation, which was assumingly done to keep the code in Drupal\Component\Annotation self-contained and without too many assumptions. This leads to the fact that many plugin annotation subclasses need to change their base class from e.g. Drupal\Component\Plugin\Annotation to Drupal\Core\Plugin\Annotation. We could avoid this by simply putting the mentioned code in Drupal\Component\Annotation\Plugin (et al.) directly.

2. We could remove some of the PluginDefinitionInterface typehints, so that code that extends the base classes does not need to be updated to comply with the typehints. Specifically, DefaultPluginManager::processDefinition() comes to mind.

3. This might be going a bit far, but we could even do things like check if a plugin deriver returns array definitions, and convert them to objects, when retrieving them in DerivativeDiscoveryDecorator. That way not even derivers would need to be changed and the ugliness would combined in a single place (with a big fat @todo D9) instead of scattered all around core and contrib.

With those 2 or 3 changes I don't really what much there is left in terms of disruption. There might still be some technical API changes, but looking through core and thinking about what I have done in custom modules so far I can't come up with anything right now. Note that this is just me thinking out loud and - needless to say - there might be things that I have missed or that I am understating, but I feel that this sort of discussion (i.e. how can we reduce/limit/avoid disruption, while still reaping (most of) the benefits) was not conducted extensively above and I think it should before giving up on this.

I want to reiterate one more time that by pushing this to D9 we are effectively saying the following:
1. Not only are we accepting the current messiness of various parts of the plugin system but we accept that we have to add more messiness in order to fix bugs such as #2485513: DefaultFactory cannot deal with objects as plugin definitions that only this issue would be able to solve without hacks.
2. Any contrib that interacts generically with plugin (definition)s will have to employ the same messiness.
3. Things like ECK will not be (cleanly) possible in D8. More precisely, derivatives will not work with object-based plugin definitions. Note that this point is not to be underestimated. It is pure coincidence of the D8 development cycle that entity types are the only object-based plugin definitions in core; with the general move to OOP, object-based plugin definitions can in fact be considered preferred over the array-based ones and will likely be very common in contrib.

And finally let me also restate that I will not even be pissed (albeit disappointed) if the decision is made to push this to D9 bearing all the above in mind. It just was not at all evident from the above that there is such an awareness, which is why I resisted to postponing this - to my mind - prematurely.

Hope that helps clarify where I am coming from , and I look forward to everyone but especially @Xano and @xjm correcting me where I am wrong, over-simplified things, missed things, etc.!

Re-adding tag.

Many thanks @tstoeckler for the very detailed comments and thoughtful feedback!

I still feel the risk of this patch is much too high given how close we are to an RC, but I haven't had a chance to follow up on all of @tstoeckler's points yet so I'll hold off on commenting more until we've taken a careful look. The suggestions for reducing the BC breaks are good insights, but I'd suggest not attempting to implement them unless/until the points raised above have been reviewed (specifically by a framework manager; thanks @Fabianx for tagging).

That said, I am still going to set this back as a major task, because it in itself is not a bugfix. Incomplete or faulty API design that may result in bugs is not in itself a bug. Refactoring to reduce fragility and provide a more robust API is a task. This issue is definitely a major one.

For #2485513: DefaultFactory cannot deal with objects as plugin definitions, I actually thought the current patch in that issue (before it was postponed) was an acceptable workaround during this phase of the release cycle.

Thanks for the summary in #108.

I didn't review the patch in full, but it looks like the changes outside the plugin system itself are predominantly in Views and test coverage.

Apart from that the component -> core change and interface type hints do stick out. Core/Component is a fairly arbitrary split so I'm meh on that change - we can add a @todo to clean things up later if we have to (i.e. we could add the core class, have it subclass the one in component, deprecate the component one immediately).

However the interface type hint is going to make it easier rather than harder for contrib modules to deal with the API change here I'd expect, so not sure removing that would be worth it. And then if contrib had to update for that change, it could also do the rename for component/core at the same time.

So agreed with @xjm, not worth trying to reduce the footprint of the patch until we've discussed a bit more whether that's worth it.

Are we sure that the only affect on contrib modules will be:
1. The core/component change
2. The interface type hint
3. Array merging - which seems mostly theoretical?

Would be good to get Berdir's opinion since he has the most familiarity with 8.x contrib and maintains some custom modules too.

However on that rename and type hint, if we did remove those changes and did the automatic array to ArrayAccess conversion, then I'm wondering whether the patch would be something we could get into a minor version (i.e. if it did slip 8.0.0, move it to 8.1.x rather than 9.x).

For example I got CacheArray into 7.x as a new API, and converted places like the theme registry and schema cache to use it. While theoretically people could have used array_* methods on those arrays, in practice almost no-one did and those patches went relatively smoothly. I don't think we've discussed array -> ArrayAccess for minor versions at all.

Also I don't see Tim Plunkett or Alex Bronsteing on this issue at all, and EclipseGc and neclimdul only once each, so tagging 'needs subsystem maintainer review'.

So I don't have a strong view on getting this in before RC yet (I would not object to it going in, but I also don't feel very strongly that it must), but it's definitely worth leaving the issue at 8.0.x to figure out ways that we can either do that (as is or with smaller sets of changes) or allow it into a minor release if not.

Reroll.

Lots and lots of automerging, 5 seemingly trivial conflicts to resolve. Limited testing - that site install works and basic pages render is about all I can say.
I will come back of fix any issues testbot finds.

Doh

Is is possible for this issue to go in before 8.0.0? -- I hope so.

I am not addressing the errors in the comment... I wanted to give the new interfaces a quick look over before they become locked down for a number of years.

Found nothing worth talking about save a couple of minor @throw comments that were missing.

Well I can say that this approach is wonderful and sound and I have no problem with it, except that it's August 2015 and it's way too late IMO.
So, let's just be blocked on a framework manager.

@Xano asked me to respond here and expressed concerns that I had not responded further or posted my thought process in more detail. Note that @catch did respond a month ago in #112 and I considered this issue off my plate at that point.

Frankly this is simply a lower priority than managing and fixing the critical issues blocking Drupal 8's release, which is how I spend all of my time. I'd hazard that's why there haven't been further responses from other committers as well. And that is exactly why I think this should be postponed to 8.1.x or later: No matter whether we can make it less disruptive, no matter whether we can break it into smaller BC chunks, the resources required and the risks added by making these changes when we are almost to RC is just not justifiable for me, especially when the other option is to spend that time directly on criticals. This is why we have a beta policy: to help us separate the things that we should do, but aren't worth making 8.0.0 take longer, from the things that we should do that are unlikely to make 8.0.0 take longer or that need to be done even if they do make 8.0.0 take longer. @catch's "I'd be okay with this not going in before 8.0.0" is, for me, a reason in itself to postpone this or any other large non-critical effort, no matter how beneficial.

Note that I am not speaking for the other committers. I still think it should be postponed, even after reviewing @tstoekler's feedback; @catch doesn't think it should be postponed until we've explored less disruptive options, but also isn't worried if it doesn't go in; @alexpott hasn't weighed in again since #108; @effulgentsia hasn't reviewed it at all. If it were a year ago, I'd propose scheduling a meeting with those guys and some of the relevant component maintainers and other stakeholders to hammer out the path forward. But it's not a year ago, and every hour of most of those folks' available time is currently going into trying to get D8 released.

Again, is not a bug; please do not set the category back to that. #2485513: DefaultFactory cannot deal with objects as plugin definitions is the bug, and can be solved without this in a 2K patch rather than a 400K one. It may be ugly and a hack and not the ideal fix, but we are at the phase in the release cycle for ugly, non-ideal hacks, not the phase for thorough and excellent architectural improvements.

Let me reiterate that none of this is meant as criticism of the patch itself. It's great work. It's just too late, and I think it was already too late when the issue was posted. Basically I agree with what @tim.plunkett said in #131.

Seems like @Xano is moving forward with #2485513: DefaultFactory cannot deal with objects as plugin definitions and we can also fix #2458769: Derivative plugin definitions contain base plugin IDs instead of derivative IDs without this issue. I think it is safe to move to 9.x now after 22 days of no disagreement. Thanks @xjm for clearing this up. Thanks @Xano for working on this.

We can re-add needs tag when the issue is ready.

PDI now exists, I'm closing this. If we want to bring in more of the Plugin.module functionality, that can go in targeted issues.