Autowire core services that do not require explicit configuration

This is hard-blocked on #3049525: Enable service autowiring by adding interface aliases to core service definitions. See #3049525-53: Enable service autowiring by adding interface aliases to core service definitions for why this can't happen yet.

Adding postponed item to remaining tasks per remaining tasks summary field details.

Correction for #3: this is probably blocked on #3320315: Allow uppercase service IDs/names — necessary for autowiring support instead.

#3320315: Allow uppercase service IDs/names — necessary for autowiring support is in already, which should make this highly doable now! 👍

@Wim as I got it was postponed on #3049525: Enable service autowiring by adding interface aliases to core service definitions so not clear title vs status

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.

#3325557: Enable more service autowiring by adding interface aliases to core modules landed!

AFAICT that means this issue can now really move forward!

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

Seems 11.x caused some failures.

I filed #3367416: [policy] Service autowiring - decide approach for multiple service implementations based on the same class and different arguments. 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).

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

#22 no I do not think so. If it's finally agreed that #3367416: [policy] Service autowiring - decide approach for multiple service implementations based on the same class and different arguments 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 #3367416: [policy] Service autowiring - decide approach for multiple service implementations based on the same class and different arguments 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.

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?

The rescope seems good and definitely simpler to read.

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?

blocker for #3296391: Allow defining injected services with PHP attributes

Rebased against lastest 11.x and got tests green.

One issue that's come up, kind of similar to what's mentioned in #41: 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.

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 #3293926: Error decorating non-existent service when inner service's module not installed 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

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 #3296391: Allow defining injected services with PHP attributes was closed.

I had a thought on a possible way to move forward:

Since Drupal has its own ContainerBuilder class, can do the following:

• Override ::get() so that only certain services that are in an allow list and explicitly set as not autowired can be instantiated, maybe with a way to limit this restriction only to compile time?
• Implement the allow list as a container parameter (core.builder_allowed_services or some better name), with modules being able to provide their own {module name}.builder_allowed_services parameters that get merged to the core one
• Set the core list with some services that are likeliest to be instantiated in service providers, such as state and module handler (and look through core service providers for other services that are being instantiated)
• Maybe also look into providing "bootstrap" versions of module handler, state, etc, similar to BootstrapConfigStorageFactory used only during compile time

Can create a new issue to look into the above if this seems worthwhile.

After a bit more thought, not sure the allow list is necessary if all the services being instantiated (and their dependencies) need to be explicitly marked autowire: false.

And even then, there's the risk that services in core that are deemed autowire-safe and have their arguments definition removed in YAML could be dependencies of services in contrib or custom code that are instantiated in those modules' service providers, leaving us back where we started. :)

One thing I should note: service providers should not, in general , do $container->get('foo') they should only do getDefinition/findDefinition, I noted at the end https://www.drupal.org/node/2974194 how to get a list of modules, how to access (raw) config and encouraged settings instead.

There are 3 cases in core where ContainerBuilder::get() is used.

Two are of them are in test modules:

• Drupal\container_rebuild_test\ContainerRebuildTestServiceProvider::alter() has $container->get('state')
• Drupal\service_provider_test\ServiceProviderTestServiceProvider::alter() has
  

     $module_handler = $container->get('module_handler');
      try {
        $this_module_relative_path = $module_handler->getModule('service_provider_test')->getPath();
     

The use of module handler likely can be replaced with the container.modules parameter, however the use of state in the first test is trickier to replace. It'd probably be better for test performance to use key value there anyway, but that still requires instantiating the key value service.

The third is Drupal\workspaces\WorkspacesServiceProvider::alter(), which has $container->get('kernel'). This last one can be ignored, because kernel is not autowired.

One thing I should note: service providers should not, in general , do $container->get('foo') they should only do getDefinition/findDefinition

Agreed. Thinking about this more, it might be worthwhile to do the allow list anyway, separate from autowiring concerns, to discourage devs from instantiating services in service providers, or at least make them think about it and provide them documentation for possible alternatives for what they want to do.