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
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:
- 3551738-remove-create-method
changes, plain diff MR !13472
Comments
Comment #3
mstrelan commentedComment #4
dcam commentedI 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 fromControllerBase, so either I'm wrong about the autowiring or maybe it got missed in the grep results.Comment #5
smustgrave commentedCan #4 be looked at.
Comment #6
mstrelan commentedErrorTestController was originally included, and it caused test fails, so I reverted it.
You're correct about
ImageStyleDownloadController, I've updated that now too.Comment #7
dcam commentedObviously 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.
Comment #8
longwaveWe are inconsistent here about using named arguments in the autowire attribute, e.g.
vs
We don't have a coding standards rule for this that I know of but we should still try to be consistent.
Comment #9
mstrelan commentedGood point. FWIW the first param is actually
$value, but the constructor has some logic to set the value from the$serviceparam. Currently we have 22 instances of using the$servicenamed param and 5 where we're using the$valueparam. Have updated this MR to useserviceconsistently.Comment #10
dcam commentedI verified that all
#[Autowire()]attributes have theservicenamed parameter. Feedback has been addressed. I'm moving this back to RTBC.Comment #11
longwaveCommitted and pushed 5cab397ea57 to 11.x and e29ebe89e1f to 11.3.x. Thanks!