Problem/Motivation

Since #3394870: Allow controller service wiring via constructor parameter attributes it's possible to autowire services in controllers extending ControllerBase. This means we can drop the create method if these conditions are satisfied:

  • It's only passing services to the constructor, no parameters or entity storage
  • Each service has a service alias matching the constructor param types*

* Or we can use the #[Autowire] attribute

There should be no BC concerns, e.g. if a child class is calling parent::create(), because the trait in ControllerBase means this method will exist.

Steps to reproduce

Proposed resolution

Find possible candidates:

grep -rl 'extends ControllerBase' core | xargs grep -l 'public static function create'

For each file, review what services are injected, ensuring they have a service alias matching the expected type. Where necessary, use the Autowire attribute to inject additional services. Skip any class that injects service parameters, entity storages or does anything else unusual.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3551738

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mstrelan created an issue. See original summary.

mstrelan’s picture

Issue summary: View changes
Status: Active » Needs review
dcam’s picture

I reviewed the changes and they're looking good so far. I double-checked the #[Autowire(...)] attributes. They're all necessary from what I can see.

When I worked on the plugin override removal issue I asked PHPStorm to find all of the overrides for me instead of using grep. So I did that here in order to catch any child classes that extend from another intermediate base class. I only found one that looks like it can be autowired: core/modules/image/src/Controller/ImageStyleDownloadController.php.

I also found core/modules/system/tests/modules/error_test/src/Controller/ErrorTestController.php, which looks like it ought to be autowire-able. It extends directly from ControllerBase, so either I'm wrong about the autowiring or maybe it got missed in the grep results.

smustgrave’s picture

Status: Needs review » Needs work

Can #4 be looked at.

mstrelan’s picture

Status: Needs work » Needs review

ErrorTestController was originally included, and it caused test fails, so I reverted it.

You're correct about ImageStyleDownloadController, I've updated that now too.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

ErrorTestController was originally included, and it caused test fails, so I reverted it.

Obviously I didn't look at the commit history. Sorry about that. In that case, let's roll with it. The new changes look good to me. My feedback was addressed.

longwave’s picture

Status: Reviewed & tested by the community » Needs work

We are inconsistent here about using named arguments in the autowire attribute, e.g.

    #[Autowire('lock')]
    protected LockBackendInterface $lock,

vs

    #[Autowire(service: 'router')]
    RequestMatcherInterface $router,

We don't have a coding standards rule for this that I know of but we should still try to be consistent.

mstrelan’s picture

Status: Needs work » Needs review

Good point. FWIW the first param is actually $value, but the constructor has some logic to set the value from the $service param. Currently we have 22 instances of using the $service named param and 5 where we're using the $value param. Have updated this MR to use service consistently.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

I verified that all #[Autowire()] attributes have the service named parameter. Feedback has been addressed. I'm moving this back to RTBC.

longwave’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 5cab397ea57 to 11.x and e29ebe89e1f to 11.3.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • longwave committed e29ebe89 on 11.3.x
    task: #3551738 Remove create method from controllers that use autowire...

  • longwave committed 5cab397e on 11.x
    task: #3551738 Remove create method from controllers that use autowire...

Status: Fixed » Closed (fixed)

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