Use autowiring for core modules and services

It turns out that we can just start using the #[Autowire] attribute now, and it works out of the box with Symfony 6.2. If autowiring fails the error message is unhelpful, but that is fixed in Symfony 6.3.

This looks really cool and I use this feature frequently in laravel/symfony projects so I am a big fan.
Since this also already converts a lot of services this would be really helpful to get in but I'm not sure how to review this. If this would be broken in some way this would lead to a lot of broken tests, so seeing that doesn't happen gives a lot of confidence.

We should probably add a change record as well?

I agree with the change record.

As someone who hasn't done any symfony project I didn't know what this was so there are sure to be others.

📌 Enable more service autowiring by adding interface aliases to core modules Fixed landed!

AFAICT that means this issue can now really move forward!

Hi I added a basic CR https://www.drupal.org/node/3366757 →

Opened a new MR for 11.x, following 📌 Enable more service autowiring by adding interface aliases to core modules Fixed hopefully everyone can see how this can simplify services going forwards. One day I hope we won't even need to write services.yml files in most cases.

Seems 11.x caused some failures.

I filed 📌 [policy] Service autowiring - decide approach for multiple service implementations based on the same class and different arguments Needs work . That would help here, by allowing to avoid the need to use #[Autowire] for services that have multiple implementations for the same interface (cache bins, loggers, etc).

Fixed test failures and autowired some things that were previously missed.

This doesn't yet autowire core.services.yml, perhaps that can be done in a followup.

Should this be postponed on 📌 [policy] Service autowiring - decide approach for multiple service implementations based on the same class and different arguments Needs work ?

#22 no I do not think so. If it's finally agreed that 📌 [policy] Service autowiring - decide approach for multiple service implementations based on the same class and different arguments Needs work is worth doing, then it's probably worth closing it and do what it preaches here, so that all things go together.

Think the open question on the other ticket needs to be answered first before closing, and I don't think I can make that call. But the question

Given that each module needs its own logger, should we automatically register a logger service for each module, and then autowire LoggerInterface to use the module specific logger with no extra configuration?

Don't want to hold this up on 📌 [policy] Service autowiring - decide approach for multiple service implementations based on the same class and different arguments Needs work if it's not needed. So lets see what the committers think.

The issue summary is clear here. Reading the comments I don't see any unanswered question.

#21 suggests a followup. Does that need to be done or not?

The title of the CR is a directive. It reads like custom code must use autowiring. I think that needs a tweak. I read the CR and it seems vague to me. It says to 'remove as many explicit arguments as possible' without direction on what 'possible' means here. I also see in the MR the use of

  _defaults:
    autowire: true

Maybe an example should be added that uses that?

I am setting to NW because the MR needs a rebase.

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 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.

This is getting unwieldy to repeatedly fix merge conflicts; I wonder if we need to rescope this into smaller issues to get them in more easily.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch 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.

+1 — what kind of split do you think would work well? 2 parts or more still?

One way forward might be to write a test to discover which services could be autowired simply by removing their arguments section (ie. all arguments in the constructor can be autowired without specify attributes), which solves the simple cases, then in a followup we handle the more complicated cases where we need cache backends or loggers to be specified.

Added a test case for #32.

Rescoping this issue to cover core.services.yml only; the changes are minimal (almost all deletions) except for a new test that ensures we don't specify arguments in services.yml where autowiring can suffice.

In followups, we can deal with autowiring more of core.services.yml (by either adding more interface aliases, or specifying the #[Autowire] attribute), and autowiring core modules as well.

Updated the change record to match the rescoped approach.

The rescope seems good and definitely simpler to read.

Note for reviewers: the changes in LamguageServiceProvider imply that this is not backward compatible for modules that might implement similar service providers to extend or alter core services. I am not sure what we can do about this.

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 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.

re: #38 what is the incompatibility? You no longer have to add an argument that can be autowired, but if you do, it still works, right?

  language_manager:
    class: Drupal\Core\Language\LanguageManager
    arguments: ['@language.default']

  public function alter(ContainerBuilder $container) {
    $definition = $container->getDefinition('language_manager');
    $definition->setClass('Drupal\language\ConfigurableLanguageManager')
      ->addArgument(new Reference('config.factory'))
      ->addArgument(new Reference('module_handler'))
      ->addArgument(new Reference('language.config_factory_override'))
      ->addArgument(new Reference('request_stack'));

IIRC the issue was that if we switch language_manager to autowiring but leave the alter hook unchanged, config.factory becomes the first argument of the new constructor, which is incorrect.

blocker for ✨ Allow defining injected services with PHP attributes Postponed

godotislate → changed the visibility of the branch 3295751-autowiring-simple-cases to hidden.

godotislate → changed the visibility of the branch 3295751-autowiring-simple-cases to active.

godotislate → changed the visibility of the branch 3295751-autowiring-11.x to hidden.

godotislate → changed the visibility of the branch 3295751-use-autowiring-for to hidden.

Rebased against lastest 11.x and got tests green.

One issue that's come up, kind of similar to what's mentioned in #41 📌 Use autowiring for core modules and services Needs work : If you have a service definition that is autowired with no arguments in the definition, or if any of its service dependencies' definitions are are autowired with no arguments, then trying to instantiate that service in a module ServiceProvder will result in error. Take these two example from test module service providers:

Drupal\container_rebuild_test\ContainerRebuildTestServiceProvider

  public function alter(ContainerBuilder $container) {
    $count = $container->get('state')->get('container_rebuild_test.count', 0);
    $container->get('state')->set('container_rebuild_test.count', ++$count);
  }

Drupal\service_provider_test\ServiceProviderTestServiceProvider 

  public function alter(ContainerBuilder $container) {
    ...
    // Make sure a cached service can be also called in a service provider.
    // https://www.drupal.org/project/drupal/issues/2363351
    /** @var \Drupal\Core\Extension\ModuleHandlerInterface $module_handler */
    $module_handler = $container->get('module_handler');
   ...

While the definitions for both state nor module_handler still have arguments, if arguments for cache.backend.database are removed, then the $container->get() call fails with an error that 0 parameters are being passed into the constructor for DatabaseBackendFactory.

This occurs because ModifyServiceDefinitionsPass compiler pass where the module ServiceProviders register and alter service definitions runs before the compiler passes to handle autowiring. Not sure how to handle this.

Wonder if we can just swap the order so autowiring runs very early, at least so service alterers get the arguments autowired? This may well have other side effects though...

I tried adding an additional AutowirePass right before ModifyServiceDefinitionsPass in CoreServiceProvider:

$container->addCompilerPass(new AutowireRequiredMethodsPass());
$container->addCompilerPass(new AutowireRequiredPropertiesPass());
$container->addCompilerPass(new AutowirePass(false));
$container->addCompilerPass(new ModifyServiceDefinitionsPass());

This led to a circular reference exception when running AutowireTest, and I did not investigate further:

1) Drupal\KernelTests\Core\DependencyInjection\AutowireTest::testAutowire
Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "http_kernel", path: "http_kernel -> http_middleware.ajax_page_state -> http_middleware.negotiation -> http_middleware.reverse_proxy -> http_middleware.content_length -> http_middleware.kernel_pre_handle -> http_middleware.session -> http_kernel".

/var/www/html/vendor/symfony/dependency-injection/Compiler/CheckCircularReferencesPass.php:67
/var/www/html/vendor/symfony/dependency-injection/Compiler/CheckCircularReferencesPass.php:70
/var/www/html/vendor/symfony/dependency-injection/Compiler/CheckCircularReferencesPass.php:70
/var/www/html/vendor/symfony/dependency-injection/Compiler/CheckCircularReferencesPass.php:70
/var/www/html/vendor/symfony/dependency-injection/Compiler/CheckCircularReferencesPass.php:70
/var/www/html/vendor/symfony/dependency-injection/Compiler/CheckCircularReferencesPass.php:70
/var/www/html/vendor/symfony/dependency-injection/Compiler/CheckCircularReferencesPass.php:70
/var/www/html/vendor/symfony/dependency-injection/Compiler/CheckCircularReferencesPass.php:70
/var/www/html/vendor/symfony/dependency-injection/Compiler/CheckCircularReferencesPass.php:43
/var/www/html/vendor/symfony/dependency-injection/Compiler/Compiler.php:73
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:752
/var/www/html/core/lib/Drupal/Core/DrupalKernel.php:1376
/var/www/html/core/lib/Drupal/Core/DrupalKernel.php:899
/var/www/html/core/lib/Drupal/Core/DrupalKernel.php:505
/var/www/html/core/tests/Drupal/KernelTests/KernelTestBase.php:374
/var/www/html/core/tests/Drupal/KernelTests/KernelTestBase.php:253

One vague idea I had is to deprecate altering services via module service providers. Service decoration can handle the bulk of use cases for altering services. And if/when 🐛 Service decorates non-existant service when module not installed Needs work gets in, that should cover a lot of use cases of conditionally altering services.

Altering container parameters based on config like in LanguageServiceProvider might be something that be handled by custom compiler pass and tokenizing BootstrapConfigFactory::get() to use in YAML.

But I'm sure there are projects that do more complicated logic than that, so that might not cover enough use cases. It's also possible that projects implement ServiceProvider register() with conditional logic to add service definitions based on whether a module is installed, in which case just removing alter() is insufficient.

This one was coming up on the 2 month mark for needs-review-queue but seems there ares still open questions in #48-50, so updating remaining tasks. Also not sure if this counts as an API change so left that as TBD.

For the core services in scope to be autowired here, only a few were excepted from the conversion test and their definitions kept their arguments. Tests are green this way. 48 - 50 calls out the exception, and it's really a question of do we keep the exception in place and punt resolving 48 - 50 to one of the follow ups where we deal with more complicated autowiring conversions, or do we deal with it now. Also, a solution to how to deal with 48 - 50 does not currently exist.

I investigated #48-50 some more. Basically, there's a reason that the AutowirePass happens after a bunch of other passes in the "optimization" phase, never mind a few more in the "before optimization" phase, in Symfony\Component\DependencyInjection\Compiler\PassConfig: a lot has to be in place first in order for the service constructor arguments to be autowired correctly. Since Drupal's ModifyServiceDefinitionsPass has to run near the very beginning, there's no way to autowire services before that without essentially running the bulk of the other passes. I think it'd work out to running almost all the passes, then running ModifyServiceDefinitionsPass, and finally running a lot of the same passes again. And even then, I'm not sure that's possible without a side effect or error.

Technically this doesn't just affect core services. If $container->get('my_service') is run in any module's ServiceProvider class, if my_service is defined as autowired with no arguments, the service will fail to load. This is probably mitigated, because the most common use case is getting the module handler and adding or altering a service based on whether another module is installed. So it's less likely that many other services, except for maybe state or config.factory, would be instantiated in module ServiceProviders.

Anyway, I'm not sure how to move forward with autowiring core services, because the use of ->get() means that the whole dependency chain of services for the service that is being instantiated can not be autowired.

Here's example output of the error during a test run:

1) Drupal\Tests\system\Functional\UpdateSystem\RebuildScriptTest::testRebuild
ArgumentCountError: Too few arguments to function Drupal\Core\Cache\DatabaseBackendFactory::__construct(), 0 passed and exactly 5 expected

/var/www/html/core/lib/Drupal/Core/Cache/DatabaseBackendFactory.php:42
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:1190
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:626
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:571
/var/www/html/core/lib/Drupal/Core/Cache/CacheFactory.php:110
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:1171
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:626
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:1308
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:1260
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:1160
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:626
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:571
/var/www/html/core/modules/system/tests/modules/container_rebuild_test/src/ContainerRebuildTestServiceProvider.php:16
/var/www/html/core/lib/Drupal/Core/DependencyInjection/Compiler/ModifyServiceDefinitionsPass.php:30
/var/www/html/vendor/symfony/dependency-injection/Compiler/Compiler.php:73
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:814
/var/www/html/core/lib/Drupal/Core/DrupalKernel.php:1380
/var/www/html/core/lib/Drupal/Core/DrupalKernel.php:899
/var/www/html/core/lib/Drupal/Core/DrupalKernel.php:821
/var/www/html/core/lib/Drupal/Core/Extension/ModuleInstaller.php:722
/var/www/html/core/lib/Drupal/Core/Extension/ModuleInstaller.php:320
/var/www/html/core/lib/Drupal/Core/Extension/ModuleInstaller.php:229
/var/www/html/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
/var/www/html/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:500
/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:555
/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:367

Thanks for trying. Given that we can't know what existing service providers might want to do, I'm not sure how we can do this without breaking backward compatibility. Maybe we just don't need to do this issue at all, and core services remain manually wired.

We could perhaps opt-in certain types of service to autowiring, such as event subscribers, which are unlikely to be altered by service providers? But unsure if there's any real benefit to that.

Given that we can't know what existing service providers might want to do, I'm not sure how we can do this without breaking backward compatibility.

Yeah, aside from checking whether a module's installed, or some state value or config value, it occurs to me that people could be doing things like a custom environment detection service and altering a defined service based on what environment it is.

Core services probably need to remain manually wired, because determining which services would never or almost never be instantiated in a module's ServiceProvider is arbitrary guesswork, other than the event subscriber example, and even then, you never know.

Removing the blocker tag because it looks like ✨ Allow defining injected services with PHP attributes Postponed was closed.