Allow plugin service wiring via constructor parameter attributes

@larowlan I added a test that is similar to your trait setup but it passed first time, so not sure what I have done wrong - any ideas?

I'll see if I can still replicate it - might have been a PHP version issue?

For full backward compatibility we might need to provide an autowiring trait, which perhaps means we can simplify some of this.

class MenuLinkDefault extends MenuLinkBase implements ContainerFactoryPluginInterface {

  public function __construct(array $configuration, $plugin_id, $plugin_definition, StaticMenuLinkOverridesInterface $static_override) {
    ...
  }

  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    ...
  }
}

In the long run we probably want to drop the create method here and use autowiring instead, but then we are no longer implementing ContainerFactoryPluginInterface, and any subclasses will no longer have their create() method called.

So maybe the answer is to autowire the constructor, and use a trait to replace create() that handles the autowiring? Then any subclasses will work with no changes - we could also find a way to notify downstream users that they should switch to autowiring, or maybe we just live with the trait forever until we deprecate ContainerFactoryPluginInterface entirely?

Sounds like this should be NW for the trait mentioned in #19?

Marked 📌 Allow typed data plugins to receive injected dependencies Closed: duplicate as a duplicate as well. Would be great to make some traction on this by answering the questions raised in #19, or perhaps this just needs an IS update to reflect the current status after the recent MR changes.

@duadua Some great ideas there.

Following these issues:
📌 Enable service autowiring by adding interface aliases to core service definitions Fixed
📌 Enable more service autowiring by adding interface aliases to core modules Fixed
it is possible to infer the correct service in many cases without needing the #[Autowire] attribute. Therefore we can implement a trait that autowires based on the type where possible, or the attribute where it isn't possible, and this seems fairly trivial. I don't think we have to worry about inheritance even? Subclassed plugins will continue to provide their own create() method, parent or child classes can switch to the trait any time, as far as I can see - this might need a bit of testing though.

Proof of concept patch attached.

If a new approach is being taken could the IS be updated?

Will new approach still require the change record? Leaving tag just in case.

Can this be expanded to all classes that have the container factory pattern -- forms and controllers IIRC.

Yes, but probably not in this issue; I picked plugins first as I was getting annoyed keeping create() methods in sync with the constructor but those are valid candidates as well.

Forms have exactly the same pattern of create() and __construct(), just use Drupal\Core\DependencyInjection\ContainerInjectionInterface instead.

But the plugin factory method and constructors have three additional arguments - array $configuration, $plugin_id, $plugin_definition - so I don't think we can use the same code/trait.

We have the controller issue

So, the trait in the patch does the job. +1.

Is there any benefit of doing it in ContainerFactory instead of using the trait?

Actually, migrate plugins get an additional parameter passed to the ::create() method.
In MigratePluginManager:

      $plugin = $plugin_class::create(\Drupal::getContainer(), $configuration, $plugin_id, $plugin_definition, $migration);

The positional mapping with 3 parameters would lose this 4th parameter.

But technically there is no guarantee that calling code will always pass the `$migration` parameter.
The 4th parameter is optional, to comply with LSP.

I think such cases need a custom trait with 4 parameters instead of 3.
The trait can be chosen per class, according to the needs of the constructor.
E.g. some migrate plugins might not need the `$migration`, then they can use the regular trait.

> But the plugin factory method and constructors have three additional arguments - array $configuration, $plugin_id, $plugin_definition

Field formatter plugins have more base constructor arguments too. I know ALL about this problem from Module Builder.... :/

Created ✨ Add an Autowire trait for plugins Needs review , as an interim DX measure for all existing (legacy) plugins.

As a step forward, would it be better to push the AutowirePluginTrait [1] in the lead up to D11, as it is so alike the services Drupal\Core\DependencyInjection\AutowireTrait ?

[1] Drupal\Core\DependencyInjection\AutowirePluginTrait
✨ Add an Autowire trait for plugins Needs review

[2] Drupal\Core\DependencyInjection\AutowireTrait
AutowireTrait allows ContainerInjectionInterface classes to be autowired →

This would seem to be in keeping with the current thrust of development. There would also need to be a specialist Trait for the migration plugins which require 4 parameters.

What would be the next steps needed? Core tests: one that shows it working the old way, and one that shows it working the new?

Agree that the Trait would be awesome

Urgh, the field formatters case is pretty horrible.

  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    return new static(
      $plugin_id,
      $plugin_definition,
      $configuration['field_definition'],
      $configuration['settings'],
      $configuration['label'],
      $configuration['view_mode'],
      $configuration['third_party_settings'],
      $container->get('entity_type.manager'),
      $container->get('language_manager')
    );
  }

Shouldn't that extraction work be done in the constructor instead? But changing all existing constructors and providing BC is going to be no fun at all.

Shouldn't that extraction work be done in the constructor instead?

Agreed. Just pass the $configuration to the constructor.

I did a quick count and there are 92 sub-classes of FormatterBase in core alone.

I was a bit worried about extra attribute parsing etc. but looking at the MR, it only happens when we need want to instantiate the plugin anyway so any overhead should be negligible.

Drive-by idea: Is the autowiring logic in Symfony able to be extrapolated/re-used here to basically allow for autowiring similar to services? It's the same thing... except these aren't first-class services. Or am I way off?

Following #40 I've had an idea for a while that I've now managed to test out, and it's looking quite promising. We can do the reflection at discovery time and store the set of required services for plugins in the plugin definition, then use that metadata in the plugin factory to skip the create() method. MR!11031 has a prototype of this.

We can't just delete ::create() straight away, because a subclass might be calling it. But for plugins that can be autowired, what about flagging ::create() as #[\Deprecated]? This would indicate both that we intend to delete it in the next major (and so we don't need to notify directly that autowiring is available) but any subclasses *should* be notified because it will break in future? Subclasses can then also implement the attribute themselves, or just convert to autowiring.

This was mentioned in #11 📌 Allow plugin service wiring via constructor parameter attributes Needs work , but it's been common practice to use ::create() in plugin classes that extend other plugin classes, to avoid issues if the parent constructor's signature changes. I think before deprecating ::create(), at least the suggestion from #12 📌 Allow plugin service wiring via constructor parameter attributes Needs work to accommodate autowired setter injection (presumably with the use of the the \Symfony\Contracts\Service\Attribute\Required atribute) should be done in a follow up.

For the inheritance problem:
I just found out that we can put attributes on variadic parameters :)

class B {
  public function __construct(
    protected readonly ServiceForB $serviceForB,
  ) {}
}

class C extends B {
  public function __construct(
    protected readonly ServiceForC $serviceForC,
    #[AutowireParentArgs]
    mixed ...$parent_args,
  ) {
    parent::__construct(...$parent_args);
  }
}

If we could create this attribute or find out that it already exists somewhere...

Running the reflection scan on possibly thousands of plugin definitions where only a fraction of them make use of it looks like a disproportionate resource overhead. Why not handling this similar to Annotations -> Attributes transition? For sure once autowiring is possible, more and more plugin implementations will adapt, but it's a matter of months if not years to come. I'd introduce an Autowire interface for plugins similar as ContainerFactoryPluginInterface to quickly identify which plugin is making use of it. For comparison, services also need an additional autowire: true declaration in order to make use of it. And there may be way more plugin definitions than service definitions around.

Fixed some more tests. But we have some interesting cases to solve:

1. ContentModerationHooks::actionInfoAlter() swaps the class of two action plugins. But these have different constructor signatures and factory methods, so we need to ideally discover constructor after alter hooks have run. But maybe someone also wants to alter the autowiring metadata?
2. NavigationMenuBlock extends SystemMenuBlock but passes in navigation.menu_tree which is an alternative implementation of MenuLinkTreeInterface. This must be explicitly declared; I think some way of opting in plugins is the only way to solve this.

I think to solve #48.1 we need to drop the decorator - instead move the autowiring discovery to the factory, and call it as a kind of factory-preparation step that follows discovery and alter.

Looking for reviews/opinions on whether adding autowire as a boolean property to every plugin definition is the correct solution here. This lets plugins opt in individually which solves #47, and I now think may be required as per #48.2, because we can't guarantee to infer correctly in all cases from interfaces alone.

If we do do this I also think we have to keep the create() methods around for now because there might be subclasses calling parent::create(). There is an attempt in the MR at a BC layer for this, because I think we can and should start removing the interface when the plugins are autowired, but the interface might not be explicitly added to subclasses.

> Looking for reviews/opinions on whether adding autowire as a boolean property to every plugin definition is the correct solution here.

Dumping it in the plugin definition seems wrong, when it's something that's only needed internally, and not by any code using the plugin definition.

Looking for reviews/opinions on whether adding autowire as a boolean property to every plugin definition is the correct solution here.

1. Could we put it in its own attribute per #51?

2. Could we flip the default in a future major release? Seems worth a spin-off issue even if we eventually decide not to do that.

We put the class name in the plugin definition, it's not too dissimilar from that - something that is needed in order to construct the plugin? To me the plugin definition is any metadata that's required before the plugin needs to be constructed, and the autowire flag fits into that category.

It would be nice to reuse the existing #[Autowire] attribute from Symfony on the class, but that's only allowed on parameters:

#[\Attribute(\Attribute::TARGET_PARAMETER)]
class Autowire

If we do go this route this would mean having a new name - #[AutowiredPlugin]?

We can flip the default either way, either globally or on a per-definition basis I think. But we haven't done that for services, I don't know if we would do it here or not.

Tried to the resolve the threads I could.

Maybe the CR well help clear up the others?