Add create() factory method with autowired parameters to PluginBase

Nice. I was looking for exactly this today!

I have tested the code together with Drupal 10.3 and can confirm it works.

Since I wanted to have the benefit of this right away, I took the liberty and made it a small module: autowire_plugin_trait.

Also see #3464357: Add a trait for autowiring properties in tests.

There I’m suggesting to add symfony/expression-language as a dependency.

That would nicely allow using expressions and enable autowiring of constructs like

#[AutowireProperty(expression: "service('entity_type.manager').getStorage('node')")]

Can we just extend the existing AutowireTrait to work here?

  public static function create(ContainerInterface $container) {
    $args = [];

becomes

  public static function create(ContainerInterface $container, ...$args) {

ie. additional arguments passed to create() become the initial set of arguments? We can array_slice() the reflected parameters so we don't try to fill in the ones we already have.

No, #8 doesn't work because ContainerFactoryPluginInterface::create() doesn't match.

Added AutowirePluginTrait to Views' PluginBase to enable autowiring by default (simply omit the create() method) and autowired some Views plugins to test that it all works.

Not sure we need explicit tests, given we are exercising it in Views tests now, so removing the tag.

Wondering if \Drupal\Component\Plugin\PluginBase should just implement it directly so all plugins benefit?

Adding the trait to \Drupal\Core\Plugin\PluginBase (not Component) seems to work, I autowired the BlockContentBlock plugin to prove it.

Shouldn't there be a test coverage for the new exception being thrown?

 throw new AutowiringFailedException($service, sprintf('Cannot autowire service "%s": argument "$%s" of method "%s::_construct()", you should configure its value explicitly.', $service, $parameter->getName(), static::class));

Added coverage for success and failure of autowiring a block plugin.

In a followup I think we could try making a base trait for AutowireTrait and AutowirePluginTrait where the common code can be abstracted away.

In fact let's do the refactor in #14 now to avoid copy-pasting the code here.

Regarding the phpstan baseline, it turns out that phpstan will accept @return annotations, and we already have that in ContainerFactoryPluginInterface, but PluginBase does not yet implement that interface. Have pushed a commit to implement that interface and update the baseline.

Unfortunately implementing ContainerFactoryPluginInterface in PluginBase breaks a bunch of tests, I don't think Migrate plugins can cope with this as they have a slightly different constructor.

Rebased.

This one needs a rebase.

Only change is core/.phpstan-baseline.php.

I pushed in a new branch, is it preferred to do a force push on the PR, or merge into the branch and avoid overriding it?

There was already an existing branch pointing to the right branch that feedback has been made on. That should be used.

Phpstan should be regenerated

As per #25:

Conflicts were resolved in the initial merge request MR!8306 when the branch was rebased on 11.x.

Switching back to Needs review.

Thanks in advance!

Pipeline appears to have conflicts. Not sure if the baseline file was manually fixed but it should just be re-generated

As Mentioned in #30 , i have fixed the pipeline failure by generating the baseline. pipeline is passing , so moving this Back to NR.

Believe close think one of the last pieces is to add a CR.

Example not super clear in core/modules/block_content/src/Plugin/Block/BlockContentBlock.php why

#[Autowire(service: 'block_content.uuid_lookup')]
Is needed but the others aren't, think a CR should clear that up.

Rebased the MR, fixed conflicts, and brought in the Autowire changes from #3464426: AutowireTrait and autowire for services behave differently for nullable types to the new trait

Drafted a CR. Mostly based off the original Autowire change record but with references & examples updated.

#37 links to the wrong CR. I think it should be that one. And the CR links back to the wrong issue.

Also left a couple of comments in the MR.

I'm confused, #38 points to the wrong CR as well. The correct one would be this.

Ah you're right thank you, I've updated #37 to point to the correct CR

All MR feedback addressed outside of one item that I've just asked for clarity on before I action.

All feedback done. Ready for review again.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Thank you.

I was wondering why only the content block was converted and found the answer in the issue comments. Added that info to the issue summary.

Left a question and a suggestion in MR comments.

Feedback addressed.

Thank you! I left one small suggestion.

I'm not 100% happy with the "AutowireArguments" name, if anyone has better suggestions feel free to offer them!

Maybe either one of

• AutowiredConstructorTrait
• AutowiredInstanceTrait

and the static method to be named as
public static function autowiredInstance()

?

I like the suggestions in #50. I'd probably prefix the method with create which is common for factory methods. E.g., createAutowired() or createAutowiredInstance().

I'd probaby vote for AutowiredInstanceTrait & createAutowiredInstance if we're going to change this. I'll wait for consensus before spending time on this.

#52 is fine with me.

Side note, the instance itself is not "autowired", but that's what "createAutowiredInstance" might wrongly indicate. Therefore I'd favor "createInstanceAutowired" but that's also really just a micro-nit-pick now (let's just ignore this comment 😃)

I'm okay with #54 as well.

Rebased & renamed the trait as per #50/#52/#54. Tests are happy.

AutowiredInstanceTrait::createInstanceAutowired

Reviewed this in context. Adapted the issue title and the CR to match the actual implementation.

There is a slight risk that the newly added create() method in PluginBasebreaks existing contrib/custom plugins with an incompatible create() method in the same way as MenuLinkMock in this issue. Authors of affected plugin classes may rebase onto Drupal\Component\Plugin\PluginBase in that case.

One minor simplification that I am equally happy to skip, otherwise this looks good to me, but I am not eligible to commit as I worked on it.

Committed d7a61c2 and pushed to 11.x. Thanks!

Thanks! Opened #3552110: Remove manual create() method from plugins as a followup so we can remove a bunch of code from core.

Did we benchmark this?

If there a lot of plugins, this is a lot of reflection runtime.

I'm not positive it's an issue, but it would be nice to have at least some confirmation we are not shooting ourselves in the foot performance wise.

@nicxvan it's only used if the plugin explicitly implements ContainerFactoryPluginInterface and doesn't implement create() itself, otherwise the reflection factory method is never called, so it should make no difference until people want to actually use it. The benchmarking can happen in the followup issue I think.

I merged this and then had a thought about performance. Do we know how this will affect a page where maybe 100 plugins have to be instantiated? Obviously it'll be easier to see after #3552110: Remove manual create() method from plugins

Now we've done this I think we can also simplify #3294266: Improve plugin autowiring performance by moving reflection to discovery time to carry out the reflection at discovery time and store the autowiring configuration in the plugin definition, then we won't need reflection at runtime.

Implemented reflection at discovery time in https://git.drupalcode.org/project/drupal/-/merge_requests/13493/diffs over in #3294266: Improve plugin autowiring performance by moving reflection to discovery time

Does anyone know if there is a similar issue open for classes that implement ContainerDeriverInterface? Should we start one?

I also noticed that \Drupal\Core\Menu\LocalTaskDefault does not extend from \Drupal\Core\Plugin\PluginBase, but instead extends from \Drupal\Component\Plugin\PluginBase, so it doesn't inherit the create() method. Was there a reason for this, or just an oversight? Happy to open a new follow-up issue and MR to fix that if that's a good next step...

I wasn't able to find existing issues, so I opened two follow-ups:

#3565338: Add create() factory method with autowired parameters to DeriverBase

#3565337: Extend \Drupal\Core\Menu\LocalTaskDefault from \Drupal\Core\Plugin\PluginBase