Enable service autowiring by adding interface aliases to core service definitions

Adding related meta issue #3021900: Bring new features from Symfony 3/4/5/6/7 into our container

Replace service IDs with class names and drop the class: key from definitions. Add aliases services IDs for BC layer.

I'm not sure this is the right approach. It might make more sense to keep the existing service names, so that they can be overridden with Service Providers without getting class names confused.

Consider this scenario:

• service name is "Drupal\core\Authentication"
• custom module "example_module"
• custom module Service Provider replaces "Drupal\core\Authentication" class with "Drupal\example_module\MyCustomAuthentication"

Seems super confusing to have a canonical service name pointing to the "wrong" concrete class.

We definitely need aliases for the service names' interfaces to unlock full autowiring support, but do we really need to ditch the existing service names?

> Proposed resolution: Replace service IDs with class names and drop the class: key from definitions. Add aliases services IDs for BC layer.

I guess this is a misunderstanding. As we want to typehint interfaces, the right docs is not the section Using Aliases to Enable Autowiring (it talks about any arguments type-hinted with the class name). What instead applies is the section Working with Interfaces on that page.

So proposed resolution:

• Add interface aliases like described in Working with Interfaces (and do not change service names)

That way, if we decorates a named service, the autowiring automagically uses that.
Also we have a separation of concerns:

• Service definition tells us how is a particular service implemented
• Interface alias tells us how shall a particular interface be autowired

This should also address @AaronBauman's concerns in #4.

Let's also emphasize this (EDIT: Not before SF4.2, sorry for the noise.):

    App\Util\TransformerInterface $shoutyTransformer: '@App\Util\UppercaseTransformer'
    App\Util\TransformerInterface: '@App\Util\Rot13Transformer'

> Thanks to the App\Util\TransformerInterface alias, any argument type-hinted with this interface will be passed the App\Util\Rot13Transformer service. If the argument is named $shoutyTransformer, App\Util\UppercaseTransformer will be used instead. But, you can also manually wire any other service by specifying the argument under the arguments key.

Here's a first attempt at providing aliases for many of the unambiguous services in core.services.yml, enough to autowire all core services at least.

If we do #3021898: Support _defaults key in service.yml files for public, tags and autowire settings then the diff will be a lot smaller as we won't need to explicitly specify autowiring for each service.

Autowiring container parameters could also be done if we use Symfony's argument binding feature: https://symfony.com/blog/new-in-symfony-4-1-getting-container-parameters...

Missed an important named argument in SharedTempStoreFactory.

Also need to specify the handler argument to SessionManager.

Fixed some tests. Really this is just an experiment - we don't have to fully autowire core.services.yml, we just have to provide interface aliases - but it would be nice to see all the tests pass with this in place.

The DrupalKernelTest fix is a hack but not at all sure why schema.inc was included pre-autowiring, but no longer is.

Fixing RebuildScriptTest is tricky because it needs the state service during a container rebuild. At this point the container is actually a ContainerBuilder, which doesn't support autowired arguments (as they are resolved during compilation). This might mean we can't mark state and all its dependent services as autowired.

Unsure what is wrong with UpdateScriptTest and not sure where to start looking.

Without this issue/MR committed, using autowiring as described at https://www.drupal.org/node/3218156 is basically impossible. To clarify: it works, but it does not work for any service that has a core-provided service injected. Which is the majority.

Noticed this at #3103682-20: CDN module 4.x: no new features + drop BC layers + drop now irrelevant headers + PHP >=8.1 + Drupal >=9.4, while working to prepare the "CDN" contrib module I maintain for Drupal 10.

Therefore raising this to Major.

The MR needs a reroll and the testbot is failing.

Most of the heavylifting was already done by longwave.
Fixed RebuildScriptTest/UpdateScriptTest that were failing, and rebased everything on 9.4.x.

This looks like such an amazing feature, thanks for the hard work. But , at least as far as I understand this, the issue title / summary and the MR are different things. Perhaps the issue could use an update?

But , at least as far as I understand this, the issue title / summary and the MR are different things. Perhaps the issue could use an update?

Thanks for noticing!
Issue updated.

Rebased against 10.0.x.

I think we should perhaps split this out into two issues: one to just add the service aliases (which means contrib/custom code can start using this), and another to enable autowiring in core services - for the latter it would be great to complete #3021898: Support _defaults key in service.yml files for public, tags and autowire settings first.

Symfony 6.1 also allows configuring the non autowireable arguments via attributes, this would be cleaner than defining them in core.services.yml: https://symfony.com/doc/current/service_container/autowiring.html#fixing...

The 3049525-aliases-only-10.0.x branch only adds aliases to core.services.yml and doesn't attempt any autowiring yet.

It also adds a test to ensure that service classes with no interface have aliases, and service interfaces that have exactly one concrete implementation have aliases. This should ensure that all non-ambiguous core services can be autowired.

The test fail is due to aliases being set for http_client, but I don't understand why:

  http_client:
    class: GuzzleHttp\Client
    factory: ['@http_client_factory', 'fromOptions']
  GuzzleHttp\ClientInterface: '@http_client'
  Psr\Http\Client\ClientInterface: '@http_client'

If I comment out both aliases then the tests pass again.

-    arguments: ['%app.root%', '%site.path%', '@cache_tags.invalidator.checksum']
+    autowire: true
+    arguments:
+      $root: '%app.root%'
+      $site_path: '%site.path%'

As a contrib maintainer, what am I looking at? When do I need to do what? When can I do what? As in, are named arguments supported currently *vague hand waving for currently*? Also, I had no idea $foo is a valid string key, good to know.

Drupal isn’t inventing these things. They’re well documented by Symfony Framework. I don’t understand what you are asking or maybe I am missing your point.

@fougere thank you for the fix - but this worries me that contrib or custom code may be broken in a similar way by this change. Why does the incorrect alias cause the test to fail, when it shouldn't be being used yet? I was thinking this might be due to a bug in our container builder - maybe something we haven't ported properly from Symfony?

@longwave:
http_client is decorated in advisory_feed_test.services.yml.
The decoration logic behaves differently if the decorated service already has an alias, and uses it instead of creating a new definition (DecoratorServicePass.php:66).

Which led to the new alias changing the test behavior.

As a side note, I did not understand what role the advisory_feed_test modules plays.
The http_client decorator it creates is immediately overwritten in SecurityAdvisoriesFetcherTest::setTestFeedResponses.
It may have been used at some point, but I think it no longer serves a purpose.

You are right, the new aliases may impact custom code.
But I think it would be limited to rare unit tests that manipulate the container, with no runtime consequence.

@chx You don't need to do anything. But this patch should allow e.g.

  user.permissions:
    class: Drupal\user\PermissionHandler
    arguments: ['@module_handler', '@string_translation', '@controller_resolver']

to become:

  user.permissions:
    class: Drupal\user\PermissionHandler
    autowire: true

The container will use the parameter types declared in the service constructor to discover which services should be injected, instead of having to manually specify them. So if later on you want to add a new service, you just add it to your constructor and (in most cases) it will just work without also having to keep the services.yml file in sync.

#3021898: Support _defaults key in service.yml files for public, tags and autowire settings will then simplify this further so you can specify autowire: true once for all services in your services.yml file, instead of having to repeat it for each service.

@longwave, thanks for the explanation.

I am sorry for the vagueness of the following concern, I am still wrapping my head around this. I looked at this patch and read https://symfony.com/doc/current/service_container/autowiring.html#autowi... and I am wondering whether has this been explored in the context of multiple modules interfering with each other? Like, you have two modules implementing some interface as a service and both work independently but what happens when you install both? This is not a concern Symfony would have, this is Drupal specific due to our modules.

I guess this ties into #41 -- currently two modules decorating the same service just work as you'd expect, indeed this is why decorating is better than altering the existing service definition. How will that work...?

It's also possible there's a better meta issue for these concerns , please feel free to redirect me to there. These concerns are not directly tied to this patch, after all.

autowiring only works if an interface has a single service for it, if it's multiple then it won't, you don't need any extra modules for that, there are examples for that in core too: cache backends are an obvious one.

A situation where your site works and then you add another module and that breaks some other module is possible but I think not that common. Most cases already where that applies already come with core like the mentioned caching or loggers. I suppose for example registering a service for a replica database connection could be a problem as module will expect to have only one of them.

But it's not that different to existing problems when for example two modules both try to replace the same service.

Potentially every module using service provider could lead to issues. Decorated services also may cause issues

Like, you have two modules implementing some interface as a service and both work independently but what happens when you install both?

Do you have an example of where this might happen? This behaviour depends on declaring the interface alias in services.yml. I would suggest that no module should declare an alias for an interface that does not belong to it.

The replica database is a great example of ambiguous classes. As you can see in the MR:

  database:
    class: Drupal\Core\Database\Connection
    factory: Drupal\Core\Database\Database::getConnection
    arguments: [default]
  Drupal\Core\Database\Connection: '@database'
  database.replica:
    class: Drupal\Core\Database\Connection
    factory: Drupal\Core\Database\Database::getConnection
    arguments: [replica]

So if you autowire Drupal\Core\Database\Connection you will get the @database service by default, as 99% of the time that is what you want.

The comment statistics service uses both:

  comment.statistics:
    class: Drupal\comment\CommentStatistics
    arguments: ['@database', '@current_user', '@entity_type.manager', '@state', '@database.replica']

This can still be autowired by specifying the argument explicitly:

  comment.statistics:
    class: Drupal\comment\CommentStatistics
    autowire: true
    arguments:
      $database_replica: '@database_replica'

Symfony 6.1 has an even better way of doing this with PHP 8 attributes: https://symfony.com/blog/new-in-symfony-6-1-service-autowiring-attributes

Eventually I hope to implement this in core, so the CommentStatistics controller could specify the service directly in the constructor:

  public function __construct(
    Connection $database,
    AccountInterface $current_user,
    EntityTypeManagerInterface $entity_type_manager,
    StateInterface $state,
    #[Autowire(service: 'database.replica')]
    Connection $database_replica = NULL,
) {...

To me, this is much better DX than having to keep the constructor and the separate services.yml in sync.

"I would suggest that no module should declare an alias for an interface that does not belong to it." -- as I said, I am not in the know of what's best, if that's what we suggest, great, we should document it and put the URL of the documentation on top of core.services.yml since that's where most people meet with services definitions.

"Symfony 6.1 has an even better way of doing this with PHP 8 attributes" -- then why are we not using this? Once again, I have no idea. I only know this is a D10 issue and back in February, D10 PHP requirement was raised to PHP 8.1 so attributes are in.

As I understand it 9.5.x and 10.0.x are supposed to be feature compatible, so we can't add PHP 8-only features until 10.1.x. But the older autowiring methods should work in 9.5.x so hopefully this can be committed earlier?

Also, it tends to be the case that smaller/simpler patches get reviewed and committed more quickly, I've already reduced scope here to only adding the interface aliases instead of trying to autowire core as well. Happy to open some followups to explore the remaining work here.

Moved the registration of kernel and service_container aliases to where the services are configured in DrupalKernel.

Also added a test for autowiring the core database and kernel services to AutowireTest, to prove that with this MR we can autowire anything aliased in core.services.yml or in the kernel itself.

Updated IS. Once we have core services aliased and #3021898: Support _defaults key in service.yml files for public, tags and autowire settings implemented we can move forward with autowiring core and core modules in followups.

As part of this we'll need to remove

  public function register($id, $class = NULL): Definition {
    if (strtolower($id) !== $id) {
      throw new \InvalidArgumentException("Service ID names must be lowercase: $id");
    }

in the two places it exists in ContainerBuilder.

That exception is showing up explicitly in #3314137: Make Automatic Updates Drupal 10-compatible since #3284422: [META] Symfony 6.2 compatibility due to changes in Symfony 6.2's FileLoader — see #3314137-27: Make Automatic Updates Drupal 10-compatible and #3314137-28: Make Automatic Updates Drupal 10-compatible for details. In #3314137-29: Make Automatic Updates Drupal 10-compatible I discovered that even Drupal core's existing autowiring test technically violates the current rule in core, by somehow bypassing the cited code above:

😬

So I agree with @longwave in #2503567-49: Only allow lowercase service and parameter names [part 2]. Closed that issue in favor of this one.

#3021898: Support _defaults key in service.yml files for public, tags and autowire settings landed on July 13. 👍
I marked #3295751: Autowire core services that do not require explicit configuration as postponed on this — it suffers from the same challenges I raised in #53.

@longwave suggested we revert the restriction mentioned in #53 in a new child issue of #3228629: [meta] Bring dependency injection more in line with upstream Symfony component rather than this one. But I was puzzled why this was able to pass tests at all. Then I saw: because this uses \Drupal\Core\DependencyInjection\ContainerBuilder::setAlias(), which oddly does not have this restriction; only \Drupal\Core\DependencyInjection\ContainerBuilder::register() and \Drupal\Core\DependencyInjection\ContainerBuilder::set() do 😅 (Which makes sense given the title.)

So … it does seem that I misunderstood the scope of this issue, so I closed #2503567 incorrectly in favor of this one; I should've closed it in favor of the new issue 😅

Created #3320315: Allow uppercase service IDs/names — necessary for autowiring support.

#3320315: Allow uppercase service IDs/names — necessary for autowiring support is in — let's get this moving forward again? :D

MR doesn't apply anymore

This looks great! But … don't we need a test to ensure that every single core service has these interface aliases? And actually, "core service" as in "service anywhere in core/", not just core.services.yml? 🤔

Keeping at Needs review to get more input.

> every single core service has these interface aliases

We don't want every core service aliased, as many of them aren't intended to be used by other services (event listeners, etc.). And we can't provide an interface alias for classes that have multiple implementations, as we don't know which one to choose.

But, inspired by some of your past coverage testing, I did add test coverage for this! See AutowireTest::testCoreServiceAliases() - this ensures that classes that don't have an interface, and classes that are the only implementation of their interface, are provided with an alias. This is what failed in #58 and notified @larowlan that core's aliases were not up to date.

D'oh, of course, good point!

What about all public, non-tagged services?

But, inspired by some of your past coverage testing, I did add test coverage for this! See AutowireTest::testCoreServiceAliases()

Woah, I totally missed that, because I didn't even look for it — because I mistakenly assumed this wasn't ready yet since I didn't see any updates in core/modules/**/*.services.yml!

Well in this case … I think this is clearly a net improvement and a huge leap forward! This probably deserves a change record?

Both using these aliases in core modules, and adding aliases for core modules, can be done in followups; this still enables contrib/custom code to start using autowiring for any core services they might want to use. We can extend AutowireTest to cover core modules later.

Also not sure why this issue is related to itself, removing that link.

Added a draft change record, it's quite a complicated part of Symfony so reviews and edits welcome to make this easier to grok: https://www.drupal.org/node/3323122

#65: 💯

#66: I think that change record is already pretty clear actually 😊👍 Just added some minor formatting improvements.

Committed and pushed ecc921dd9e to 10.1.x and e4e4b54c1c to 10.0.x. Thanks!

Discussed with @catch and @longwave. I feel that there is a benefit in backporting the aliases to 9.5.x so contrib can leverage this in modules that are 9.5.x and 10 compatible. If we do that we should probably remove the "all the core services have an alias" test.

Will sort out the change record once we've made a decision about whether we are going to backport this to 9.5.x

Yay! I already stumbled upon this (#3103682-20: CDN module 4.x: no new features + drop BC layers + drop now irrelevant headers + PHP >=8.1 + Drupal >=9.4) when I tried to adopt autowiring in the main contrib module I maintain — so this definitely helps bring an improved DX to many contrib modules! 🥳

And if the backport to Drupal 9.5 happens (which I agree is harmless), then contrib modules can start adopting this sooner — modules that only support the current minor can start using autowiring when Drupal 9.5 & 10.0 ship; modules that support the previous minor too will be able to do so in 6.5 months.

OTOH, if we don't backport, most of contrib will only be able to start using this much later.

Backported to 9.5.x, I kept the test and added aliases for the extra services in 9.5.x.

Why didn't it cover services declared in core modules? What's wrong with system.services.yml, user.services.yml, etc?

@Chi: I asked the same thing in #62 — see @longwave's answer in #63.

@Chi follow-up issues for anything in core modules that would be useful are v welcome!

So the autowire will only work when absolutely all service dependencies have aliases. Correct?

What services in system.services.yml or user.services.yml do you need to inject? Most of them are tagged services such as access checkers, event subscribers, or theme negotiators - it doesn't seem useful to be able to autowire these into something else.

user.flood_control is very useful. Other services might be used much less frequently, but I am not 100% sure.

Not sure this is the right way to fix either of those errors, but the tests should pass now.

Re #78-80 we can open followups to add aliases for core module services, and also to actually start using autowiring in core.

+++ b/core/tests/Drupal/KernelTests/Core/DependencyInjection/AutowireTest.php
@@ -40,8 +40,12 @@ public function testAutowire(): void {
   /**
    * Tests that core services have aliases correctly defined where possible.
+   *
+   * @group legacy
    */
   public function testCoreServiceAliases(): void {
+    $this->expectDeprecation('Drupal\Component\Bridge\ZfExtensionManagerSfContainer is deprecated in drupal:9.4.0 and is removed from drupal:10.0.0. The class has moved to \Drupal\aggregator\ZfExtensionManagerSfContainer. See https://www.drupal.org/node/3258656');
+
     $services = [];
     $aliases = [];
     foreach (Yaml::decode(file_get_contents('core/core.services.yml'))['services'] as $id => $service) {

I think we should see if the service is deprecated and if it is not insist on an alias.

Great idea. Then we can revert all the changes needed to fix the tests and remove aliases for deprecated services, given that nobody will be autowiring them anyway.

In fact let's revisit that a bit.

Tagging

Committed 365dfc2 and pushed to 9.5.x. Thanks!

Opened #3325557: Enable more service autowiring by adding interface aliases to core modules for #75-80

This feature introduced a critical issue:
#3325824: New autowiring breaks Container::get() service loading

Discussed #3325824: New autowiring breaks Container::get() service loading in slack with @alexpott, @xjm, @longwave. Whether #3325824: New autowiring breaks Container::get() service loading is a regression is very borderline - it relies on code execution order (essentially a cache race condition) which is a bug, and which this patch 'fixes' as a side-effect. However since it's very hard to tell how many (or if) contrib modules will be affected by it, we decided to roll this back from 9.5.x and 10.0.x

Leaving in 10.1.x, I think we just need a release note there. Wrote the release note as if we fixed the actual bug (instead of it disappearing due to a side-effect)

I updated the change record's branch and version fields to reflect that this is now only in 10.1.

+++ b/core/core.services.yml
@@ -1168,22 +1291,28 @@ services:
   access_manager.check_provider:
     class: Drupal\Core\Access\CheckProvider
     calls:
       - [setContainer, ['@service_container']]
     public: false
+  Drupal\Core\Access\CheckProviderInterface: '@access_manager.check_provider'

We have a problem here. The alias is public but the service is not. Given this is 10.1.x I think we can fix this in a follow-up issue. Because relatedly setting this alias to private doesn't work due to https://github.com/symfony/symfony/pull/48591

This was already added to the 10.1.0 release notes draft.

There's TrustedCallbackInterfacewhich needs exclude from aliases, faced in #3354584-14: Deprecate TrustedCallbackInterface in favour of TrustedCallback attribute