Allow controller service wiring via constructor parameter attributes

This seems fairly simple to implement based on the parent issue. Converted NodeController to use autowiring simply by deleting the create() method.

Trying to find an example to convert in core where we would need to specify #[Autowire]; some existing controllers can be automatically wired just by removing create() but others need e.g. entity storage injected which autowiring can't currently handle. Wondering if we should add a Drupal specific attribute for injecting entity handlers...

That failure because the change or a pipeline issue?

Not sure where the error went but all green.

Moving to NW for the change record. If I'm understanding correctly we won't need create() anymore? Will it eventually be deprecated?

Added CR: https://www.drupal.org/node/3395716 →

create() is still currently needed in some cases where a constructor argument is something other than a service (e.g. an entity storage handler or a config object). As the implementation in the base class handles the autowiring there isn't really any need to change or deprecate this at the moment; you can simply omit the method to use the base implementation or override it if you have more specific needs when constructing your controller.

Tagging for some basic test coverage for the automatic inference and the #[Autowire] attribute.

Added some test coverage by autowiring SystemTestController. Also improved the error message if autowiring fails.

Only rebased and removed an unused statement.

But according to test-only feature it's passing still.

The test-only feature will still pass; the test checks that the $persistentLock variable is still injected after refactoring away the create() method. I chose this service because LockBackendInterface cannot be autowired as it has multiple implementations and we need the #[Autowire] attribute to make it work. If you remove the attribute locally you should see the new exception:

1) Drupal\KernelTests\Core\Controller\ControllerBaseTest::testCreate
Symfony\Component\DependencyInjection\Exception\AutowiringFailedException: Cannot autowire service "Drupal\Core\Lock\LockBackendInterface": argument "$persistent_lock" of method "Drupal\system_test\Controller\SystemTestController::_construct()", you should configure its value explicitly.

Ah gotcha, thank you for explaining @longwave. Then I believe this is should be good.

Added explicit test coverage for the new exception.

Question: should this create() method be provided as a trait so other users of ContainerInjectionInterface can use it too?

May of jumped the gun. Putting back into review

Answered my own question: 📌 Allow plugin service wiring via constructor parameter attributes Needs work provides such a similar trait that I think the two can be combined. The one here is slightly simpler (it doesn't deal with additional args) so let's get this in first and then we can add more features over there.

I think AutowireTrait makes more sense. Leaves it open for additional features later on? A

Also appears to be nightwatch failures. Thought about rerunning but figured if we updated that trait name it will run then.

Moved to the simpler name.

Gitlab is green so going to mark it.

Added an new change record describing how to use AutowireTrait: https://www.drupal.org/node/3396179 →

Updated the ControllerBase change record to refer to it.

Do we have any performance concerns at all. In Symfony land autowiring does not happen at runtime but here it does.

To me, this has no performance concerns at the moment as we are only replacing ControllerBase::create() which should be instantiated once per page load and I think any increased time should be negligible. I intend to add a similar implementation in FormBase in a followup issue where I also don't foresee any issues. The improved DX is much more important.

AutowireTrait is entirely opt-in and so should not be used by classes that are instantiated several times, but that is ultimately up to the consumer of the trait.

When it comes to plugins it might be a different story; we may have to figure out the autowiring dependencies at discovery time and use cached values at runtime for performance, which is why I've switched to concentrating on this over 📌 Allow plugin service wiring via constructor parameter attributes Needs work

Yep the DX win definitely worth it.

I think it is worth documenting the performance assumptions and the idea that it should not be used on something that is instantiated a lot on the trait. That way we'll be helpfully contrib / custom make good decisions about when to use it.

Added comment, back to RTBC.

Perhaps the trait itself could do static caching per class of discovered arguments, to avoid the multiple instantiation issue? It would still be preferable to do it up front at discovery time where possible, but we could work around the performance issue here if we needed to. Either way, to me this is for a future issue.

Committed to 11.x and backported to 10.2.x

This is a major DX win.

Added something to release notes draft

larowlan → closed merge request !5034

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