Problem/Motivation

Following core best practices, allowing autowiring and autoconfiguring of services reduces boilerplate and reduces the maintenance burden of updating service config.

In addition search_api backends can reduce the amount of configuration of services. See #3527820: Add autowire and autoconfigure to services.yml

Steps to reproduce

Proposed resolution

  • Add autowire and autoconfigure to service defaults
  • Remove argument definitions
  • Remove tags that are autoconfigured (e.g. event_subscriber

Remaining tasks

Issue fork search_api-3528042

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:

About issue forks

Comments

kim.pepper created an issue. See original summary.

kim.pepper opened merge request !241

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Active » Needs review
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

I reverted the autowiring of SearchApiConverter as the BC logic in there was too complex. Created a follow up for that #3528045: [PP-1] Autowire SearchApiConverter service

acbramley’s picture

acbramley commented
Status: Needs review » Reviewed & tested by the community

This is looking great, manually verified all the service aliases match the classes/interfaces of the services.

Some of these services probably don't need aliases as they're unlikely to be autowired (e.g the event subscriber or param converter) but I don't think there's any harm in adding htem.

drunken monkey made their first commit to this issue’s fork.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Needs review

Thanks a lot for creating this issue and already providing a great MR!

Looked pretty good to me, just fixed merge conflicts and amended. (Also, I wasn’t aware or had already forgotten you could omit the constructor doc blocks … As it is allowed, I guess it really does make sense to drop them. Created #3542815: Clean up (service) constructors as a follow-up for other classes.)

Also:

Some of these services probably don't need aliases as they're unlikely to be autowired (e.g the event subscriber or param converter) but I don't think there's any harm in adding htem.

I do: backwards compatibility. As soon as we define the class name as a service key we cannot just swap out the class anymore.
Therefore, I removed the aliases for all services that don’t have an interface. I think this should still cover all that are likely to be actually autowired anywhere.

Please review! And thanks a lot, again!

acbramley’s picture

acbramley commented
Status: Needs review » Reviewed & tested by the community

Looks great, thanks

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Fixed

Good to hear, thanks for your feedback!
Merged.
Thanks again, everyone!

Status: Fixed » Closed (fixed)

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