Document and add tests for service autowiring

👍 This is my favorite feature of SF.

Should we profile this?

Some of the current passes are already very slow and this seems like something that could be quite expensive as well. Nobody cares much about that in Symfony world, but container rebuild is 50% + of the installation time of a large install profile...

Reading the patch does this mean someone could write a contrib module providing the same functionality?

Here is just a quick doc entry.

Is it a requirement to add the compiler passes provided by symfony in the \Drupal\Core\CoreServiceProvider class?

If im not mistaken i think they are added/enabled by default.
Im pretty sure the container builder uses the default compiler which on instantiation takes the default pass config containing all the symfony provided compiler passes configured by default.

See:

\Symfony\Component\DependencyInjection\ContainerBuilder::getCompiler
\Symfony\Component\DependencyInjection\Compiler\Compiler
\Symfony\Component\DependencyInjection\Compiler\PassConfig

Also a dump of $container->getCompilerPassConfig() in the core service provider shows the autowire passes etc included.

Does this patch not need just to contain the yaml parsing tweaks?

Hi o/

I'm from the Symfony world, so apologies if I violate any etiquette unintentionally!

I'm playing with this currently, and it seems like it mostly works. The big blocker (and it *is* big) is not exactly to autowiring, but the nice new features like _defaults and service-auto registration don't work, because the YamlFileLoader seems like it's getting increasingly out of date. So, autowiring works, but it's not possible use _defaults in a module's services file to default *all* your services to autowire. And it's also not possible for a module to auto-register a bunch of services from one or more directories. This limits autowiring's usefulness.

The other big piece is that core will need to add aliases so that *their* classes can be autowired. That's because autowiring works by reading the type-hint and then looking in the container for a service with that id. So, for example, to enable the config.factory service to be autowired, core could do this:

services:
    config.factory:
        # ... all the normal stuff

    Drupal\Core\Config\ConfigFactoryInterface: 
        alias: 'config.factory'

That will introduce a new "alias" to config.factory, which matches the type-hint that should be used.

So not a lot of work needs to be done to allow end-users to take advantage of the autowiring features, but some work *does* need to be done.

Cheers!

Sorry, to clarify, about the service aliases.

In Symfony 3, there is still the "magic" autowiring that works by trying to match 1 service with the specific type. That is deprecated, and users could choose to turn it off entirely: https://github.com/symfony/symfony/pull/26278/files

By adding aliases, core would be enabling users to use the new, non-deprecated way.

Cheers!

@weaverryan Thank you for those details.

Why do we need both "autowired" and "autowire"?

in these patches:

        if (isset($service['autowired'])) {
            $definition->setAutowired($service['autowired']);
        }

around line 300:

        if (isset($service['autowire'])) {
            $definition->setAutowired($service['autowire']);
        }

Switching to "autowire" passes my tests locally.

In other words, aside from minor changes in YamlFileLoader, autowire is already supported, per #2611816: Update to symfony 2.8
Functionally, all we need is a test, and support for service definitions as strings.

1. +++ b/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
   @@ -143,6 +149,10 @@ private function parseDefinition($id, $service, $file)
   +            $service = array();
   

   nit []

2. +++ b/core/tests/Drupal/KernelTests/Core/DependencyInjection/AutowireTest.php
   @@ -0,0 +1,32 @@
   +      $this->container->get('Drupal\autowire_test\TestInjection'),
   +      $this->container->get('Drupal\autowire_test\TestService')->getTestInjection()
   ...
   +      $this->container->get('Drupal\autowire_test\TestInjection2'),
   +      $this->container->get('Drupal\autowire_test\TestService')->getTestInjection2()
   

   should we use ::class here?

@weaverryan thanks for your session at Drupalcon, I added #3049524: Support _defaults in services.yml files for improved autowiring for adding _defaults and resources support

I also added #3049525: Enable service autowiring by adding interface aliases to core service definitions for the alias support (Symfony 4).

Tagged all three with Service autowiring

updated patch with larowlan's suggestions from #20

re #21: also check out parent Plan with links to a few more symfony-related updates #3021900: Bring new features from Symfony 3/4/5/6 into our container

Unrelated test harness failure.

1) Drupal\Tests\system\Functional\Update\UpdatePathTestBaseFilledTest::testUpdatedSite
Exception: Warning: file_put_contents(/var/www/html/sites/simpletest/.htaccess): failed to open stream: Permission denied
file_save_htaccess()() (Line: 376)

grrrr, simpletest!

From the Symfony docs:

> The autowiring system looks for a service whose id exactly matches the type-hint: so App\Util\Rot13Transformer.

Doesn't that mean that autowired services can't be swapped for a different implementation, since the typehint is a class rather than an interface?

Doesn't that mean that autowired services can't be swapped for a different implementation, since the typehint is a class rather than an interface?

This was my first thought too, but aliases solve this problem:

Aliases are used by the core bundles to allow services to be autowired. For example, MonologBundle creates a service whose id is logger. But it also adds an alias: Psr\Log\LoggerInterface that points to the logger service. This is why arguments type-hinted with Psr\Log\LoggerInterface can be autowired.

So, here's how i *think* it would work in Drupal context:
* canonical service names do not change
* add aliases in services.yml to link interfaces with service names
* existing ServiceProvider mechanism (and implementations) of replacing services at run-time remains the same: we can still fetch the definition using the existing service names

slootjes's github has an example of aliasing a core drupal service using interface:

  Drupal\Core\Entity\EntityTypeManagerInterface:
      alias: 'entity_type.manager'

And just to reiterate: Drupal's container already supports autowiring and aliases.
The patch in this thread is only adding some test coverage and docs for the existing feature.

I think one problem is that we have have a few types of services that share the same interface, we don't have one logger, we have many different logger services, each logging to a certain channel. And we have many different cache "bins". So unless I'm missing something, if you implement one of those then you can't use autowiring? Or we would need to add a unique interface for e.g. each cache bin.

No, it uses parameter name and matches the service alias. e.g. $entity_type.manager => entity_type.manager

Except that it can't figure out EntityTypeManager correctly by default. Nor $database => Connection...
For example, here's what's needed on top of this patch to get LB to work:

diff --git a/core/core.services.yml b/core/core.services.yml
index a4327bda9b..6e99385acc 100644
--- a/core/core.services.yml
+++ b/core/core.services.yml
@@ -357,6 +357,7 @@ services:
     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
@@ -559,6 +560,7 @@ services:
     parent: container.trait
     tags:
       - { name: plugin_manager_cache_clear }
+  Drupal\Core\Entity\EntityTypeManagerInterface: '@entity_type.manager'
   entity_type.repository:
     class: Drupal\Core\Entity\EntityTypeRepository
     arguments: ['@entity_type.manager']
diff --git a/core/modules/layout_builder/layout_builder.services.yml b/core/modules/layout_builder/layout_builder.services.yml
index 6e94ed74d2..c8e46698fd 100644
--- a/core/modules/layout_builder/layout_builder.services.yml
+++ b/core/modules/layout_builder/layout_builder.services.yml
@@ -1,7 +1,7 @@
 services:
   layout_builder.tempstore_repository:
     class: Drupal\layout_builder\LayoutTempstoreRepository
-    arguments: ['@tempstore.shared']
+    autowire: true
   access_check.entity.layout_builder_access:
     class: Drupal\layout_builder\Access\LayoutBuilderAccessCheck
     tags:
@@ -16,33 +16,33 @@ services:
   plugin.manager.layout_builder.section_storage:
     class: Drupal\layout_builder\SectionStorage\SectionStorageManager
     parent: default_plugin_manager
-    arguments: ['@context.handler']
+    autowire: true
   layout_builder.routes:
     class: Drupal\layout_builder\Routing\LayoutBuilderRoutes
-    arguments: ['@plugin.manager.layout_builder.section_storage']
+    autowire: true
     tags:
      - { name: event_subscriber }
   layout_builder.param_converter:
     class: Drupal\layout_builder\Routing\LayoutTempstoreParamConverter
-    arguments: ['@layout_builder.tempstore_repository', '@plugin.manager.layout_builder.section_storage']
+    autowire: true
     tags:
       - { name: paramconverter, priority: 10 }
   cache_context.layout_builder_is_active:
     class: Drupal\layout_builder\Cache\LayoutBuilderIsActiveCacheContext
-    arguments: ['@current_route_match']
+    autowire: true
     tags:
       - { name: cache.context}
   cache_context.route.name.is_layout_builder_ui:
     class: Drupal\layout_builder\Cache\LayoutBuilderUiCacheContext
-    arguments: ['@current_route_match']
+    autowire: true
     tags:
       - { name: cache.context }
   layout_builder.sample_entity_generator:
     class: Drupal\layout_builder\Entity\LayoutBuilderSampleEntityGenerator
-    arguments: ['@tempstore.shared', '@entity_type.manager']
+    autowire: true
   layout_builder.render_block_component_subscriber:
     class: Drupal\layout_builder\EventSubscriber\BlockComponentRenderArray
-    arguments: ['@current_user']
+    autowire: true
     tags:
       - { name: event_subscriber }
   logger.channel.layout_builder:
@@ -50,4 +50,4 @@ services:
     arguments: ['layout_builder']
   inline_block.usage:
     class: Drupal\layout_builder\InlineBlockUsage
-    arguments: ['@database']
+    autowire: true

That's not a reason not to do this, but it is something to be aware of and to document.
And to be ready for the additions needed in core.services.yml

one problem is that we have have a few types of services that share the same interface

Symfony documentation suggests using interface aliases to deal with this.
https://symfony.com/doc/current/service_container/autowiring.html#dealin...

Though we could simply not to use autowire for such cases.

Imho, The benefits of autowiring and autoconfiguring for Drupal core are unclear. Too much magic just to spare a few lines in configuration files. We need to support this to align container implementation with upstream project, but we don't have to enable it for core services.

This is some good discussion: should we move to another thread?
The patch so far is ostensibly just test coverage for existing autowire support.

There's a planning thread here: https://www.drupal.org/project/drupal/issues/3021900
And a task to update core service aliases here: https://www.drupal.org/project/drupal/issues/3049525
If we want to continue discussion here, the IS should be updated

The patch so far is ostensibly just test coverage for existing autowire support.

Except that if you try to use autowiring right now, you won't be able to compile your container. Because you need this change:

+++ b/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
@@ -120,7 +120,13 @@ private function parseDefinitions($content, $file)
-            $service['tags'][] = ['name' => '_provider', 'provider' => $provider];
+            // If service is a string then it is the short form of an alias.
+            if (is_array($service)) {
+                $service['tags'][] = [
+                    'name' => '_provider',
+                    'provider' => $provider
+                ];
+            }

Also, enabling a new feature of core without discussing how we're going to manage the follow-ups feels irresponsible. I'm not trying to hold up this issue, I think autowiring is great.
Tagging for some FM input here.

I think the _only_ thing I'm missing is how to go about adding the aliases to core.services.yml when there are ambiguities. Say I want to rewrite LB to use autowiring, as above, I'd also need to change core.services.yml. If that's fine, then I think we're good here! I just don't want to get into another situation where we have the _ability_ to do something without all the committers being on board when core devs actually try to do that thing.

Autowiring is already working (https://www.drupal.org/docs/drupal-apis/services-and-dependency-injectio...), but what is not working, is the ability to autowire services without a cache rebuild. Like it's provided by plain symfony. Are you talking about this?

I think Drupal requires a cache rebuild for any changes related to services.
The main points are already stated on #15. No _defaults and no aliases.

Inspired by a talk @weaverryan gave at Drupalcon I created a small module to alleviate those two points.
https://gitlab.com/upstreamable/drupal-autoservices

In a team we tried to work with autowired services in d8
We got very frequently a very annoying problem

If user1 change a service definition or I'm not sure what on a service. Use2 take code changes with git pull and drush cr (and everything else) is broken

Because drush caches are somehow exploding

If someone want more explanation I could try to write done a how to reproduce.
But I just think like #39 pointed out, the service injection caching in drupal is not "autowire" functionality compatible

I wonder if that is related to the problem I found that Drupal doesn't handle a module being moved[*}:

1. drush cr
2. move a contrib module's folder
3. drush cr
4. BOOM

I reported it as a Drush bug https://github.com/drush-ops/drush/issues/4661, but the maintainer there says it's to do with drupal_rebuild().

The problem is basically that the {cache_container} table is not emptied early enough.

[*] This is a fairly recent new problem, I think -- Drush used to be fine about modules being moved if you did a rebuild.

Going to go ahead and mark this RTBC to try and get a framework manager review. This is a simple patch that starts to allow us to use autowiring, once this is committed we can explore in other issues what happens if we start adding interface aliases to core.services.yml and actually enabling autowiring across core.

Personally I would love to see the day where plugins can be autowired and we don't need to manually write create() methods any more, alongside PHP 8 constructor promotion this would make our code much more concise. Autowiring services to start with would seem to be a step towards that.

+++ b/core/core.api.php
@@ -852,6 +852,11 @@
+ * more about this on https://symfony.com/doc/current/service_container/autowiring.html

+++ b/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
@@ -120,8 +120,12 @@ private function parseDefinitions($content, $file)
+                    'name' => '_provider',
+                    'provider' => $provider

@@ -145,6 +149,10 @@ private function parseDefinition($id, $service, $file)
+            $service = [];

there's some phpcs issues here

Addressing the FM review

In #35 alexpott is in favour of this so I think the only thing holding this up is the comment from @tim.plunkett in #36

I think the _only_ thing I'm missing is how to go about adding the aliases to core.services.yml when there are ambiguities. Say I want to rewrite LB to use autowiring, as above, I'd also need to change core.services.yml. If that's fine, then I think we're good here! I just don't want to get into another situation where we have the _ability_ to do something without all the committers being on board when core devs actually try to do that thing.

I think to resolve that we need a policy documented somewhere, so there's no ambiguity.

YamlFileLoader ignores our phpcs conventions as it is a copy of Symfony code, so that actually looks correct to me for Symfony style. Agree that core.api.php needs rewrapping though.

What do we need to do to get a policy for interface aliases in place? Can we do it here or do we need a separate [policy, no patch] issue? To me I don't see any issue with adding interface aliases to core.services.yml where appropriate, nobody is forced to use them but it opens up the possibility of using autowiring. It might be worth trying an experimental patch where we autowire as much of core as possible in one go, just to see how many aliases we would need to add?

> It might be worth trying an experimental patch where we autowire as much of core as possible in one go, just to see how many aliases we would need to add

I think that would be a good idea

> What do we need to do to get a policy for interface aliases in place

I'll put it on the next committer meeting agenda

Having the example from the LB one above is probably a good start for discussion

Agree that we could put this in without an alias policy, the code won't change whatever policy we adopt.

I already started on a full core autowiring patch, will try to finish that at some point.

It would really help in evaluating the impact of this change to have a draft change record of what the before/after code might look like for a given module.

Added a draft CR with an example autowired service from core.services.yml: https://www.drupal.org/node/3218156

Feedback on the change record (I was going to take a stab at this but you beat me to it!)

By defining aliases for each of the interfaces that are specified in the constructor for this service...The container build process can then automatically wire up services that have these dependencies.

It seems to me this is either overstated or ambiguous. Ambiguous in so far this example adds as much boilerplate as it removes. As a developer myself who loves Symfony's autowiring compared to the status quo in Drupal, autowiring the container sounds great. But if I have to add aliases for services I don't manage (core) then it's unclear to me from the CR:

1) where I should add these aliases - in my own module or hacking core?

2) what the benefit is, if you must add the boilerplate?

I think this sets the groundwork for really cool stuff in contrib and core, but most of that coolness is postponed until core adds aliases for the short service names that are ubiquitous in the Drupal ecosystem, since we have no real convention for Symfony service autoconfiguration as well.

Overstated in so far as, it seems right now this is really mostly helpful for services that are solely or mostly dependent on services "I" define in custom land. Then I am responsible for handling the aliasing and can choose to take advantage of this if it actually saves time.

Thoughts on the above?

@bradjones1 Please feel free to update the CR as you see fit. I think I was a bit over eager for the initial version but as linked above I was hoping that #3049525: Enable service autowiring by adding interface aliases to core service definitions would follow shortly after this issue and then core services would be autowirable and remove quite a bit of boilerplate. I think if that issue does get in then it would be worth using the same CR for both issues, rather than developers having to read about autowiring and then learning separately they can actually use it for core services.

Thanks to you both; I updated the change record draft.

+++ b/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
@@ -147,6 +151,10 @@ private function parseDefinition($id, $service, $file)
+        if (null === $service) {
+            $service = [];
+        }

I think at least in theory, services can be set to NULL independently of autowiring. Therefore, can we pull this out into a separate issue and add a unit test for it in Drupal\Tests\Core\DependencyInjection\YamlFileLoaderTest? The corresponding Symfony commit is https://github.com/symfony/symfony/commit/28a1b5ac4779a5cac48b716e150b67..., which was all the way back in 3.3.

#33 added the "Needs framework manager review" tag in part because the patch at that time included changes needed to YamlFileLoader. However, that has since been committed separately in #2828099: Service container aliases do not work.

#59 references the only remaining change in the #51 patch to YamlFileLoader, but per that comment, I think we can do that separately as well.

Meanwhile, #41 states the opinion that we should make the aliases private. If we follow that opinion, then we don't even need to wait for #59's separate issue, as demonstrated in the patch attached here.

At which point, this issue just becomes about documenting and adding tests for what is already possible. I support both doing that in this issue, and also doing the proposed follow-up of #3049525: Enable service autowiring by adding interface aliases to core service definitions, so removing the "Needs framework manager review" tag.

Nits:

1. +++ b/core/core.api.php
   @@ -852,6 +852,11 @@
   + * Specifying dependencies explicit can be quite an effort. The container can
   

   s/explicit/explicitly

2. +++ b/core/core.api.php
   @@ -852,6 +852,11 @@
   + * also auto-wire your dependency using reflection on container build time. Read
   

   I believe Symfony always uses "autowire" without the hyphen?

3. +++ b/core/modules/system/tests/modules/autowire_test/autowire_test.services.yml
   @@ -0,0 +1,13 @@
   +  # Use an alias so an interface autowired is tested.
   ...
   +    public: false
   

   s/interface autowired/autowired interface

4. +++ b/core/modules/system/tests/modules/autowire_test/autowire_test.services.yml
   @@ -0,0 +1,13 @@
   +    public: false
   

   It's probably worth adding a note here as to why these are marked private?

NW for #61, also removing "needs change record" as we have one.

Addresses #61 and does a little bit more docs tweaking as well.

+++ b/core/core.api.php
@@ -852,6 +852,12 @@
+ * a service's arguments from the constructor's type-hints. See

I did wonder about typehint vs type hint vs type-hint but we use all three interchangeably so I think this is OK.

Nits fixed, this only adds documentation and a test, and there is further proof that we can autowire parts of core going on over at #3049525: Enable service autowiring by adding interface aliases to core service definitions so for me this is RTBC.

1. +++ b/core/modules/system/tests/modules/autowire_test/src/TestService.php
   @@ -0,0 +1,30 @@
   +  public function getTestInjection() {
   +    return $this->testInjection;
   +  }
   +
   +  public function getTestInjection2() {
   +    return $this->testInjection2;
   

   is it worth addig return typehints?

2. +++ b/core/tests/Drupal/KernelTests/Core/DependencyInjection/AutowireTest.php
   @@ -0,0 +1,35 @@
   +    $this->assertSame(
   +      TestInjection::class,
   +      get_class($this->container->get(TestService::class)->getTestInjection())
   +    );
   +    // Ensure an autowired class works.
   +    $this->assertSame(
   +      TestInjection2::class,
   +      get_class($this->container->get(TestService::class)->getTestInjection2())
   +    );
   

   why not assertInstanceOf()?

#65 addressed, back to RTBC.

+++ b/core/core.api.php
diff --git a/core/misc/cspell/dictionary.txt b/core/misc/cspell/dictionary.txt
index 80d6cb2fc6..f6328a4e3f 100644

+++ b/core/tests/Drupal/KernelTests/Core/DependencyInjection/AutowireTest.php
@@ -0,0 +1,32 @@
+  public function testAutowire() {

Superpick: this could use a : void return typehint.

Fixed #69, leaving as RTBC.

I did a little more tweaking of the proposed CR given some issue grooming I'm doing tonight: https://www.drupal.org/node/3218156/revisions/view/12318678/12332962

Adding issue credit to reviewers.

Even though I co-authored parts of this patch, this patch is small, my changes were trivial, the patch has received plenty of review, and it sat in RTBC for plenty of time for people to kick it back if they wanted to.

Therefore, pushed to 9.3.x. Thanks, everyone!