Improve plugin autowiring performance by moving reflection to discovery time

As an example, this code

  /**
   * Constructs a new MenuLinkDefault.
   *
   * @param array $configuration
   *   A configuration array containing information about the plugin instance.
   * @param string $plugin_id
   *   The plugin_id for the plugin instance.
   * @param mixed $plugin_definition
   *   The plugin implementation definition.
   * @param \Drupal\Core\Menu\StaticMenuLinkOverridesInterface $static_override
   *   The static override storage.
   */
  public function __construct(array $configuration, $plugin_id, $plugin_definition, StaticMenuLinkOverridesInterface $static_override) {
    parent::__construct($configuration, $plugin_id, $plugin_definition);

    $this->staticOverride = $static_override;
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    return new static(
      $configuration,
      $plugin_id,
      $plugin_definition,
      $container->get('menu_link.static.overrides')
    );
  }

can be reduced to something like

  /**
   * Constructs a new MenuLinkDefault.
   *
   * @param array $configuration
   *   A configuration array containing information about the plugin instance.
   * @param string $plugin_id
   *   The plugin_id for the plugin instance.
   * @param mixed $plugin_definition
   *   The plugin implementation definition.
   * @param \Drupal\Core\Menu\StaticMenuLinkOverridesInterface $static_override
   *   The static override storage.
   */
  public function __construct(
    array $configuration,
    $plugin_id,
    $plugin_definition,
    #[Service('menu_link.static.overrides')]
    StaticMenuLinkOverridesInterface $static_override,
  ) {
    parent::__construct($configuration, $plugin_id, $plugin_definition);

    $this->staticOverride = $static_override;
  }

With PHP 8 constructor property promotion this can be reduced even further, but that is for a separate issue.

Rough prototype which converts two core plugins. Backward compatibility will be the tricky part here.

The name ContainerAutowiredPluginInterface is in the hope that one day we can drop the attribute in most cases and still inject the correct service, if we complete #3049525: Enable service autowiring by adding interface aliases to core service definitions

There is also an argument for using https://github.com/symfony/dependency-injection/blob/6.2/Attribute/Autow... as the attribute instead of creating our own, as we might want to use Symfony's attribute for autowiring services.

Refactored to not require an interface and borrow Symfony's autowire attribute. Added some tests for various cases including autowired subclasses inheriting from non-autowired parents and vice versa.

Symfony also supports autowired setter injection, that is definitely something we could consider supporting as well, if that helps - but the code as it stands should support both the old create() method and autowiring, even in subclasses.

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

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?

@duadua Some great ideas there. I especially like the idea of doing the reflection at discovery time instead of runtime, though that needs a bit more thought on how to architect and store the necessary data.

Following these issues:
#3049525: Enable service autowiring by adding interface aliases to core service definitions
#3325557: Enable more service autowiring by adding interface aliases to core modules
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.

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.

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.

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.

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 do instantiate quite a few plugins on some page loads so storing the metadata in advance and avoiding reflection at runtime would be good.

I think with this technique we can work around the varying requirements of constructors of different plugin types by passing in more metadata into the discovery and/or extending the factory.

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] (or a similar custom attribute)? 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.

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

Perhaps we need to add an autowire property to Drupal\Component\Plugin\Attribute and all subclasses? This doesn't solve point 1, however.

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.

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. We also need to store the set of services somewhere - if not on the plugin definition, then where?

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.

#3452852: Add create() factory method with autowired parameters to PluginBase landed, which implements this at runtime instead of discovery time. I think we can continue here with adding the reflection step at discovery time and using that data if it's available instead of needing reflection at runtime.

I think they can be done independently? What I'm kinda proposing here is splitting up the reflection and instantiation parts so they are done at different times, but as long as both parts understand about closures, it doesn't matter whether the reflection is done at discovery or runtime.

Retitling this as I think we can narrow the scope a bit.

Needs tests, but MR!13493 should improve performance by handling reflection at discovery time for plugins autowired via PluginBase::create().

Can't reproduce those three fails locally, all those tests pass for me.

Maybe we need a separate map object where we store the arguments once against the class name. Which when I write it out, sounds quite like a container!