I'll try to summarize the situation as I get it. Forgive the possible poor wording, and feel free to correct / edit :-)

Drupal has a DIC, and a drupal_container() functional wrapper to access it when it is not injected in the current scope.

The established best practice is to avoid using drupal_container() whenever possible, and instead;
- Inject the container in the client object that needs to access service objects from the container
The code in the client object then does $service = $this->container->get('some.service');
That is using the container in a "service locator" style.
- Directly inject the service object into the client object (i.e fetching the service from the container *outside* the client object, the client object is not aware there's such a thing as a centralizing container object)
That is using the container in a "real dependency injection" style, and, if I get things right, is preferred when possible.

More and more, and rightly so, patches that add drupal_container() calls within OO methods raise "we should make that injectable" comments.

Problem : our main mechanisms for creating objects (the Plugin API & the Entity API) do not allow either of the above, because they use generic factories that are not container aware. Right now, I need something from the DIC in an object that's either an entity (say, a config entity) or a plugin implementation, I have no other way than calling drupal_container().

I'll focus on the Plugin API here.

Possible approaches :

1) Add a ContainerAwareFactory, that receives the container from the plugin manager (no problem here, the plugin manager, being exposed in the DIC, can receive the DIC itself) and passes it to instantiated plugin instances.
This only allows plugin classes to use the container in a "service locator" way. No direct injection of the actual objects.

2) Allow real dependency injection in plugin classes ? Note that the plugin manager or factory cannot predict which DIC entries a specific plugin implementation class will need.
It seems this means having a way for plugin classes to expose which DIC entries they need - in their metadata ? in a static method ?
Then the ContainerAwareFactory can read this info, fetch the corresponding objects from the DIC, and pass them to the plugin constructor.

3) other proposals ?

Discuss :-)

Files: 
CommentFileSizeAuthor
#135 container-1863816-135.patch4.55 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,607 pass(es).
[ View ]
#133 container-plugin-interface-1863816-133.patch4.51 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,914 pass(es), 74 fail(s), and 102 exception(s).
[ View ]
#133 interdiff.txt598 bytestim.plunkett
#131 container-plugin-interface-1863816-131.patch4.51 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#124 plugin-1863816-124.patch5.27 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 54,459 pass(es).
[ View ]
#124 interdiff.txt1.08 KBtim.plunkett
#123 plugin-followup-1863816-123.patch4.67 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 54,453 pass(es).
[ View ]
#99 drupal-1863816-99.patch7.14 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 54,433 pass(es).
[ View ]
#99 interdiff.txt1.5 KBParisLiakos
#96 drupal-1863816-96.patch7.26 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 54,576 pass(es).
[ View ]
#93 drupal-1863816-93.patch7.25 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 54,295 pass(es).
[ View ]
#93 interdiff.txt1.01 KBdawehner
#81 drupal-1863816-81.patch7.3 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 54,556 pass(es).
[ View ]
#78 drupal-1863816-78.patch7.29 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 54,035 pass(es).
[ View ]
#78 interdiff.txt990 bytesdawehner
#75 drupal-1863816-75.patch7.29 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 54,001 pass(es).
[ View ]
#75 interdiff.txt775 bytesdawehner
#68 drupal-1863816-68.patch10.86 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 53,260 pass(es), 41 fail(s), and 3 exception(s).
[ View ]
#68 interdiff.txt4.78 KBdawehner
#65 drupal-1863816-65.patch7.29 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 53,332 pass(es).
[ View ]
#61 drupal-1863816-61.patch7.41 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 53,200 pass(es).
[ View ]
#54 drupal-1863816-54.patch7.45 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 52,564 pass(es).
[ View ]
#52 drupal-1863816-51.patch4.19 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 52,121 pass(es), 27 fail(s), and 5 exception(s).
[ View ]
#49 drupal-1863816-48.patch3.37 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 52,583 pass(es).
[ View ]
#35 plugin-DIC-inject-1863816-35-do-not-test.patch16.05 KByched
#35 interdiff.txt1.4 KByched
#33 plugin-DIC-inject-1863816-33-do-not-test.patch15.23 KByched
#13 plugin-DIC-inject-1863816-13-do-not-test.patch11.28 KByched
#12 LosoDiAnnotationsBundle_sample.php_.txt1.17 KByched

Comments

fubhy’s picture

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.

tim.plunkett’s picture

Issue tags:+VDC

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

yched’s picture

The problem with 1) is that the net result is having an awful lot of objects referencing the DIC directly.
Then suddenly those objects can't serialize (last time I checked, the DIC was not serializable because it's based on closures). And tracking what's referenced where and will get serialized when is a hair-pulling mess.

In the past couple months, we've added the ability for (some, not all, but that inconsistency is another issue) FAPI / render #callbacks to accept methods on objects. Then you find some plugin code doing $form['#pre_render'][] = array($this, 'myMethod');.
Boom. FAPI will serialize the form and hit a fatal because $this can't serialize if $this->container holds a reference to the DIC.

I've advocated to limit that "methods as FAPI / render #callbacks" feature to static methods only, but to no avail so far :-). I'll start that battle again at some point, but that's just an example that the serialization chain can be leaky.

RobLoach’s picture

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.

fubhy’s picture

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?

fago’s picture

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.

fubhy’s picture

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.

yched’s picture

@fago / @fubhy: Right, the plugin manger and the factory cannot know which services an arbitrary plugin class will need.

They cannot even know *whether* some plugin classes will need services from the DIC or not, so, strictly speaking, it shouldn't even be a task for the plugin manager to take special measures to allow for this, because if it fails to do so, 3rd party plugins are screwed :-/.
It's a bit like : "if the plugin manager didn't specifically include a DerivativeDiscoveryDecorator, then 3rd party is screwed and cannot use derivatives".

Crell’s picture

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.

yched’s picture

Registering plugin classes themselves in the DIC sounds like it would defeat the whole discovery mechanism in Plugin API.
And I'm not sure why we'd need to have them in the DIC ? We're not relying on the DIC to instantiate the plugin objects for us.

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

Is that really different from what we currently do in the DIC itself ?

<?php
$container
->register('some.service', 'Some\Class')
  ->
addArgument(new Reference('some.other.service'));
?>

similarly "hardcodes" the 'some.other.service' entry to be used for the 'some.service' service ?

If plugin classes specified the DIC entries they want to use (in their annotations or something), it's just saying "give me whatever is configured in the DIC for those services". Would that be bad ?
That's what they do currently by being forced to call drupal_container()->get('some.service.I.cannot.get.injected.for.now'); at some place in their own code.

which means that, say, swapping out the DB connection for a slave connection is difficult or impossible or impractical.

Would it ?
- For testing purposes, you can just give the plugin manager a mock container with the right mock services - then plugins would be created using the services in that mock container. Isn't that how we'd do for non-plugin stuff ?
- Other than that, for regular runtime code, the plugins use whatever is configured in the DIC, as they would if they were registered in the DIC directly ?
- What this doesn't allow is : I want to instanciate a plugin but switch the db.connection that this plugin will use to a slave connection just for this once. Do we want to do this ?

Crell’s picture

"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.)

yched’s picture

Status:Needs review» Active
StatusFileSize
new1.17 KB

So looking a bit in Symfony land, I found this PR:
https://github.com/symfony/symfony/pull/1412
Which was closed in favor of a couple 3rd party bundles :
- AutowiringBundle
- InjectorBundle
- LosoDiAnnotationsBundle (doc)
- JMSDiExtraBundle (doc)
The first two do not qualify themselves as "production ready", and link to the other two, LosoDiAnnotationsBundle & JMSDiExtraBundle, so I mostly looked at those two.

The scope of those is strictly to allow exposing classes as service entries in the container through annotations in the class file rather than through the 'regular" mechanisms (PHP/ContainerBuilder, configuration files). Thus, they consist of compiler passes that participate in the final compiled DIC. (JMSDiExtraBundle has a slightly wider scope, supports using @inject annotations in service classes *and* in Controller classes)

As was anticipated, this is largely different from our use case though. The plugin API does not register plugin implementations in the DIC, and plugin instances are not created through DIC->get(), so our target is not putting stuff in the DIC.

So it seems what we can look for here is inspiration with syntax at best (both bundles are fairly close syntax-wise, see attached example).
But even there, the fact that the logic is not targeted to run at compile time rules out any reflexion-based wiring, which restricts the syntactic possibilities.

Working on a proof-of-concept patch, will report back asap.

yched’s picture

Status:Active» Needs review
StatusFileSize
new11.28 KB

Proof-of-concept patch - only takes care of TestPluginManager / MockUserLoginBlock used by FactoryTest.

  • Relies on a 'dependencies' entry in the plugin definition - i.e in most cases in annotations :
      'dependencies' => array(
        'service_1' => 'test_plugin_manager.service_1',
        'service_2' => 'test_plugin_manager.service_2',
      ),
  • The PluginManager's __construct() receives a $container, and passes it to the factory object.
    Defaultfactory::createInstance() parses the 'dependencies' entry, fetches the corresponding objects from the $container, and passes the resulting $dependencies array to the plugin constructor.
  • PluginBase::__construct() does foreach ($dependencies as $name => $service) {$this->{$name} = $service;}

TODOs:

  • Make all existing plugin managers receive a $container and pass it to their factory (currently only TestPluginManager does it)
  • Make all factories process 'dependencies' and pass the resulting $dependencies array to the plugin constructor - or maybe use a separate PluginBase::setDependencies() setter method ? Might be easier on implementations.
  • Does it mean managers and factories should implement ContainerAwareInterface ?
  • Support a syntax for injecting container parameters in addition to services
  • Support a syntax for "NULL if not found" (aka $container->get($id, NULL_ON_INVALID_REFERENCE))

Issues / TBD:

  • Agree on the location for the 'dependencies' information.

    - In the plugin definition (i.e @Plugin annotation) - current patch:
    Pros : Alterable, along with the whole of the definition. And the alterability happens below cache point, no runtime CPU impact.
    Cons: No inheritance. if MyWidgetPlugin extends WidgetPluginBase, it probably needs to include the dependencies of WidgetPluginBase in addition to its own. Not doable if the info sits in the plugin definition, child classes do not inherit the "definition" of their parents, and thus need to explicitly list the parent's dependencies.

    - In a static dependencies() method in the plugin class:
    Pros: Allows inheritance
    Cons: Alterability ? Would mean adding an additional explicit drupal_alter($plugin_class::dependencies()) at runtime in the factory, which is problematic performance-wise...

  • PluginManagers hold a reference to the $container (either directly or through their factory), and thus are not serializable anymore.
    This is a problem given #1851706: Make all core plugin managers be consistent as to whether $this or $this->discovery is passed to the factory. In short, some managers currently pass themselves ($this) as the DiscoveryInterface param expected by the factory, some other pass the raw discovery ($this->discovery), and there's a controversy whether core should settle on one or the other.

    This "discovery" object is then passed and referenced within each plugin object, to support PluginBase::getDefinition().

    Problem: If $my_plugin->discovery is the plugin manager, then suddenly $my_plugin is not serializable anymore - and we have the same problem as exposed in #3 above, just as if we went for the "service locator" approach and passed the $container to the plugins directly.

    $form['#entity'] = $node, anyone ? :-)

effulgentsia’s picture

Status:Active» Needs review

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?

yched’s picture

I think it would be nice to somehow add definition inheritance to the plugin system

Hmm, not sure about that one. It would be a forced inheritance, where the child class has no way to remove an entry present in the parent definition ?

effulgentsia’s picture

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.

yched’s picture

#16:
Hah, true. Busted. With the current shape of ProcessDecorator, most plugins reference their manager.

Possible workarounds:
- Support PluginBase::getDefinition() by having the plugin object hold a copy of the definition array rather than a reference to the discovery. I fear the memory impact - but then again, maybe php's copy-on-write behavior would make it a wash.
- Find the mythical "other formulation for processDecorator" that would somehow fix this by making the discovery not reference the manager...

yched’s picture

Support PluginBase::getDefinition() by having the plugin object hold a copy of the definition array rather than a reference to the discovery

I gave this a try, and according to xhprof, there's indeed no memory overhead. I've opened #1875182: Pass plugin definition to plugin instances instead of discovery object for that.

dawehner’s picture

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.

yched’s picture

Thinking a bit more about this in the last couple days - yes, we do need inheritance, and putting dependencies info just in the plugin definition won't fly :
- Most plugin types provide one (or several) base plugin class, which contains active methods whose code might need some services from the DIC. Those classes are, in most cases, not actual plugins themselves (they are often abstract), and thus have no annotation/"plugin definition" to advertise those dependencies.
- Then it would be the job of each actual plugin that chooses to extend a base class, to include the dependencies of that parent class in its own definition. But then you cannot add a new dependency in the base class without breaking existing contrib plugins that rely on it.

So, declaring dependencies through a class member (method or property, TBD) seems the way to go. The base class can expose its dependencies, and child plugin classes can extend the list with their own.
When a plugin is instanciated, the factory processes the list and injects the objects taken from the DIC in the constructor.
Or, maybe, a setServices() method directly receives the $container as a param and does a series of $this->service_foo = $container->get('some.service');. In this case, the method doesn't have to be static, dependencies are collected and injected right after the plugin has been created.

But either way, if dependencies are not in the definition, it's not clear how to make them alterable, as @Crell requested in #11 above (e.g. use 'database.slave' instead of 'database' for this specific plugin class, because "I know its fine on my site").
It seems that means an extra alter hook on the array of dependencies - ran on each plugin instantiation then, since that info is not part of the (cacheable) definition. Performance-- :-(...

Crell’s picture

For some dependencies, interface injection may be sufficient.

<?php
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.

yched’s picture

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

I'm not sure I get the picture. We can't put an entry per plugin definition (e.g per available widget type) in the DIC, because we need to be able to instanciate several instances of the same plugin class. A plugin definition is not a service.
So I guess you're thinking of something else, but can't figure what :-).

Crell’s picture

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?

yched’s picture

What might work would be putting dependencies info for a given plugin implementation as a DIC param (not service).
One entry per available plugin class (not per instanciated plugin), as an array of 'propertyName' => 'service.id'

But:
- memory impact, that's putting a lot of additional stuff in the container, loaded in memory on each request. Are there partial containers ?
- not sure how to get that info in there to begin with. Seems like a lot of extra processing at compile time

Crell’s picture

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...

beejeebus’s picture

i agree with Crell. the memory issue wouldn't be a worry, but the life cycle would be.

i really, really want to avoid having to rebuild the DIC in circumstances other than module enable/disable type events.

if the suggestion in #24 fits within that, then i'm ok with it.

katbailey’s picture

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.

dawehner’s picture

... 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.

yched’s picture

Not sure the "abstract factory" services mentioned by @katbailey would work out with the current Plugin API design, because the Plugin Type Managers themselves are basically already abstract factories :
- $manager->getInstance($options) applies some logic to map $options into a [$plugin_id, $configuration] pair,
- it then calls $this->createInstance($plugin_id, $configuration),
- which in turn proxies to $this->factory->createInstance($plugin_id, $configuration)
And right now, $this->factory is not injected, it's defined by the manager in its own __construct() : the current Plugin API is designed around the idea that the Manager class is auto-contained, defining the plugin type behavior (namely, discovery and factory) in one single file. (at least that's the current design, there are discussions in #1903346: Establish a new DefaultPluginManager to encapsulate best practices about injecting stuff in the manager instead, but no real agreement yet)

So it makes little sense for this->createInstance($plugin_id, $configuration) to fetch an abstract factory for the $plugin_id from the DIC, abstract factory which will only point back to the manager itself, since only the manager currently knows what is the concrete factory.
Also, if the manager is that "abstract factory for plugin_id X" service, we would be creating a new manager object for each different plugin that gets instanciated in the request, right ? Doesn't match what the manager is there for.

The proposal in #24 was to put the dependencies for each plugin in DIC *param* entries (not factories in service entries). Fairly similar, but what we actually need are params ("what are the dependencies for this plugin_id ?"), not a service, we already have it, it's the manager.

Pretty similar impact memory-wise, though, and while this worried me too, Crell didn't seem to be impressed in #25.

Also : about inheritance - yeah, we probably need inheritance, see #20.

EclipseGc’s picture

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

yched’s picture

Component:plugin system» node system

@EclipseGc :

This is basically what the PoC patch in #13 was doing - except that one was putting the dependencies in the $configuration array, thus not forcing the fairly slow Reflectionfactory for any manager willing to allow their plugins to receive stuff from the container.

Also, defining dependencies in plugin definitions means no inheritance, which is what has been discussed from #20 on.
If my plugin B extends some plugin A, then it also probably needs to include plugin A's dependencies.
It then has to explicitly duplicate them in its own definition, but it then means that they'll be out of sync if plugin A's dependencies change in a point release.
Means any plugin in core or contrib is basically frozen on that regard after the initial release : can't add new dependencies, can't point an existing one to a different service id.

This being said,

- The solutions mentioned so far (expose dependency info in a plugin static method rather than in the definition, then collect this data either when compiling the DIC or in an additional "collect + alter + cache" step within discovery) all mean loading plugin classes in PHP space, which defeats the efforts that went in the annotations parser.

- Alternate proposal : have plugin_b put the "extends plugin_a" info in it's definition somehow, and resolve the actual dependencies for each plugin with PHP logic in an additional definitions processing step (pre-cache of course).
That sounds like reimplementing the OO inheritance mechanism on arrays, but we have a graph library, right ?
I don't see how we escape "either we load classes and can use the language inheritance mechanism, or we don't load classes and resolve inheritance ourselves."

- Other possibility : punt and ignore that question of inheritance completely...
I'm not sure what kind of BC policy we can aim for regarding core classes (among which plugins) being potentially extended in contrib, and what constitutes a BC break on those. The question already exists with our classes as they stand right now - what can you change on a plugin class under the assumption that it might be extended in contrib ?

Crell’s picture

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.

yched’s picture

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."

Yeah, that's what the existing "set up injection in annotations" Symfony bundles I looked at in #12 all do. "i want this $object param to receive 'some.container.id'. If it's good enough for them...

So, here's #13 :
- rerolled,
- and with ReflectionFactory adapted in addition to DefaultFactory.

'do-not-test' still, since only FactoryTest pass atm.
Before this can be fed to the testbot, all existing plugin managers and factories will need to be adjusted to receive the Container.
Thus, I'd rather get an agreement on the approach first.

Basically, how this works is :
- As a plugin implementation, you add this to your "definition" (annotations, typically)

      'dependencies' => array(
        'service_foo' => 'some.service',
        'service_bar' => 'some.other.service'
      ),

- behind the scenes, the factories receive the $container, process the info into a $dependencies array containing the objects pulled from the container, and pass it to the plugin's __construct().
- If your plugin subclasses PluginBase, you can directly find the individual injected objects in $this->service_foo and $this->service_bar, that's taken care of by PluginBase::__construct().
- Otherwiise, it's up to your __construct() to do what it sees fit with the $dependencies it receives.

Crell’s picture

+++ 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.

yched’s picture

StatusFileSize
new1.4 KB
new16.05 KB

We need to somehow document (...)

Agreed. But the "how / where" to document this is not too clear to me :-/

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?

Somthing like that could be baked in the ReflectionFactory I guess. Attached patch does that.

The thing is, I'd really like to avoid forcing the use of reflection / ReflectionFactory on every plugin type that wants to allow its plugin implementations to benefit from injection - because that basically means moving all plugin types to ReflectionFactory (on what grounds would a plugin type forbid contrib plugins to ever receive injected dependencies ?), and ReflectionFactory has a non minor performance impact.

yched’s picture

"and ReflectionFactory has a non minor performance impact."
Actually, so much so that I'd feel much more comfortable if it was removed from core, or renamed to SlowAsHellDontUseMeForAnythingElseThanPrototypingFactory or something before we ship :-/

Crell’s picture

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.

yched’s picture

Back when I was working on "field formatters as plugins", I remember hitting a massive perf regression until I figured I was still using the ReflectionFactory "for quick prototyping" instead of a dedicated factory.

Just re-ran some ab tests on a custom page callback that instanciates 200 times the widget plugins for all fields of article nodes.
That's 600 plugin instanciations on a request - fairly big, but not that unlikely given everything and his dog is a plugin now.

HEAD (WidgetPluginManager uses a custom factory based on DefaultFactory):
Time per request: 136.878 [ms]

Switching WidgetPluginManager to use ReflectionFactory:
Time per request: 166.150 [ms]

--> + 20% impact :-/

dawehner’s picture

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?

EclipseGc’s picture

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

Eclipse

yched’s picture

Interesting indeed.

But we probably don't want to add another cache lookup, so this data would be part of the cached definition, processed at discovery time ? But this would then mean loading the plugin classes in php space at discovery time, which the current annotations parser was designed to avoid.

EclipseGc’s picture

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.

EclipseGc’s picture

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

yched’s picture

The other option is to not use reflection and just trust that the constructor has variables in the order that they're in the annotation

But the __construct() might have other params, other than objects injected from the container and listed in the "dependencies" annotation - typically params taken from the $configuration passed to createInstance(). How do we tell which is which ?

yched’s picture

Also, even if we find a way to have dependencies passed as separate params in the constructor (as opposed to a single $dependencies array param, which is what #33 does), you "MUST still define the services as properties on your plugin object", for the very reasons @Crell details in #34.

So in the end I'm not sure why "single $dependencies param" would be so much worse ? Yeah, type hinting and fatal error if the wrong dependency is passed...

EclipseGc’s picture

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

yched’s picture

Proposed by @msonnabaum & @dawehner on IRC :
Have factories create plugins by calling a create() factory method on plugin classes, rather than __construct() directly.

- Each plugin class has a create() static method, which receives the arguments needed for __construct() that are not dependencies from the container (e.g, in the case of TypedData plugins $configuration, $name, $parent - see TypedDataFactory), and the $container
- It extracts dependencies out of the $container, using whatever "$foo = $container->get('some.service.id');" logic it sees fit (it's in a method, so we gain inheritance for "PluginA extends PluginB"), and passes them to __construct().
- __construct() is fully hinted, and can be used to unit-test the class with arbitrary mocked up dependencies. Only, the plugin system doesn't call it directly, but goes through create().
- no need to add anything in the annotations

Simple :-)

yched’s picture

So, signature-wise :
- __construct() methods have different signatures in each plugin class (they hold the dependencies of this specific plugin implementation), that's fine, PHP allows that.
- create(), being the method called by the factory, needs to have the same signature across each plugin in a given plugin type (like is currently the case for __construct()) - that is, unless the plugin type uses a reflection-based factory, but for performance sake we have to consider those as the exception. But it will have different signatures across different plugin types.

So this means that we cannot put create() in a PluginInterface, or provide a base implementation in PluginBase.
It's up for each plugin type to document it in its own FooBarPluginInterface, and provide a base implementation, if applicable, in FooBarPluginBase (for plugins with no dependencies, I guess ?).

No biggie I guess, just sayin'.

dawehner’s picture

StatusFileSize
new3.37 KB
PASSED: [[SimpleTest]]: [MySQL] 52,583 pass(es).
[ View ]

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

EclipseGc’s picture

Status:Needs review» Needs work
+++ 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:

<?php
$this
->factory = new MethodFactory($this);
?>
neclimdul’s picture

Status:Needs work» Needs review

- __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.

dawehner’s picture

Component:node system» plugin system
StatusFileSize
new4.19 KB
FAILED: [[SimpleTest]]: [MySQL] 52,121 pass(es), 27 fail(s), and 5 exception(s).
[ View ]

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).

Status:Needs review» Needs work

The last submitted patch, drupal-1863816-51.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new7.45 KB
PASSED: [[SimpleTest]]: [MySQL] 52,564 pass(es).
[ View ]

This shows an example in Table.php

tim.plunkett’s picture

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...

Crell’s picture

+++ 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.)

EclipseGc’s picture

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

msonnabaum’s picture

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.

yched’s picture

@Crell :

We absolutely can add an interface that has the create() method on it, since that has a fixed signature

It doesn't.
The signature of create() is a contract between the factory used by the plugin type and plugin implementation classes of this type, and can vary depending on the plugin type.
Any given plugin type (i.e manager) can use a dedicated factory (see WidgetFactory or TypedDataFactory).
[edit: typically, that's because you want to extract individual entries from the $configuration array that gets passed by the client code to Manager::createInstance()]

Currently, the factory imposes a signature on plugins __construct() methods, which for this reason, is not in a PluginInterface. With the proposed approach, this will shift to create(), which therefore cannot be in a generic interface - at best in a FooBarPluginInterface for plugins of the FooBar type.

dawehner’s picture

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?

dawehner’s picture

StatusFileSize
new7.41 KB
PASSED: [[SimpleTest]]: [MySQL] 53,200 pass(es).
[ View ]

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

tstoeckler’s picture

+++ 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?

Crell’s picture

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.

EclipseGc’s picture

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.

dawehner’s picture

StatusFileSize
new7.29 KB
PASSED: [[SimpleTest]]: [MySQL] 53,332 pass(es).
[ View ]

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

Interdiff failed.

Crell’s picture

+++ 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.

EclipseGc’s picture

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

dawehner’s picture

StatusFileSize
new4.78 KB
new10.86 KB
FAILED: [[SimpleTest]]: [MySQL] 53,260 pass(es), 41 fail(s), and 3 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, drupal-1863816-68.patch, failed testing.

EclipseGc’s picture

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

Eclipse

dawehner’s picture

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 :(

dawehner’s picture

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.

Crell’s picture

+++ 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.

dawehner’s picture

@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.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new775 bytes
new7.29 KB
PASSED: [[SimpleTest]]: [MySQL] 54,001 pass(es).
[ View ]

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

xjm’s picture

Priority:Normal» Major
+++ 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?

tim.plunkett’s picture

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

dawehner’s picture

StatusFileSize
new990 bytes
new7.29 KB
PASSED: [[SimpleTest]]: [MySQL] 54,035 pass(es).
[ View ]

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?

yched’s picture

-    $this->factory = new DefaultFactory($this->discovery);
+    $this->factory = new MethodFactory($this);

can we get a comment explaining why we're passing the manager here ?

Looks like a leftover from previous iterations where the Manager had the Container. Doesn't seem needed anymore if the Container is pulled in MethodFactory::createInstance() ?

- re: MethodFactory / MethodFactoryPluginBase / "@todo That's a horrible name."
Right :-)
Can't really coin a good name either for now, but I'd tend to think that those class names should hint at the notion of container or services (as in "those are the classes my plugin type needs to use if I want to allow its plugins to receive injected services"), rather than the notion of "method factory", which, from the point of view of consumer APIs, seems more of an implementation detail.
In other words: emphasis what it offers rather than how it works.

EclipseGc’s picture

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

dawehner’s picture

StatusFileSize
new7.3 KB
PASSED: [[SimpleTest]]: [MySQL] 54,556 pass(es).
[ View ]

What about ContainerAware(Factory|PluginBase)?

tim.plunkett’s picture

Issue tags:+Plugin system

Tagging.

dawehner’s picture

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

andypost’s picture

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

EclipseGc’s picture

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

donquixote’s picture

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.

EclipseGc’s picture

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

dawehner’s picture

@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.

EclipseGc’s picture

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

donquixote’s picture

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.

dawehner’s picture

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.

tim.plunkett’s picture

+++ 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

dawehner’s picture

StatusFileSize
new1.01 KB
new7.25 KB
PASSED: [[SimpleTest]]: [MySQL] 54,295 pass(es).
[ View ]

Fixed these points.

Crell’s picture

+++ 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.

EclipseGc’s picture

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

dawehner’s picture

StatusFileSize
new7.26 KB
PASSED: [[SimpleTest]]: [MySQL] 54,576 pass(es).
[ View ]

@EclipseGC
Thanks for making it clear!

New names: ContainerFactory and ContainerFactoryPluginBase.

tim.plunkett’s picture

Title:How to make plugins receive injected objects from the container ?» Allow plugins to have services injected
Status:Needs review» Reviewed & tested by the community

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

tim.plunkett’s picture

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

ParisLiakos’s picture

StatusFileSize
new1.5 KB
new7.14 KB
PASSED: [[SimpleTest]]: [MySQL] 54,433 pass(es).
[ View ]

docs nitpicks

alexpott’s picture

Title:Allow plugins to have services injected» Change notice: Allow plugins to have services injected
Priority:Major» Critical
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

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

I think that the plugins change notice needs updating.

twistor’s picture

woot!

fago’s picture

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?

Berdir’s picture

@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.

Xano’s picture

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

donquixote’s picture

+    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")

EclipseGc’s picture

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

fago’s picture

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?

EclipseGc’s picture

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

fago’s picture

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?

EclipseGc’s picture

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

donquixote’s picture

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.

donquixote’s picture

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

<?php
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’s picture

My understanding of @EclipseGc #106-#108 is that:

- A plugin type that strives for generic reusability outside of Drupal should use Contexts rather than ContainerFactory as the mechanism by which plugins can receive their dependencies, since Contexts allow plugin implementations to not depend on Drupal's own DIC and service ids structure.

- For all plugin types that have no such Drupal-agnostic desire (for example because they are tied to a drupal-specific subsytem anyway - entities, fields, views, form API...), ContainerFactory seems leaner, easier to leverage (?), and consistent with other "factory method" patterns in core ?

EclipseGc’s picture

@yched,

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

Eclipse

ParisLiakos’s picture

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

Crell’s picture

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.

donquixote’s picture

#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.)

EclipseGc’s picture

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

yched’s picture

[edit: sorry, crosspost, the intent is not "doing that to @EclipseGc". Yet, as we write more plugin classes and move more stuff to services, the questions for plugins and plugin types authors are real.]

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.

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. That's the role of the create() method. "I know how to take care of my own dependencies if you give me a Drupal DIC".

Integrating with Drupal services needs to happen *somewhere*. Interoperability and abstraction is fine, but at some point, there needs to be a firm ground somewhere.

Being able to write plugin classes that are drupal agnostic and "just work with (for example) a PSR-3 logger and a Symfony Request" is great. But that's not the case of (rough estimate) 90%+ plugin classes that are going to be written for D8, that will need to work with forms, entities, fields, urls, render arrays, the theme layer, config, state, cache... All those that currently need to do a drupal_container() / Drupal::getSomeService() call to get their job done. They are de-facto tied to Drupal, and that's fine, let's live with it, and let's not make that use case harder.

And the plugin type *can not* decide in advance what is "the reasonable set of services that any and all plugin that is going to be written in the next five years in contrib and custom code will need, ever, period". Hence, the need for each plugin to handle its own dependencies.

So yes, I'd tend to consider any plugin type, other than the few that explicitly target at drupal agnosticity, that doesn't use Containerfactory and the create() factory method, to be a bug.

EclipseGc’s picture

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

yched’s picture

- yched: 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
- EclipseGc: 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

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.

If something is supposed to be swappable, like "the db connection this plugin uses", then it seems this should be an explicit "plugin setting" (in the sense of "formatter settings, widget settings..." and #1764380: Merge PluginSettingsInterface into ConfigurablePluginInterface - and it might very well be that Contexts are a different / richer formulation of exactly the same thing).
For all the other dependencies, just use ContainerFactory and create() ? I mean, ContainerFactory and Contexts aren't mutually exclusive, are they ?

I'm also a bit skeptical about "a layer above the manager that tracks and provides contexts and dependencies and offers them up to be altered", and what that would mean for all the current code leveraging plugin types in the actual subsystems around core : views various handlers, field types, widgets, formatters, image styles...
Does it mean they'd need to reshuffle their code around the places where they currently do $my_manager->getInstance($some_options) to make room for this extra layer ? Plus associated UI impact ?
I mean, this sounds great for SCOTCH because this looks like we already know from Panels, but I guess we don't expect all systems using plugin types to be mini-SCOTCH systems ?

EclipseGc’s picture

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

tim.plunkett’s picture

Priority:Critical» Major
Status:Active» Needs review
StatusFileSize
new4.67 KB
PASSED: [[SimpleTest]]: [MySQL] 54,453 pass(es).
[ View ]

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.

tim.plunkett’s picture

StatusFileSize
new1.08 KB
new5.27 KB
PASSED: [[SimpleTest]]: [MySQL] 54,459 pass(es).
[ View ]

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

donquixote’s picture

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)

donquixote’s picture

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.

EclipseGc’s picture

Status:Needs review» Needs work
+++ 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

EclipseGc’s picture

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

fago’s picture

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.

ParisLiakos’s picture

Title:Change notice: Allow plugins to have services injected» [Followup] Allow plugins to have services injected

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

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new4.51 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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.

dawehner’s picture

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.

tim.plunkett’s picture

StatusFileSize
new598 bytes
new4.51 KB
FAILED: [[SimpleTest]]: [MySQL] 55,914 pass(es), 74 fail(s), and 102 exception(s).
[ View ]

Duh, thanks.

Status:Needs review» Needs work

The last submitted patch, container-plugin-interface-1863816-133.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new4.55 KB
PASSED: [[SimpleTest]]: [MySQL] 55,607 pass(es).
[ View ]

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

dawehner’s picture

Let's wait on the testbot.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

There we go.

jibran’s picture

Title:[Followup] Allow plugins to have services injected» Change notice: Allow plugins to have services injected
Priority:Major» Critical
Status:Reviewed & tested by the community» Active

#135 is committed so back to change notice.

larowlan’s picture

Status:Active» Needs review

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

andypost’s picture

Looks good, suppose needs review someone from VDC

tim.plunkett’s picture

Title:Change notice: Allow plugins to have services injected» Allow plugins to have services injected
Priority:Critical» Major
Status:Needs review» Fixed
Issue tags:-Needs change record

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

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

jibran’s picture

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

Automatically closed -- issue fixed for 2 weeks with no activity.