Allow plugins to have services injected

I have been struggling with this lately too. I think it's pretty easy to add a ContainerAwareFactory for plugins that would forward the container through the plugin with reflection. And I think that's what we want for other, specific services as well... Forward them to the plugin constructors with reflection.

I don't think 2 is feasible. 1 is what we've discussed in VDC issues.

Are you suggesting adopting more constructor injection within the Plugin classes? We should put together a list of classes that are missing it, and battle through them one by one in separate issues. If the object dependencies are too vast, than getter/setters would also work.

Thinking about this more I believe forwarding is not an option. It's simply not flexible enough. Would it be an option to allow plugins to define a factory service through which they should be built?

ad #5: I think the plugin manager should cover that. So what about:

3) Add the dependencies to the plugin manager and pass them on when intantiating plugins (or use setter injection). This also means you can still use the plugin manager when testing with injected test-dependencies.

ad #6: Yes, but there is no way for you to know which services custom (contrib?) plugins will require. There is always a chance that a plugin might require a service which you did not foresee when you wrote the plugin manager. Also, you don't want to randomly inject services into the plugin manager just because they "might" be used by one or more of the plugins. You want to dynamically inject them when building the plugin. Hence, forwarding is a bad idea if you ask me.

For reference, the preference order is:

Injected dependencies > injected container (service locator) > drupal_container() > normal utility functions

We should always be pushing as far to the left as possible.

Also for reference, the first part of this thread: #1810912: [meta] Decide on pluggability, especially the first 2-3 comments. (After that it drifts off topic.)

(Thinking aloud...)

What we want to be able to do is minimize the number of objects that are ContainerAware, and ensure that those that are do little if anything beyond being ContainerAware mappers. So I'd argue that plugins themselves should not, ever, be ContainerAware. They should be injected. The plugin mappers, however, may be container aware as they're really just glorified factories. (If I have my plugin terminology right.)

For that to work, however, the plugin instances, or at least classes?, need to be registered in the DIC so that the container can handle their dependencies. But that means a potentially very very large number of objects in the container, and dynamic registration in the container. It also means we need a way other than writing a bundle class to specify what services a given plugin instance uses.

We are going to need to resolve dynamic container registration anyway. But the latter problem is more troublesome, as specifying service IDs on the plugin class itself (eg, as annotations) doesn't really get us much more decoupling than just injecting the container itself. It's still hard coded to particular service IDs, which means that, say, swapping out the DB connection for a slave connection is difficult or impossible or impractical.

We could, I suppose, use annotations but allow them to be manipulated; effectively then it's no worse than an alter hook, but done using compiler passes.

Of course, then we still need to solve dynamic container registration. Blargh.

"Just this once", no. However, consider that we want to end up with services "database", "database.slave", "database.default", "database.default.slave", "database.someotherkey", "database.someotherkey.slave", etc.

Now, for whatever reason, I want to change plugin class Foo to use database.slave rather than database, because I know on MY site it is safe to use the slave database. The constructor signature doesn't change; it's still __construct(DatabaseConnection $connection); But we have to change what object is wired into it.

How do we do that?

For most services, that's done in the bundle so you'd use a compiler pass to muck with it. If it's specified in an annotation on the class, then you need to edit the annotation in order to change it.

Of course, most plugins have an Alter decorator on them, which allows us to muck with stuff defined in the annotation. (I really want to move that from an alter hook to an alter event, but that's neither here nor there.) So, maybe that's sufficient? Still doesn't help with dynamic registration though. I may be over-thinking it, honestly. We should check into annotation usage in Symfony and see if they have a better solution here. (Although given the different use case they may not.)

child classes do not inherit the "definition" of their parents

I think it would be nice to somehow add definition inheritance to the plugin system. Feel like opening an issue for that?

If $my_plugin->discovery is the plugin manager, then suddenly $my_plugin is not serializable anymore

Actually, the serialization problem also exists if $my_plugin->discovery is the manager's discovery object, but if that discovery object contains a ProcessDecorator with a reference to the manager.

Regarding inheritance, what about a static property/method which is called by the ProcessDecorator, to put it into the plugin definition?
If we agree on that I would be happy to work on that.

For some dependencies, interface injection may be sufficient.

if ($plugin instanceof DatabaseAwareInterface) { 
  $plugin->setDatabaseConnection($this->container->get('database'));
}

But that's not going to scale beyond a few very common dependencies like the database or translation system.

Ideally we'd be able to put the plugin instances into the DIC, and then the DIC configuration handles setting up the dependencies for the instance. If it's a bit slow to detect that's fine, because the runtime impact is almost nil once the container is compiled. Of course, to do that we need to figure out the "dynamic DIC configuration" problem, which so far we have not found a solution for.

Hm, you're correct. I forgot about that annoying detail. :-)

So we need a ContainerAware plugin factory, and then some way to indicate which services a plugin requires. But putting service IDs in a plugin hard-couples it to the DIC, which we want to avoid (and doesn't then buy us much over the Plugin being ContainerAware, which is an option but one we want to avoid most of the time). Interface injection only works for a few key services and doesn't scale. Property injection is un-Drupalish (requires public properties) and doesn't scale any better than interface injection anyway.

Which leaves Constructor injection, which is not really much better than interface injection in this case. We'd be using reflection to check the type of constructor parameters, but that only gives us interface names, not service IDs. We also have the variable name available, which only helps if we're going to require that variable name match a service ID, and then we're right back where we started. We could also use annotations on the class or constructor, but then we're once again back to the class having service IDs, and per yched that data is not cacheable.

So... I think we're left with only two possible alternatives:

1) Accept that the class will syntactically have service IDs in it somewhere, meaning it's not truly decoupled from the container.

2) Define dependencies in an alterable fashion (annotation, a bundled config file a la Symfony, a hook, etc.) and do the work necessary to make that cacheable so that we avoid the performance problem of another alter invocation one very frickin plugin load.

Although, now that I think about it here's an idea I'm loathe to suggest: We're making the DIC fast by compiling it to PHP. Could we make the plugin factories fast by compiling THEM to PHP, in much the same way?

That could work. That way we don't need to write a new compiler.

Memory-wise, I think I'm OK with it. I can't say for sure how much of an impact it would be, but my gut feeling is that it would be a wash, since we'd need that data anyway on most requests. By putting it in static code, at least APC has a chance to help with it. There's no "partial container" concept, but I don't think we need that.

The extra processing at compile time doesn't bother me. The fact that it then runs into the "dynamic DIC configuration" problem once again.

We really need to figure that bugger out...

I may just be exposing my own ignorance here as I still haven't really looked at the plugin system, but I'll risk it anyway...

Is there a reason we can't use the abstract factory pattern for plugins? That would seem to me to solve the problem of plugins that are dependent on services. For each plugin definition you'd have a factory, and that factory would be a service, and thus wired into the DIC. So it could pass on service dependencies to the plugins it instantiates. It would not be container aware and there'd be no hard-coding of service IDs anywhere except in the DIC wiring. Overriding the exact service that gets used could then be done in a bundle, i.e. just by changing the factory service's definition.

I'm guessing this is just totally at odds with the architecture of the plugins system but thought I'd throw it out there because it's how I've been assuming this kind of thing would work.

... Interface injection only works for a few key services and doesn't scale. Property injection is un-Drupalish (requires public properties) and doesn't scale any better than interface injection anyway. Which leaves Constructor injection ...

The current plugin base constructor looks like that:

  public function __construct(array $configuration, $plugin_id, DiscoveryInterface $discovery) {

So yeah constructur injection could work, what about method injection?

@katbailey
The problem with having an one factory per plugin could be too many definitions in the container. I think at least just core has around 300 or even more existing plugins already and the number could be 1000 or more on big sites, see the grep:

find . -name "*" | grep "Plugin" | grep ".php" | wc -l
520

One problem I see, which maybe is out of scope, is plugin dependency inheritance. If you have a plugin which requires service A and B and another plugin which extend that class you will always have to stick with the dependencies, they never can be updated, as all child plugins would take care about it.
Maybe it could be possible to use the inheritance information to build up the dependency automatically, but require to manual doing that will not scale.

This should basically work. https://gist.github.com/4703642 I know people aren't thrilled about the prospect of calling drupal_container() directly in the factory. If the manager is in the DIC and the container is injected into the manager, then when the manager is passed to the factory, it can be called from there. I'm not thrilled about that solution since I think it increases the barrier to entry on creating a manager, but in any event. This basic code should work.

The code provided will give the plugin developer the ability to type hint their dependencies in the plugin constructor, and will allow them to have any parameters they want for the constructors (not unified for the plugin type). This code also provides the caller the ability to inject the dependency themselves instead of the factory supplying it for the plugin.

Eclipse

Given the importance of this issue, I'm comfortable punting on the inheritance question for now. Redeclaring inheritance via annotations sounds ugly, but at least it could be added later if we find a need for it.

Speaking to folks at Sunshine PHP, it looks like the Symfony answer in this area seems to be "meh, put a service name in an annotation." (Possibly including service aliases in the container.) So while that still feels ugly to me, no better solution has presented itself and worse solutions certainly have, so I'm OK with running with that.

+++ b/core/lib/Drupal/Component/Plugin/PluginBase.php
@@ -44,11 +44,18 @@
-  public function __construct(array $configuration, $plugin_id, DiscoveryInterface $discovery) {
+  public function __construct(array $configuration, $plugin_id, DiscoveryInterface $discovery, array $dependencies = array()) {
     $this->configuration = $configuration;
     $this->plugin_id = $plugin_id;
     $this->discovery = $discovery;
+
+    foreach ($dependencies as $name => $service) {
+      $this->{$name} = $service;
+    }
   }

We need to somehow document that even with this magic you MUST still define the services as properties on your plugin object.

1) They become public otherwise, which we don't want.

2) We need that as a place to document them.

3) There's a non-trivial memory benefit to doing so: https://gist.github.com/nikic/5015323

I am also uncomfortable with throwing the dependencies in as an array. That means we can't type hint them. Is there no way to have individual plugins define the constructor params themselves, and then use reflection for linking up the variables like we do for routing?

+++ b/core/modules/system/tests/modules/plugin_test/lib/Drupal/plugin_test/Plugin/plugin_test/mock_block/MockUserLoginBlock.php
@@ -24,12 +24,27 @@ class MockUserLoginBlock extends PluginBase {
+  /**
+   * An injected service.
+   *
+   * @var \Drupal\plugin_test\Plugin\PluginTestDependency
+   */
+  public $service_2;

These should be in camelCase, like any other property.

Do we have benchmarks for the relative factory costs? Reflection itself is a fairly fast operation. Not zero cost, but not the time suck that many language operations are.

I'm wondering whether we could reduce the amount of overhead needed for the ReflectionFactory by not determine the information at runtime but sort of cache it?

you'd have to cache it per plugin id, but that's an interesting idea.

Eclipse

The other option is to not use reflection and just trust that the constructor has variables named whatever the dependency keys are named, and in the order that they're in the annotation. Then we could just use cufa. There are some DX wtfs in that suggestion, but that should be a performance win over reflection.

actually names don't matter if we're doing that suggestion.

Actually I'm full of crap. Apparently we can't call the class constructor w/o using reflection or manually instantiating.

Just started a factory that uses ::create instead of new, but still passes configuration etc. along.

+++ b/core/lib/Drupal/Component/Plugin/PluginBase.phpundefined
@@ -7,6 +7,7 @@
+use Symfony\Component\DependencyInjection\Container;
 
 /**
  * Base class for plugins wishing to support metadata inspection.
@@ -52,6 +53,26 @@ public function __construct(array $configuration, $plugin_id, DiscoveryInterface

@@ -52,6 +53,26 @@ public function __construct(array $configuration, $plugin_id, DiscoveryInterface
   }
 
   /**
+   * Creates an instance of the plugin.
+   *
+   * @param \Symfony\Component\DependencyInjection\Container $container
+   *   The container to pull out services used in the plugin.
+   * @param array $configuration
+   *   A configuration array containing information about the plugin instance.
+   * @param string $plugin_id
+   *   The plugin_id for the plugin instance.
+   * @param DiscoveryInterface $discovery
+   *   The Discovery class that holds access to the plugin implementation
+   *   definition.
+   *
+   * @return static
+   *   Returns an instance of this plugin.
+   */
+  public static function create(Container $container, array $configuration, $plugin_id, DiscoveryInterface $discovery) {
+    return new static($configuration, $plugin_id, $discovery);

Please don't add this to the Component PluginBase. Make a Core DICPluginBase or something. The whole of th eplugin system does not need a dependency on Symfony's DIC.

+++ b/core/lib/Drupal/Core/Plugin/Factory/MethodFactory.phpundefined
@@ -0,0 +1,24 @@
+    return $plugin_class::create(drupal_container(), $configuration, $plugin_id, $this->discovery);

This is completely unnecessary, just inject the container into an implementing manager as part of the containing module's [Module]Bundle.php declaration for the plugin manager class, and then PASS THE WHOLE MANAGER TO THE FACTORY, as I've been saying for months and months and months now.

+++ b/core/modules/views/lib/Drupal/views/Plugin/ViewsPluginManager.phpundefined
@@ -35,7 +36,7 @@ public function __construct($type, array $namespaces = array()) {
+    $this->factory = new MethodFactory($this->discovery);

Not this, this:

$this->factory = new MethodFactory($this);

- __construct() methods have different signatures in each plugin class (they hold the dependencies of this specific plugin implementation), that's fine, PHP allows that.

This is actually a function of not having __construct() on and interfaces which was a deliberate design decision so injection systems like the reflection factory and what ever we're building here could be built.

that is, unless the plugin type uses a reflection-based factory, but for performance sake we have to consider those as the exception.

I'll say that despite the title, this issue has been focused on methods for injecting services not just random objects because the later is easily supported with our current tools making this kinda irrelevant. Also, reflection has a cost but that cost could be entirely acceptable in any number of cases so I don't know that I'm ready to accept this statement without benchmarks demonstrating a problem. Its some incredibly useful magic that sold a lot of people on Plugins.

I'd say we very much could have an interface too. We can't put it on PluginInterface but if the a factory implementation exists, it requires a interface to work so it should be simple enough to provide and let the plugin type's interface implement it.

I agree though we can't really provide a useful default implementation. The very idea of the create method means that there isn't a predictable constructor.

Still noodling on how I fee about it. Its a weird shift in responsibility to putting the factory method on the plugin and just proxying and I'm still considering how it will play out.

Thanks for the review. Just posting a version which does not interfere with the component, but not injecting the container into the manager yet
as this could cause more problems (let's discuss that).

This shows an example in Table.php

This seems like it directly clashes with #1875182: Pass plugin definition to plugin instances instead of discovery object. But the end result for Views looks pretty nice...

+++ b/core/lib/Drupal/Core/Plugin/Factory/MethodFactory.php
@@ -0,0 +1,24 @@
+  public function createInstance($plugin_id, array $configuration) {
+    $plugin_class = static::getPluginClass($plugin_id, $this->discovery);
+    return $plugin_class::create(drupal_container(), $configuration, $plugin_id, $this->discovery);
+  }

The corollary to all of this is that the factory must become container aware, so that it can pass in $this->container rather than drupal_container().

We absolutely can add an interface that has the create() method on it, since that has a fixed signature. And we should, for consistency with controllers and to improve debugging. (So PHP will fatal on you in a useful place if you screw up.)

Please, DO NOT put the factory in the container, or try some other methodology for making it "container aware". The manager is already in the DIC, and the manager should be able to pass the container to the factory if the container is injected into the manager, so while that needs a follow up, drupal_container() in the factory for the time being should suffice.

I will, as I have with all other plugin related things I've objected heavily to, write up a manifesto as to what's wrong with this whole approach tomorrow, but I've lost any hope of stopping it.

Eclipse

The only way to go forward with this is to call drupal_container/Drupal::getContainer in the factory. Anything else will invite bikesheds about the plugin system and what is or isn't kosher.

Yeah at the moment the plugin system doesn't enforce anything, so something like an interface is not needed,
though what about providing an interface without a required method, which would somehow document that this class supports create().

How can we make progress on this issue now?

Sorry for not providing an interdiff, but sometimes this tools simply hates me.

+++ b/core/lib/Drupal/Core/Plugin/Factory/MethodFactory.php
@@ -0,0 +1,24 @@
+    return $plugin_class::create(drupal_container(), $configuration, $plugin_id, $this->discovery);

Here and elsewehere: Does it make sense to directly pass the definition instead of the entire discovery?

IMO, create() should get as much information as possible; the constructor should get the bare minimum necessary. It's create()'s job to figure out what that bare minimum is.

We've converted to passing the plugin definition instead of the discovery object for the rest of the plugin system just... 2 days ago I think, so probably yes.

Oh that's cool! Rerolled the patch against plugin_definition.

Interdiff failed.

+++ b/core/lib/Drupal/Core/Plugin/Factory/MethodFactory.php
@@ -0,0 +1,25 @@
+    return $plugin_class::create(drupal_container(), $configuration, $plugin_id, $plugin_definition);

drupal_container() is already deprecated in favor of Drupal::getContainer(), if you need the raw object. But even then, if createInstance needs the container (and it does if we're going this route) then the factory object itself needs to be container aware. External static calls inside a method are a no-no.

Which is 100% injectable from the Manager if only we passed the manager to the factory per the Plugin Component specifications. No magic necessary. Also, we largely have an agreement at this point that we'll be removing most of the decorators and reverting to this. Is it contentious to suggest this patch be the first to re-adopt that approach? If so, that's fine I won't push it, but I think we have a basic agreement that we're going back this direction per #1903346: Establish a new DefaultPluginManager to encapsulate best practices

Eclipse

I'm not really happy about this change, so I'm wondering whether the classes should be implement the container aware interface, or just get the container directly in the constructor.

We shouldn't have to implement container aware interfaces at all here. I was expecting constructor injection for the manager and the factory.

Eclipse

Sure I could fix that, though this sadly doesn't help to actually fix a "fundamental" issue with injection in some places: form state.
Form state serializes the objects (like an entity, like a executed view) in order to rebuild the form later. This works fine, as long nothing in the object actually tries to store keep a db connection, which is the case here: The display bag get's the plugin manager injected, which then, since this patch, has the container, booom :(

So again, what do you think about using #61
It's the approach which works. Storing the container in the factory directly has all kind of different issues.

+++ b/core/lib/Drupal/Core/Plugin/Factory/MethodFactory.php
@@ -0,0 +1,24 @@
+    return $plugin_class::create(drupal_container(), $configuration, $plugin_id, $this->discovery);

This makes #61 not viable. Calling a function (or now a static method of the Drupal class) from inside a method is a no-go. We need some other way to get the container into the factory than calling out to a global.

@Crell
Well inject it somehow is easy, though once it's stored, form state might have a problem, but yeah maybe we can work around that,
see some effort of chx.

Updated the patch in 65 to use Drupal::getContainer().

+++ b/core/lib/Drupal/Core/Plugin/MethodFactoryPluginBase.phpundefined
@@ -0,0 +1,40 @@
+ * Contains \Drupal\Core\Plugin\MethodFactoryPluginBase.
+ *
+ * @todo That's a horrible name.

lol

I think this is major?

Until someone proposes a new way to get the container into the factory, this is the best we had. The major win is that we're containing the hacky drupal_container() call to one place where its documented that its happening.
And, the other code changes here won't need to change.

+++ b/core/lib/Drupal/Core/Plugin/MethodFactoryPluginBase.phpundefined
@@ -0,0 +1,40 @@
+ * Defines a base plugin that can pull it's dependencies from the container.
+ */
+
+class MethodFactoryPluginBase extends PluginBase {
+
+  /**

its, not it's. Also, no blank line

+++ b/core/lib/Drupal/Core/Plugin/MethodFactoryPluginBase.phpundefined
@@ -0,0 +1,40 @@
+   * @return static

Is that a normal way to denote this? If so, good to know, we should do that more.

+++ b/core/modules/views/lib/Drupal/views/Plugin/ViewsPluginManager.phpundefined
@@ -35,7 +36,7 @@ public function __construct($type, array $namespaces = array()) {
-    $this->factory = new DefaultFactory($this->discovery);
+    $this->factory = new MethodFactory($this);

I'm not trying to rehash this again, but can we get a comment explaining why we're passing the manager here?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/style/Table.phpundefined
@@ -59,6 +63,30 @@ class Table extends StylePluginBase {
+  }
+
+
   protected function defineOptions() {
     $options = parent::defineOptions();

Extra new line

Thanks for the review!

its, not it's. Also, no blank line

Good catch.

Until someone proposes a new way to get the container into the factory, this is the best we had. The major win is that we're containing the hacky drupal_container() call to one place where its documented that its happening.
And, the other code changes here won't need to change.

Thanks for pointing this out.

Is that a normal way to denote this? If so, good to know, we should do that more.

Well I wouldn't say that this is a standard, though it at least works for phpstorm to actually understand what's going on. I'm pragmatic here,
because just @return MethodFactoryPluginBase tells you not really a lot.

I'm not trying to rehash this again, but can we get a comment explaining why we're passing the manager here?

I understand the difference, though I don't know how to describe it. Do you have an idea?

The best reason to be passing the manager here is if you ultimately intend on injecting the container into the manager from the DIC. If you do, then I'd keep passing the Manager. If you don't, I'd still keep passing the manager, but that's just me :-)

Eclipse

What about ContainerAware(Factory|PluginBase)?

Tagging.

#81: drupal-1863816-81.patch queued for re-testing.

Related issue #1947536: Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service
Breadcrumbs are going to be managed as plugins so they need a clean way to access container

injection of individual services (or groups of services) into a plugin manager (and thus its plugins) is really trivial. The ONLY use case for the code in this particular patch is for plugin systems that have an unpredictable set of services, and even then a hook of some sort in the specific implementation should likely make this doable. This is really designed to take care of some things in views. I'd prefer we not go around applying this to everything under the sun. It's a complete misuse of the system and I'm only not fighting it here because of the limited use it should see.

Eclipse

The ONLY use case for the code in this particular patch is for plugin systems that have an unpredictable set of services

Such as almost every block plugin.

No, most block plugin's needs should be able to be anticipated. I have no intention of going the route this patch offers. As far as I'm concerned this is a views thing.

Eclipse

@EclipseGc
Well, at least breadcrumbs, formatters/fields/etc. and views need them, so it doesn't feel to be just a views thing.
The patch at least doesn't make it the standard, so people can choose what they need.

Right, and I'm ok with saying "our plugin type is SO locked into core's use case that tying it to the DIC is our simplest solution". I'm not ok with suggesting this route to every plugin type because there are other solutions, and you should really prove that your needs match this use case. Not use this as the hammer to beat plugins into submission. But using this approach does hard bind you to a very narrow use case, and that's not necessarily beneficial.

Eclipse

What about:
- A plugin manager can have more than one plugin factory.
- Modules can have their own plugin factories injected into the plugin manager via DIC compiler pass.
- The plugin factories are services, so they can have the dependencies of the plugins they want to create.

Details:
- We need some kind of mapping to decide which factory can create which plugin.
- Instead of annotations on the plugin itself, we could let the factory declare which plugins it can create.

Problem:
As soon as the DIC creates the plugin manager, it will create all the dependencies of all plugins that could ever be instantiated. E.g. on the block plugin manager, even if you only want one specific block, the DIC would create a huge number of services that are needed by some exotic block.

One solution would be to make those factories container-aware.
But.. we should avoid container-awareness as much as possible, imo.

As soon as the DIC creates the plugin manager, it will create all the dependencies of all plugins that could ever be instantiated. E.g. on the block plugin manager, even if you only want one specific block, the DIC would create a huge number of services that are needed by some exotic block.

Okay great, that's not usuable at all.

One solution would be to make those factories container-aware.
But.. we should avoid container-awareness as much as possible, imo.

Which is sort of the solution which is provided already by this patch, isn't it? The details are a bit different, but we can't avoid that at the moment.

+++ b/core/lib/Drupal/Core/Plugin/ContainerAwarePluginBase.phpundefined
@@ -0,0 +1,39 @@
+ * @todo That's a horrible name.

I think if we remove this comment, we're pretty much good to go

+++ b/core/lib/Drupal/Core/Plugin/ContainerAwarePluginBase.phpundefined
@@ -0,0 +1,39 @@
+use Drupal\Component\Plugin\PluginBase;
+
+use Symfony\Component\DependencyInjection\ContainerInterface;

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/style/Table.phpundefined
@@ -7,10 +7,14 @@
+use Drupal\Component\Plugin\Discovery\DiscoveryInterface;
 use Drupal\views\Plugin\views\wizard\WizardInterface;
 use Drupal\Component\Annotation\Plugin;
 use Drupal\Core\Annotation\Translation;
 
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Symfony\Component\HttpFoundation\Request;

No blank lines between use statements

Fixed these points.

+++ b/core/lib/Drupal/Core/Plugin/ContainerAwarePluginBase.php
@@ -0,0 +1,37 @@
+class ContainerAwarePluginBase extends PluginBase {

Yes, this is a terrible name. :-) Specifically, because the object that results is NOT ContainerAware. This is a default implementation of a particular factory method; no more, no less. "ContainerAware" should refer to the object, not the class.

Instead, call these things "ContainerFactory" or something like that.

To be clear, I have no complaints about the patch we're discussing here. My comments should not be interpreted as wanting to prevent this in any way.

Eclipse

@EclipseGC
Thanks for making it clear!

New names: ContainerFactory and ContainerFactoryPluginBase.

I think this is a step forward. Once more plugins start this we can regroup and improve if needed.

#96: drupal-1863816-96.patch queued for re-testing.

docs nitpicks

Committed 9c74b54 and pushed to 8.x. Thanks!

I think that the plugins change notice needs updating.

woot!

Awesome - thanks everyone involved here!

I'd have a question though: Should the static create method be part of the plugins interface? Considering conditions as example, every condition plugin would have to implement in order to be instantiated. Thus, create would have to be part of the ConditionInterface?

@EclipseGc was quite strongly against making that a requirement. Which I think makes sense, it would be weird if you don't need any services, for example.

Don't we do the same with controllers, for instance?

+    return $plugin_class::create(\Drupal::getContainer(), $configuration, $plugin_id, $plugin_definition);

Isn't this something we should avoid?

Crell (#66)

drupal_container() is already deprecated in favor of Drupal::getContainer(), if you need the raw object. But even then, if createInstance needs the container (and it does if we're going this route) then the factory object itself needs to be container aware. External static calls inside a method are a no-no.

And even if we would somehow inject the container into the factory in a clean way, this would still give the factory full access to all of the container, making it less predictable.

I was thinking about an alternative like this:
#1973618: DIC: Lazy instantiation of service dependencies (ProxyManager for "proxy services")

Using Conditions as an example here.

I'm currently working on a ContextBag locally that will be available per implementation of plugins that need contexts. It's the implementation surrounding a plugin use case's job to get and provide the appropriate contexts. As that relates to this, my RequestPath condition I'm working on right now need the request object. It's the implementation's job to make sure it has the request object and can reliably pass it to the plugin that needs it. This isn't for the plugin to decide. If we do that, the plugin type becomes completely dependent on Drupal Core's specific use case and would require hijacking a whole service to change (or altering the plugin, though really very similar results either way).

In short the plugin implementation should have the opportunity to provide any dependencies it wants. The plugin context system is robust enough to handle this, the only time I'm ok with a plugin type doing what this patch provides is when it is so hard coded to core's assumptions that it won't matter.

I hope that make sense.

Eclipse

As that relates to this, my RequestPath condition I'm working on right now need the request object.

To me plugin contexts and dependent services are two different bills. I agree with what you described for plugin contexts, but if a condition would need the typed data manager we would not want to declare that as required context but just make sure the appropriate service gets injected somehow. For that I'd use the ContainerFactory approach of this issue.

@EclipseGc was quite strongly against making that a requirement. Which I think makes sense, it would be weird if you don't need any services, for example.

Well, if a plugin - say conditions - make use of the ContainerFactory it calls the static create method without any additionally checks, thus makes it a requirement for condition plugins. Consequently, the requirement should be denoted in the ConditionInterface as without the method my condition won't work - not?

The plugin system supports raw class contexts which is functionally dependency injection in virtually every way. The only difference here is where we satisfy the dependency. Instead of tightly coupling the plugin to the container (which is what the above solution is) we allow for a layer that has the opportunity to solve a specific implementation's dependency problems. I'm working through code for this right now. I think it'll be pretty reasonable,.

The system in this patch provides a static create method that allows the manager/factory to instantiate the plugin with it's specific dependencies. That's great but also not really because while calling create() is optional for the individual plugins, to actually make use of this fact and provide different dependency resolution you'd have to override the whole manager (which would mess up other implementations) or you'd have to provide a different plugin class, which has a different create() method, which has the same side effects, or you have to build a new plugin manager that just does it differently and doesn't call create() in which case you're right back to where you started before this factory. In short this factory is ONLY useful for plugin types that can be tightly coupled to the container, if you have ANY other need, you're going to find yourself in trouble in certain use cases, and I'd just prefer actually solve THAT problem. Like I said elsewhere in this issue, I'm fine with this as a solution for views. There are perhaps a handful of other plugin types this might be ok for as well, but ultimately it's sort of lazy in a bad way because it assumes you know everything this plugin type could ever be used for, and that's likely untrue.

I know for certain that it's untrue for conditions and blocks. These are plugin types that are meant to be reused outside core's specific case. That alone means they need extra consideration, and I suspect they are not alone as plugin types in this regard.

Eclipse

These are plugin types that are meant to be reused outside core's specific case.

While I appreciate the enthusiasm to fix that in a better, more re-usable way, it doesn't change that the current ContainerFactory depends on the static create method. Thus *if* gets used, it should be in the interface?

right, but I have no intention of using it for Conditions because I intend on solving the bigger problem there. Hopefully that makes sense.

Eclipse

EclipseGc, Fago (#107)

To me plugin contexts and dependent services are two different bills. I agree with what you described for plugin contexts, but if a condition would need the typed data manager we would not want to declare that as required context but just make sure the appropriate service gets injected somehow. For that I'd use the ContainerFactory approach of this issue.

I discussed this on IRC with EclipseGc, and then gave it some more thought.
I think (and this is exactly what Fago says) we really have two separate concerns here: Plugins can depend on services, and they can depend on contextual stuff. The contextual stuff can be satisfied in different ways depending on the configuration of the particular plugin instance. But there are still service dependencies that are supposed to always be satisfied with a particular service. (unless we are in a test)

Each of those needs their own solution.
In this issue we focus on plugins' dependencies on services from the DIC.
Contextual stuff is a valid concern, but can be dealt with elsewhere.
Inventing a system for contextual stuff is a nice idea, but then using it for plain service dependencies would be wrong, imo.

@rootatwc (#99)
Maybe I missed a thing, but.. why can't the factory and the manager have the container injected?

class MyPluginManager {
  function __construct($container) {
    $factory = new ContainerAwarePluginFactory($this, $container);
    [..]

This still has the flaw of dealing with the explicit container, but it is better than pulling it from global state via \Drupal::getContainer().

@yched,

Yes other than the part about reusing outside of Drupal. Really it's just about reusability at all.

Eclipse

re #112
I am probably the wrong person to ask..i just fixed some docs...
speaking of, i messed up:P
followup:
#1974380: Wrong inheritdocs in \Drupal\views\Plugin\views\style\Table and \Drupal\Core\Plugin\Factory\ContainerFactory

In general, it sounds to me like donquixote is on the right track here.

Dependent SERVICES (which are logic objects) should be injected via create()/__construct().

Dependent CONTEXTS (which are data objects) should be injected however the context system is doing that.

That fully supports reusability, with the caveat of create() resulting in a dependency on the DIC (as previously noted). But something somewhere needs to have the services to inject.

#116
And one plugin can depend on some of each.

I think part of EclipseGc's idea is that a plugin class itself should not make that distinction. It just depends on objects no matter where they come from.
Instead, the disctinction should be made in the thing that instantiates the plugin and binds it to the Drupal site.

Still, I don't think that the "context bag" should handle depended-on services.

The remaining flaws I see with the current approach:
- Explicit \Drupal::getContainer(), instead of having the container injected (#112). This can be fixed, imo.
- Static factory in the plugin class, instead of a standalone factory object. (#90 + #105) (*) I'm afraid if we would try to change that we'd want to reinvent large parts of the plugin discovery. So we don't.
- Explicit use of the container, instead of "lazy dependency instantiation" / "proxy services". (#105) (*) We are far away from that, so it is probably not going to happen.

((*) The idea: Modules provide services which act as plugin factories. Plugin factories are registered to the plugin manager via tags + compiler pass, but the manager does only receive proxies (placeholders) of these factories. Whenever a plugin is to be created, the plugin manager asks the respective factory proxy to do the job. This triggers instantiation of the "real" factory and all its dependencies, which then creates the plugin.
Again, I don't see this happening, and do not seriously suggest this atm.)

I'm sort of done with this conversation at this point. I made it very clear I was ok with this code so long as it wasn't force on every plugin type. The discussion that is happening now is attempted to do EXACTLY that, and I have way too much work to do to argue the semantics of this. Depending on the DIC is tightly coupling a particular plugin to the Drupal implementation. This is a thing I don't want to do. If you disagree in principle with me say "the context system is robust enough to do this" then whatever I guess, none of that debunks the fact that I'm very opposed to tightly coupling the individual plugins to specific services. That is the responsibility of another layer of the system, regardless of implementation, and again while the create() method in this approach makes injecting different services to a plugin as dependencies a technical reality, it is not a practical one for all the reason I mentioned in #108.

I'd prefer to not spend more Scotch time, defending the plugins I'm trying to use, from this approach. If we can't agree to that, then I guess we'll waste more of the very little time we have left on this topic, but that's quite discouraging. Please don't do that to me.

Eclipse

The whole point of this issue is that "the thing that instantiates the plugin and binds it to the Drupal site" (the plugin factory) cannot know what services each and every individual plugin (some of them living in contrib / custom code) needs. Only the plugin itself knows that.

Actually the plugin doesn't know that either. Only the implementation around this plugin's use case knows that. You may have a set of plugins that use db connections, but if you have more than one db connection available, and that plugin is referencing the service for the primary default/default, then you're now SOL if you wanted to reference a different database connection because to do so, you'd have to do one of the following:

1.) Extend the old classes, and provide a second set next to them with an overridden create method to the new db.
2.) Override the old class with a new class and screw any system using the old classes
3.) Provide a new plugin manager that uses a method other than create() to instantiate these classes (same approach different static method) with references to another db_connection which also implies that you have to extend every plugin and alter in the new classes over the old.

Again, I can totally see how some plugin types may never need to have this level of ability to swap things out, but this approach is tightly coupling the plugin to the DIC, and that's a bad thing for anything that intends on being moderately generic. Providing a layer above the manager that tracks and provides contexts and dependencies and offers them up to be altered, or whatever is a good move here and is implementation specific (reusable approach) and means that you could provide more things to solve the same dependency, so my aforementioned issue with the database connection vanishes because there's no tight coupling here.

Again my only stipulation on this code was ever "do what you need, but don't force it on me". I will continue to maintain that stand point.

Eclipse

but I guess we don't expect all systems using plugin types to be mini-SCOTCH systems ?

No, of course I don't expect that, which is exactly why I was ok with this code going in, again with the caveat that it not be forced on everyone.

OK, swapping a service for a different service isn't possible if the list of service ids is hardcoded in the plugin, as is the case with ContainerFactory & create(). I'd question whether supporting swappability of any arbitrary dependency of a plugin is a reasonable expectation.

Ok, we're starting to get closer to the same page here now. So if they're not swappable, why are we injecting them? As I've illustrated a few times before injecting anything that's NOT in the DIC (even for testing purposes) is going to require so much setup as to make the test irrelevant. At this point you could have called out to the \Drupal::service() method and gotten the exact same tight coupling and behavior and not had to completely change how the plugin was instantiated. The only reason to go to this level of effort is if they ARE swappable (the database connection example is kind of the canonical example for the dependency injection argument and we're straight up saying "oh you can't do that" here so...).

So... either:

1. They're swappable, and we discuss the agenda I've been pushing for my own plugins.
2. They aren't swappable, and you all keep this for your own uses and don't force this thing I don't want on my plugin types.
3. They aren't swappable, and you all back this out and revert back to straight \Drupal::service() calls.

I don't really care which of these happens, again so long as no one pushes this on me, I'll just pretend it doesn't exist.

Eclipse

Okay, here is a follow-up patch.

It adds an interface that any plugin is free to implement.
It is good to have options, lets not be any more restrictive than we need to be.

@donquixote suggested we check for the interface in the factory.
Otherwise every single plugin for your plugin type must have a create() method.

Yep. I support this.
This gives more substance to the "It is good to have options".
Patch looks nice.
(but I leave it to others to rtbc)

I am still not happy with \Drupal::getContainer() in ContainerFactory.
Follow-up: #1975302: Plugin factories should not depend on $discovery. ContainerFactory can become a service.

+++ b/core/lib/Drupal/Core/Plugin/ContainerFactoryPluginInterface.phpundefined
@@ -23,15 +22,13 @@ class ContainerFactoryPluginBase extends PluginBase {
+  public static function create(ContainerInterface $container, array $configuration, $plugin_id, array $plugin_definition);

This method, in order to really fulfill the use case that's being asked for here, should not define anything other than the $container parameter. Any additional plugin type specific parameters that are passed here probably should be plugin type specific, and the interface that corresponds to that plugin type can extend the method to show what additional parameters are relevant within that use case. Otherwise we're clearly stating that this interface only works with classes that are extending PluginBase.

+++ b/core/lib/Drupal/Core/Plugin/Factory/ContainerFactory.phpundefined
@@ -14,12 +14,19 @@
+
+    // If the plugin provides a factory method, pass the container to it.
+    if (is_subclass_of($plugin_class, 'Drupal\Core\Plugin\ContainerFactoryPluginInterface')) {
+      return $plugin_class::create(\Drupal::getContainer(), $configuration, $plugin_id, $plugin_definition);
+    }
+
+    // Otherwise, create the plugin directly.
+    return new $plugin_class($configuration, $plugin_id, $plugin_definition);

I think I'd rather this throw an exception if it doesn't implement that interface. Most other factory related stuff does something similar if it gets something it doesn't expect to get.

Eclipse

Tim talked to me about my second point. I retract that, since the use case is to not force all plugins of a type to implement the interface. Not what I was expecting, but also not opposed. I think we need to discuss the interface a little further yet though.

Eclipse

OK, swapping a service for a different service isn't possible if the list of service ids is hardcoded in the plugin, as is the case with ContainerFactory & create(). I'd question whether supporting swappability of any arbitrary dependency of a plugin is a reasonable expectation.

Well, making the dependency of a plugin explicit is a big win beside swappability. I agree that this is not for making things configurable like the used database connection or say the filter format to use, it's for making dependencies between objects and thus systems explicit. It helps to untangle variously inter-connected systems and makes those connections *explicit* and even enables us to re-wire it to other systems fulfilling the same interfaces, to mock those systems and to make things easily unit-testable. (I think I just repeated crell's message once again :-)

That static create method gives us all that as long as we use it properly, i.e. make dependencies explicit and use create() only to fulfill them.

ok, then
1) The inheritdocs were in another issue and committed #1974380: Wrong inheritdocs in \Drupal\views\Plugin\views\style\Table and \Drupal\Core\Plugin\Factory\ContainerFactory
2) Lets retitle this
3) Next time lets open followups, we are already at #130 here, this makes this confusing for all of us

Checking whether a plugin actually has the create method is nice. +1 for an interface

Rerolling. We had a plugin discussion today and the lack of a ContainerFactoryPluginInterface came up, and a couple people even thought we already had one.

So every plugin which has a different

+++ b/core/lib/Drupal/Core/Plugin/ContainerFactoryPluginBase.phpundefined
@@ -4,31 +4,19 @@
+abstract class ContainerFactoryPluginBase extends PluginBase {

We should also impement that interface.

Duh, thanks.

Heh, I uploaded the same patch twice. The interdiff was correct.

Let's wait on the testbot.

There we go.

#135 is committed so back to change notice.

Change notice is here https://drupal.org/node/2012118

Looks good, suppose needs review someone from VDC

Thanks! I tweaked it, but no substantial changes (added code tags and fixed indentation)

https://drupal.org/node/2012118/revisions/view/2715768/2715842

Is https://drupal.org/node/1938688 needs an update?