Allow Plugins to specify Services via Annotation

I discussed this with mpotter separately, and my initial reaction was slightly negative. I was convinced otherwise.

My main objection: "Why is the boilerplate bad? It is straightforward and forces you to think with rigor about your dependencies."

As Mike notes, for developers learning this that haven't already internalized service injection and factory methods, this is not rote typing boilerplate but a series of bizarre incantations. My misty history with Java makes this kind of boilerplate boring but not challenging, playing to my own bias to dismiss the difficulty of climbing this learning curve for what should be a common development task.

The annotation maintains the requirement to think about the dependencies.

1. +++ b/core/lib/Drupal/Core/Plugin/Factory/ServiceFactory.php
   @@ -0,0 +1,42 @@
   +    $plugin = parent::createInstance($plugin_id, $configuration);
   

   If I understand this, we're still creating the plugin using the existing direct use of the constructor or the create factory method, but any services in the annotation (that are presumably not already handled via create() are then "side-loaded".

2. +++ b/core/lib/Drupal/Core/Plugin/PluginServiceTrait.php
   @@ -0,0 +1,38 @@
   +  public function setServices(array $services) {
   

   Since the services are not directly attached as standalone properties, do you envision developers using $this->getService('date.formatter'); routinely?

3. +++ b/core/lib/Drupal/Core/Plugin/PluginServiceTrait.php
   @@ -0,0 +1,38 @@
   +    // @todo: Convert each service id to camelCase property name.
   

   Or is that @todo intended to happen as part of this issue?

5.1:

Yes, we extend the existing ContainerFactory here, so all the normal operations that you expect to happen will happen. Plugin that have a static create() method will continue to work. Plugins the don't implement that interface or our new one here will just be instantiated via "new $plugin_class(...)". But the result of ContainerFactory::createInstance() is passed to our new factory, which is then evaluated to see if it qualifies for (as you say) side loading of additional services. This removes the need to know and understand the static factory approach, and actually removes the tight coupling of services to plugins (which is a complaint I've had for the better part of the last 5+ years with our current approach).

In short "yes" to your point.

5.2:

mpotter and I have discussed this a little bit. I'd prefer a type-hinted solution myself via actual properties on the plugin, but unless there's a general consensus around doing that, it actually adds back to the boilerplate problem. I'm neutral on that topic for the time being. To answer your direct question, any time someone needed one of the services they'd injected, as the code stands right now, they'd call $this->getService('service.id').

5.3:

I'd actually support leaving the ids exactly as they are in the annotation. This gives us clear documented "call this" sort of approach to the getService() method.

Overall, I'm very ++ to this patch.

Eclipse

Sorry to be blunt - but I'm wondering how this is an improvement on using \Drupal::service()?

The idea of the container factory plugin interface is your dependencies are documented in the constructor.

With this approach there's just a setServices call with a random array and a getServices which is essentially a wrapper around \Drupal::service.

I mentor a junior developer and in his early code I told him to use \Drupal in plugins.

When we got onto dependency injection and swapping out implementations we moved to talking about constructor injection.

Thinking back on that mentoring, I would not have seen this as a step forward in his development as most of the advantages of dependency injection are lost - e.g. you cannot unit test this without a booted container.

Apologies for being blunt, no offence intended.

I should note that I feel the same way about ControllerBase and FormBase, they too are using the same service-locator pattern (not dependency injection).

In similar fashion though, mentoring junior developers I would start with those (ControllerBase and FormBase) - and then when it becomes a problem (e.g. testing difficulty) move onto why dependency injection is better than service location.

My 2c

Larowlan,

So in some ways "yes", and in some ways "no".

This is functionally setter injection, though there's no type hinting. To that end we do NOT need a fully bootstrapped container to test the plugins that would use this, simply building an array of mock services and calling setServices() should invoke code in a meaningful and testable way.

That means this is actually quite a bit better than simply using \Drupal::service() because $this->getService('some.id') is NOT a wrapper around $container->get() or \Drupal::service(). That work is done in the factory (same as the ContainerFactory does now) and only the explicit services required by the plugin are injected.

WHY?

Well because while you're story about mentoring a developer is great (and one I totally agree and support) the mechanic by which we inject dependencies today is lacking in a number of ways:

1.) Lots of boilerplate for every class that requires a service.

I actually shouldn't need to enumerate this because we've all done it. Some of us may find it acceptable, some of us may not. I thought I was alone in the "This is unacceptable camp" until mpotter approached me with his ideas and we iterated a bit.

2.) The existing mechanism actually tightly couples plugins to services.

This has been a complain of mine for at least the 5ish years. Ever since we converted the RequestPath Condition and this whole thing went this direction, I have very publicly made it clear that I didn't think it was ok. In order to get different services into a plugin a whole new subclass has to exist. With documented service in the annotation, this open up alter hooks to swapping out services with other compatible services. It opens up easy plugin "cloning" with different service ids (allowing for multiple implementations through the same code). I made these argument years ago and well... either I made the argument badly, or it's a bad argument.

Regardless of the above, the proposed code is no less testable, and it reduces the net boilerplate a developer must implement in order to get a service into their plugin in a documented and testable way. It does the very explicitly at the expense of a typehinted constructor and (currently) proper properties on the class.

Noting the above, I've suggested something like this for the annotation:

/**
  * @Block(
  *   id = "some_id",
  *   services = {
  *     "foo" = "some.service.id"
  *   }
  */

The setServices code could:

public function setServices(array $services) {
  foreach ($services as $key => $service) {
    $this->$key = $service;
  }
}

This allows for type hinted properties which is good DX wise, AND... I think a little magic could be introduced to use the property annotations to validate the service objects that are passed in. That's just a suspicion right now but I bet it's totally doable. This would lead to really simple plugin objects like this:

namespace Drupal\some_block\Plugin\Block;

use Drupal\Core\Block\BlockBase;

/**
 * Provides a 'Some Block' block.
 *
 * @Block(
 *   id = "some_block",
 *   admin_label = @Translation("Some Block"),
 *   services = {
 *     "foo" = "some.service.id"
 *   }
 * )
 */
class SomeBlock extends BlockBase {

  /**
    * @var \Drupal\some_block\FooInterface
    */
  protected $foo;

  /**
   * {@inheritdoc}
   */
  public function build() {
    return $this->foo->renderSomething();
  }

}

This is about the best middle ground of boilerplate and DX I can imagine and makes it possible for new devs to get the services they want in a testable/injectable/type-hinted way as quickly as possible.

Sorry for being long winded, I hope it was at least helpful and explanatory.

Eclipse

Ok, I agree that change would be better - a good mix of the two.

The pattern of injecting dependencies into plugins via the static create method has been a gripe of mine for a very long time :)

However, if we're going to take the annotation route, wouldn't it make more sense to annotate the class variables directly? For example:

class Plugin {

  /**
   * @Service("date.formatter")
   */
  protected $foo;
}

@amateescu

That's a VERY interesting idea... I'll play with a little code here and see what happens.

Eclipse

Re #15 provided its done at discovery and cached (effectively populating the services key in the definition). Otherwise there is a run-time performance overhead of Reflection used in annotations.

FWIW I really like this approach a lot; the more I read through this issue, the more I'm on board. I think declaring your service(s) in an annotation is a lot cleaner and easier to read for future maintainability. If nothing else, anything that can reduce boilerplate code is a generally positive thing.

Ok, this is SUPER rough and does a bunch of stuff wrong, but I wanted to mock up amateescu's suggestion asap. I've included the RequestPath Condition converted to this approach to show how it looks and works.

Eclipse

So, actually you don't have to document what @var is at all on the property, you JUST SHOULD. There's no validation against that right now. To do that I'd have to expand some things pretty considerably if I don't want to use reflection during run time. This is just a rough "what do we think of this approach?" patch.

So this will suffice:

/**
 * @Service('some.service.id')
 */
protected $var;

The @var declaration is good for dx since they'll get autocomplete on the methods and properties of that variable only if they do that, but not a requirement.

Eclipse

Also, I expected this patch to fail much more spectacularly...

Very confused by the difference between the IS and the patch.
Also, there is no indication that @Service on a property only works when used on a plugin class.

This seems like a lot of unreliable magic.

I fought this 4.5 years ago in #1863816: Allow plugins to have services injected, when we initially settled on the static factory method.

IMO either this should be available to any OO code, or none.

Additionally, the proposal is not a move toward Dependency Injection at all.

It instead creates a custom syntax for the Service Locator anti-pattern, but just in a way that *seems* more palatable.

My proposal achieves *exactly* the same goals as the original issue summary. The concerns from #25 apply to the current IS as well, so you're actually against the whole idea of this issue?

Yes, I am against the whole idea, not just one of the proposed implementations

Just to be clear, there is no "Core Team". I'm 1 of the 3 Plugin subsystem maintainers, yes. But @EclipseGc is one of the others, and we clearly disagree here. There is no conspiracy.

Also, I'm not going to respond to any of the hyperbole or baiting in #28 either.

Without constructor-based dependency injection, you cannot be assured of the completeness of an object. The object cannot use its own dependencies within the constructor, it cannot safely call any methods without the possibility of a dependency existing. The public setServices() method could be called during runtime by any code, breaking encapsulation.

The static factory method is "outside" the actual confines of the class itself, by being static it cannot affect the object directly, only new instances. This is by design.

IMO this proposal is no better than \Drupal::service() calls. Your assertion that "you must use a Kernel test" in that case is patently false. With a quick grep, there are at least 100 unit tests in core alone that have to work around \Drupal::service() calls.

$ grep -nrA10 "new ContainerBuilder" * | grep setContainer | wc -l
     134

Of course this is not ideal. But to say it is impossible is untrue.

I'm all for attempting to reduce boilerplate. But any proposal that bypasses constructor injection is a no-go from me.

I totally agree with @tim.plunkett there is no reason you cannot unit test. The tricky part of unit testing is actually setting up all the mocks and then realizing that you might not actually provide useful tests.

While I'm not trying to go down the whole "backdrop" path here, I think we need to make the Drupal developer experience better. Going from using "format_date()" to dozens of lines of code just to "do it right" turns people away from Drupal, and costs our clients real dollars. With shrinking Drupal project budgets, teams are often forced to do it quicker. Which means more code will break when helper functions are deprecated, more code will be written that doesn't lend itself to unit testing.

I totally agree that DX sucks. I think enlarging our facades might be the way to go here, but I honestly don't believe people will unit test many things on custom projects anyway. Having kernel tests gives you almost the same benefits with less setup pain.

For me adding this method adds another way to do things, and adds another level of potential confusion. Its a local optimisation which looks nice now.

##########

For projects I've been working on I came up with the following progression, which is a natural progression and each step builds up on the other:

Step 1

Use \Drupal::service(), turns out its actually as easy unit testable as other stuff.
Setting up the mocks, that's where the difficulity of unit tests area, well and you wonder
whether its worth unit testing at all.

Step 2

You care about autocompletion:

Hide it behind a helper method: getEntityTypeManager();

Step 3

You want to simulate injection.

protected $entityTypeManager;

public function __construct() {
  $this->entityTypeManager = \Drupal::entityTypeManager();
}

Step 4

You want to allow injenction:

protected $entityTypeManager;

public function __construct($entity_type_manager) {
  $this->entityTypeManager = $entity_type_manager ?: \Drupal::entityTypeManager();
}

Step 5

You actually inject it:

...
public static function create() {
  ...
}

I left out the @var / @param bits, because I honestly believe that just putting them on once on the helper function is the only place where it actually matters. We have a way to strict rule around that in core, but at least from my point of view, there is no reason to have to follow that.

This is a cross post with 29/30 and doesn't address anything in them

Hey Tim,

Glad to see you here. You mentioned bunch of important technical bits into your objections that I'd like to try to discuss. #25 in particular.

Additionally, the proposal is not a move toward Dependency Injection at all.

It instead creates a custom syntax for the Service Locator anti-pattern, but just in a way that *seems* more palatable.

So, before we move forward, let's define these things some.

Service Locator: From https://en.wikipedia.org/wiki/Service_locator_pattern

"The service locator pattern is a design pattern used in software development to encapsulate the processes involved in obtaining a service with a strong abstraction layer. This pattern uses a central registry known as the "service locator", which on request returns the information necessary to perform a certain task."

So if I may be so bold, that actually sounds like the entirety of what we call our dependency injection container. It's a "central registry" which returns objects registered within it on request by a particular name. A bit of the language on that page is clearly aimed at compiled applications vs scripted ones, so some of the analogies break down, but mostly simply, I'd say we already have a service locator in Drupal and to our credit, we've leveraged it as a platform for dependency injection as often as possible and discouraged service location (code calling to \Drupal::service('some.service') or code which has been handed the whole container and from which it extracts specific services)

Dependency Injection: From https://en.wikipedia.org/wiki/Dependency_injection

"In software engineering, dependency injection is a technique whereby one object supplies the dependencies of another object. A dependency is an object that can be used (a service). An injection is the passing of a dependency to a dependent object (a client) that would use it. The service is made part of the client's state. Passing the service to the client, rather than allowing a client to build or find the service, is the fundamental requirement of the pattern."

I bolded and emphasized the bit I find most telling here because this tends to jive with my many year distaste for static factory methods. A static factory (I would argue) is actually much closer to service location than dependency injection because (in the terms above) the client is in charge of finding its services for itself. This goes back to my many year old argument about plugins being tightly coupled to their specific service dependencies because php code on the plugin itself is enforcing specific services to be injected. It breaks principles of dependency injection but has successfully masqueraded as "dependency injection" within the Drupal community for a number of years now. :-( This is in-fact service location.

Why/How is this proposal different?

While the current approach within D8 does benefit from constructor injection, most of the benefits we'd want from dependency injection are absent in this approach for reasons I've mentioned on this thread, in this post and historically elsewhere. Constructor injection is great from a testability perspective. No argument. That being said, we're not proposing service location in this case, but rather setter injection. We're delegating that injection to the plugin factory, and in keeping with the intent of dependency injection (vs the letter of it) our methodology allows different services to be injected into our class w/o subclassing. Since dependencies are calculated and cached, they can be altered to include different services w/o any change to the plugin itself.

These differences are immediate improvements over what exists in D8 today, and furthermore, they're easier to implement and use. As noted by myself and others, this decreases the amount of boilerplate code required without increasing, to any significant degree, the testability requirements of the plugin.

Testing code today:

$dep1 = $this->mock('Dep1Interface');
$dep2 = $this->mock('Dep2Interface');
$dep3 = $this->mock('Dep3Interface');

$plugin = new PluginClass($dep1, $dep2, $dep3, 'plugin_id', [], []);

// do stuff

Testing code that uses this approach:

$dep1 = $this->mock('Dep1Interface');
$dep2 = $this->mock('Dep2Interface');
$dep3 = $this->mock('Dep3Interface');

$plugin = new PluginClass('plugin_id', [], []);
$plugin->setServices(['foo' => $dep1, 'bar' => $dep2, 'baz' => $dep3]);

// do stuff
?>

Is it constructor injection? no. :-( I like the cleanliness and benefits of constructor injection as much as everyone else. But I have a long standing issue with the static factory approach. I've tried to state those issues as clearly as possible in this comment. I don't see constructor injection vs setter injection as a offensive in nature. I don't see type hints as an issue that must be fought over, and I don't see how this is "service location" the second we moved the dependency injection out of the plugin itself. (If anything the current pattern is actually much closer to service location than what's being proposed here.)

Again, I hope I've argued my point as clearly as possible here. I think there are a number of benefits to adopting this. It's much closer to what I wanted 4 years ago (and is better than what I was arguing for back then), and looks like others who've used the current D8 approach have also found it lacking. Thoughts?

Eclipse

#29 had some great objections we should address:

Without constructor-based dependency injection, you cannot be assured of the completeness of an object. The object cannot use its own dependencies within the constructor, it cannot safely call any methods without the possibility of a dependency existing. The public setServices() method could be called during runtime by any code, breaking encapsulation.

I'm going to address these in reverse order-ish I think

The public setServices() method could be called during runtime by any code, breaking encapsulation:

That seems super solve-able. We need only never reinstantiate these object properties once they're set. That seems sane to do no matter what.

Without constructor-based dependency injection, you cannot be assured of the completeness of an object:

This is only true during testing. Granted we're in a special case here since this is plugins and we control the factory which instantiates these objects, so under normal circumstances I'd agree with the comment, but since it's plugins, it only affects testing.

The object cannot use its own dependencies within the constructor:

Agreed. I'd love to dismiss this, but I cannot. It's pretty common to have the EntityTypeManager injected and then pull the storage of a single entity type off of it. I don't think that derails this approach by any means, but I will totally concede the point.

[The object] cannot safely call any methods without the possibility of a [missing] dependency

Kinda reworked what you said there because I think this is what you were saying. (feel free to correct me if I'm not addressing what you intend)

So again, were this any old class, yes totally, but since it's plugins, we have some guardrails to prevent that situation with the exception of during testing. And again, I don't think the new requirements after this change are any more onerous from a testing perspective than they've ever been.

On all the basic objections:

So I addressed what Tim had to say point by point, and as he points out, we have differing opinions on this (and that's ok). Still I'd like to point out a few things as they pertain to this topic because again, I am very committed to a change like this and have wanted it for literally years now but simply arguing people's counter arguments doesn't MAKE my argument.

1. Constructor injection:
   While constructor injection is nice, and I too prefer it to setter injection, the benefits of the plugin system are in place to help ensure we always have working objects when they're instantiated through the plugin manager.
2. Whole system:
   Tim argued that there's nothing to specify this is plugin specific, and yeah, that's true but this was disclaimered as a rough patch. Additionally I feel it's kind of unfair to take a thing that we wrote for plugins as essentially say "if it doesn't apply everywhere...". That being said I'd LOVE to see this everywhere, but...
3. Reflection:
   Much of the rest of Drupal uses reflection as a basic assumption of its day to day operation. Routing is probably the strongest example of this because it uses reflection both during route compile time and route execution (in the run time). We've traditionally shied away from doing that with plugins because "Reflection is slow", but 3 quick points: 1) Reflection is a C-level operation, so it's still pretty quick all things considered 2)
   Reflection is faster now than it has ever been and 3) Even if 1 & 2 weren't true, all those discussions happened before we were caching the crap out of everything. We could certainly revisit that at this point I think. The result may be the same, but the landscape of the discussion has certain changed a lot since the last time we had it.
4. Reflection Cont':
   In the case of plugins, we could do reflection during discovery and document the relevant bits we care about in the cached definitions (similar to what I'm doing in this patch now) and get back to constructor based injection. That would likely bring the boilerplate back up to par with what exists today but would (I think) resolve most, if not all, of Tim's objections. This would also break the tight coupling I've complained about for years between the container and the plugin itself. But I think it would probably mean that mpotter's arguments in favor of this approach might be less than they are now because of the boilerplate issue.

Thoughts?

I propose the following patch in an effort to address the concern raised in #25 by @tim.plunkett, that this is not a move towards dependency injection.

It does this by altering the factory such that any service that is in the annotation is then delivered to the constructor. The patch allows this by incorporating both methods discuss here, in the annotation or in the comment of the properties. (It use the core part of the patch in comment #19).

 public function createInstance($plugin_id, array $configuration = array()) {
    $plugin_definition = $this->discovery->getDefinition($plugin_id);
    $plugin_class = static::getPluginClass($plugin_id, $plugin_definition, $this->interface);
    $services = array_values($this->getServices($this->getServiceDefinition($plugin_definition)));
    // 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, ...$services);
    }

    // Otherwise, create the plugin directly.
    return new $plugin_class($configuration, $plugin_id, $plugin_definition, ...$services);
  }

In an effort to still meet the needs of the original post it also includes a trait that provides a constructor that does the boiler plate of assigning the services to their respective classes.

  public function __construct() {
    $args = func_get_args();

    // Shift off the standard plugin arguments and call parent.
    $configuration = array_shift($args);
    $plugin_id = array_shift($args);
    $plugin_definition = array_shift($args);
    parent::__construct($configuration, $plugin_id, $plugin_definition);
    
    // Walk though the remaining args and assign them to services.
    $service_property_names = array_keys($this->getServiceDefinition($plugin_definition));
    foreach($args as $id => $service) {
      $this->{$service_property_names[$id]} = $service;
    }
  }

So a plugin writer need only annotate his services and use the trait, and then they will be available. Here is an example the LayoutConfigBlock is using both the block service and the layout service:

/**
 * Provides a 'LayoutConfigBlock' block.
 *
 * @Block(
 *  id = "layout_config_block",
 *  admin_label = @Translation("Layout config block"),
 *  deriver = "Drupal\layout_config_block\Plugin\Derivative\LayoutConfigBlockDerivative",
 *  services = { 
 *    "pluginManagerCoreLayout" = "plugin.manager.core.layout"
 *  }
 * )
 */
class LayoutConfigBlock extends BlockBase {

  use PluginServiceTrait;

  /**
   * The Block Manager.
   *
   * @Drupal\Core\Annotation\Service("plugin.manager.block")
   */
  protected $pluginManagerBlock;

1. +++ b/core/lib/Drupal/Core/Plugin/Factory/ServiceFactory.php
   @@ -0,0 +1,65 @@
   +      return $plugin_class::create(\Drupal::getContainer(), $configuration, $plugin_id, $plugin_definition, ...$services);
   

   Ok, you can't do this because ...$var is 5.6 notation and we are still, as far as I know, constrained to 5.5 code.

2. +++ b/core/lib/Drupal/Core/Plugin/PluginServiceTrait.php
   @@ -0,0 +1,48 @@
   +  public function __construct() {
   +    $args = func_get_args();
   +
   +    // Shift off the standard plugin arguments and call parent.
   +    $configuration = array_shift($args);
   +    $plugin_id = array_shift($args);
   +    $plugin_definition = array_shift($args);
   +    parent::__construct($configuration, $plugin_id, $plugin_definition);
   +    ¶
   +    // Walk though the remaining args and assign them to services.
   +    $service_property_names = array_keys($this->getServiceDefinition($plugin_definition));
   +    foreach($args as $id => $service) {
   +      $this->{$service_property_names[$id]} = $service;
   +    }
   +  }
   

   Assuming we COULD use 5.6, you don't need most of this code anyway, you could

   
   public function __construct($configuration, $plugin_id, $plugin_definition, ...$services) {
     parent::__construct($configuration, $plugin_id, $plugin_definition);
     foreach ($services as $key => $service) {
       if (property_exists($this, $key)) {
         $this->{$key} = $service;
       }
     }
   }
   
   

   That'd be REALLY great if we could do that, but we can't w/o a change to our php version. But that has knock on effects that are good all around because it gives mpotter what he wants, but it can also facilitate what tim.plunkett is asking for since any plugin can still fully type hint their constructor since ...$services in the factory actually unpacks the arguments, a constructor like this would be valid:

   
   public function __construct($configuration, $plugin_id, $plugin_definition, FooInterface $foo, BarInterface $bar) {
   
   

Argument unpacking and variadics would go a long way towards giving both groups exactly what they want. I'd tend to lean more toward having the type hints in our constructors for the time being due to our php restrictions, but the variadics and argument unpacking could absolutely be done in contrib until such time as we can feasibly move it to core (D9).

Thoughts?

Eclipse

@EclipseGc could we us call_user_func_array instead of ... if we are not able to use 5.6?

Can't cufa the constructor.

Here's something I had rattling around:

diff --git a/core/lib/Drupal/Core/Plugin/Factory/ContainerFactory.php b/core/lib/Drupal/Core/Plugin/Factory/ContainerFactory.php
index 7aefa5917e..9508be2924 100644
--- a/core/lib/Drupal/Core/Plugin/Factory/ContainerFactory.php
+++ b/core/lib/Drupal/Core/Plugin/Factory/ContainerFactory.php
@@ -16,6 +16,23 @@ public function createInstance($plugin_id, array $configuration = []) {
     $plugin_definition = $this->discovery->getDefinition($plugin_id);
     $plugin_class = static::getPluginClass($plugin_id, $plugin_definition, $this->interface);
 
+    if (!empty($plugin_definition['services'])) {
+      $args = [];
+      foreach ($plugin_definition['services'] as $service_name) {
+        $args[] = \Drupal::getContainer()->get($service_name);
+      }
+      $args[] = $configuration;
+      $args[] = $plugin_id;
+      $args[] = $plugin_definition;
+
+      // @todo Remove this check someday.
+      if (version_compare(PHP_VERSION, '5.6.0', '>=')) {
+        return new $plugin_class(...$args);
+      }
+      else {
+        return (new \ReflectionClass($plugin_class))->newInstanceArgs($args);
+      }
+    }
     // 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);

@tim.plunkett I have incorporate your strategy in the following patch.

  public function createInstance($plugin_id, array $configuration = array()) {
    $plugin_definition = $this->discovery->getDefinition($plugin_id);
    $plugin_class = static::getPluginClass($plugin_id, $plugin_definition, $this->interface);
    $services = array_values($this->getServices($this->getServiceDefinition($plugin_definition)));
    $args = array_merge([$configuration, $plugin_id, $plugin_definition], $services);

    // If we have an old version of php we can not use ... .
    if (version_compare(PHP_VERSION, '5.6.0', '<')) {

      // If the plugin provides a factory method, pass the container to it.
      if (is_subclass_of($plugin_class, 'Drupal\Core\Plugin\ContainerFactoryPluginInterface')) {
        return forward_static_call_array([$plugin_class, "create"], array_merge([\Drupal::getContainer()], $args));
      }

      // Otherwise, create the plugin directly.
      return (new \ReflectionClass($plugin_class))->newInstanceArgs($args);
    }

    // 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(), ...$args);
    }

    // Otherwise, create the plugin directly.
    return new $plugin_class(...$args);
  }

1. +++ b/core/lib/Drupal/Core/Plugin/Factory/ServiceFactory.php
   @@ -0,0 +1,79 @@
   +  protected function getServiceDefinition($plugin_definition) {
   +    if ($plugin_definition instanceof PluginDefinitionInterface && isset($plugin_definition->services) ) {
   +      return $plugin_definition->services;
   +    }
   +    elseif ( is_array($plugin_definition) && !empty($plugin_definition['services']) ) {
   +      return $plugin_definition['services'];
   +    }
   +    return [];
   +  }
   

   This part looks fine to me.

2. +++ b/core/lib/Drupal/Core/Annotation/Service.php
   @@ -0,0 +1,24 @@
   +class Service extends AnnotationBase {
   
   +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -246,9 +249,31 @@ public function processDefinition(&$definition, $plugin_id) {
   +      AnnotationRegistry::reset();
   +      AnnotationRegistry::registerLoader('class_exists');
   +      $reader = new AnnotationReader();
   +      foreach ($reflection->getProperties() as $property) {
   +        if ($annotation = $reader->getPropertyAnnotation($property, '\Drupal\Core\Annotation\Service')) {
   +          $services[$property->name] = $annotation->get();
   +        }
   +      }
   

   This part does not.

If we are expanding the existing concept of plugin annotations to include 'services', that seems reasonable.

But once we introduce the ability to @Service() annotate a property, it makes no sense to couple that to plugins.
It would also need to work for classes instantiated via ClassResolver::getInstanceFromDefinition().

@tim.plunkett,

I look to see if there was some higher order place that we could do this work so that it would hit both the plugin system and the ClassResolver, but they seem to be independent of each other. Also that system does not use annotations at all (here we are just injecting the services into the plugin definition that is already pulled from the annotations), so I would expect we would have to ensure that there is a want/need for annotations in general.

Could expanding this to that system be a follow up task?

+  public function createInstance($plugin_id, array $configuration = array()) {

Could we also pass in default arguments and merge so that weird plugin managers like Typed Data Manager could use this?

Actually it might be better to let the caller (plugin manager) pass in the arguments it needs for its constructor rather than assuming the default constructor arguments.

@mradcliffe

I rewrote plugins from scratch in PHP-land (I have a github repo if you'd care to see it) in which I try to make far fewer assumptions about what constructor parameters are required by the plugin, but there are certain things that the plugin system itself wants, like the plugin definition for example.

Changing that in D8 is probably VERY BC-breaking, so I'm not certain we want to tackle that here, but providing an easier way for services to get injected seems sane and doable.

Eclipse

@EclipseGc, I agree that we shouldn't be breaking constructor arguments. And that is exactly why I think that the ServiceFactory class shouldn't assume the constructor arguments, and then it could be re-used by Typed Data Manager that implements plugins with different constructor arguments.

So instead of

+class ServiceFactory extends ContainerFactory {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function createInstance($plugin_id, array $configuration = array()) {
+    $plugin_definition = $this->discovery->getDefinition($plugin_id);
+    $plugin_class = static::getPluginClass($plugin_id, $plugin_definition, $this->interface);
+    $services = array_values($this->getServices($this->getServiceDefinition($plugin_definition)));
+    $args = array_merge([$configuration, $plugin_id, $plugin_definition], $services);

We do something like this

class ServiceFactory extends ContainerFactory {

  /**
   * {@inheritdoc}
   */
  public function createInstance($plugin_id, $plugin_definition, array $defaultArguments) {
    $plugin_class = static::getPluginClass($plugin_id, $plugin_definition, $this->interface);
    $services = array_values($this->getServices($this->getServiceDefinition($plugin_definition)));
    $args = array_merge($defaultArguments, $services);

And the caller could do $createdItem = $this->factory->createInstance('created', $definition, [$definition, $name, $parent]); for #2903549: Replace REQUEST_TIME with time service in CreatedItem and #2053415: Allow typed data plugins to receive injected dependencies.

Because the default arguments are plugin id, plugin definition and configuration, all of which a plugin will need (true, some don't have configuration, but most do, and so the factory has to be uniform... again in my rewrite I tried to make this more configurable per plugin).

Still the issue you're citing here is a separate concern from what we're discussing on this issue in my mind. So maybe file a related ticket and we can discuss it further there.

Eclipse

If I may just throw a wrench into this whole discussion, I think that the way dependency injection in Drupal is a bit burdensome having to have a create method, know the service names by their aliases which do not match the class name etc is not great.

I have used the Laravel framework a lot, which is also Symfony based and uses dependency injection, but it's dependency injection is completely automagical, by way of Reflection API.

If your controller constructor, or any controller method, defines an argument with an explicit object class, it just gets injected. There is no need to think further of it. You just use when needed and no need to think ahead or define it ahead of time, know what alias to use etc.

The biggest issue I have is finding out what the name of a service is when I want to use it, whether I'm defining it in the arguments for a service I've created in it's YML file, or need to put them in the create method of my object class. Because, unlike Laravel, Drupal is modular and therefore has services defined all over the place instead of in a single config file, it's very hard to find out what the name of a service is. I can easily find documentation on the various service object classes for Drupal, but nowhere in that documentation does it simply tell me what it's service alias name is for when I want to use it.

So if you could simply have services injected by way of using the desired class at the top of your file and then putting it in the function signature, this would greatly simplify it. No need to think about what services you want to use, just use them and it all just lazy loads and happens automagically.

This would of course be a major change that would ruin everything existing, unless you maintain backwards compatibility for all existing modules for a while, but I'm wondering why it was not just done this way in the first place.

Symfony's introduction of autowiring came years after the plugin system was written. I absolutely agree that we'd be doing it that way if it had existed at the time!

There are several issues about autowiring:
https://www.drupal.org/project/issues/search/drupal?text=autowiring&stat...

The biggest issue I have is finding out what the name of a service is when I want to use it, whether I'm defining it in the arguments for a service I've created in it's YML file, or need to put them in the create method of my object class.

How autowiring would help on this? You still need to figure out the class/interface name of the injected service. Furthermore, in many cases you have to name arguments in a specific way. That kind of magic brings additional confusion.
Example from Rate Limiter documentation.

// if you're using service autowiring, the variable name must be:
// "rate limiter name" (in camelCase) + "Limiter" suffix
public function index(RateLimiterFactory $anonymousApiLimiter)

There is a very simple way to get a list of availble services. Which is using special CLI command. Like follows.

• Symfony - console debug:container
• Drupal Console - drupal debug:container
• Drush (with Devel module) - drush devel:services

Also Symfony plugin for PhpStorm helps a lot in this.

This seems stalled and is blocking some simpler DI improvement issues like #2053415: Allow typed data plugins to receive injected dependencies

What's the real win here instead of just adding support for ContainerFactorPluginInterface to more/all plugin types. Coming at this form writing custom fields, where the lack of proper DI is an ongoing DX headache.

@Chi yes that is a very good and valid point. There's also the API documentation online to help find services, so one way or another you do have to find and know what they are. I am a fan of the cleaner code, though.

Added reroll of patch #38 on Drupal 10.1.x.

Try to fix PHP stan errors

Having native attributes it should be easier now in context of #3252386: Use PHP attributes instead of doctrine annotations

Probably we can close it for #3296391: Allow defining injected services with PHP attributes

As the failed test shows it needs more logic to inject properly https://dispatcher.drupalci.org/job/drupal_patches/155264/testReport/jun...

+++ b/core/lib/Drupal/Core/Plugin/Factory/ServiceFactory.php
@@ -0,0 +1,82 @@
+    // If we have an old version of php we can not use ... .
+    if (version_compare(PHP_VERSION, '5.6.0', '>')) {

this check should be removed

I didn't see this issue before opening #3294266: Improve plugin autowiring performance by moving reflection to discovery time but these appear to be duplicates. Over there I propose using the Symfony #[Autowire] attribute syntax to replace the plugin factory method.

Yes, so let's make this a dup (unless contrary reason comes up).