Support _defaults key in service.yml files for public, tags and autowire settings

I've given the patch a quick test and it seems to work well.

It's allowed me to change my drupalversary.services.yml from:

services:
  cache.drupalversary:
    class: Drupal\Core\Cache\CacheBackendInterface
    arguments:
      - drupalversary
    factory: cache_factory:get
    tags:
      - name: cache.bin

  Drupal\drupalversary\Controller\UserController:
    autowire: true

  Drupal\drupalversary\Service\AccountRetriever:
    autowire: true

  Drupal\drupalversary\Service\CreatedDateParser:
    autowire: true

to:

services:
  _defaults:
    autowire: true

  cache.drupalversary:
    class: Drupal\Core\Cache\CacheBackendInterface
    arguments:
      - drupalversary
    factory: cache_factory:get
    tags:
      - name: cache.bin

  Drupal\drupalversary\Controller\UserController: ~
  Drupal\drupalversary\Service\AccountRetriever: ~
  Drupal\drupalversary\Service\CreatedDateParser: ~

Autowiring is already supported, as of #2611816: Update to symfony 2.8, so I don't think this needs to be postponed on #3021803: Document and add tests for service autowiring since this thread is about support for _defaults, not autowiring specifically.

This patch is a re-roll of #3 and includes test coverage for the new _defaults: autowiring, tags, and public.

There are still some TODOs in the code, not sure what to do with those.

Tagging as this looks like a good next target if #3021803: Document and add tests for service autowiring gets in.

+++ b/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
@@ -42,6 +42,14 @@ class YamlFileLoader
+// @todo I think remove these.
+//        'autoconfigure' => 'autoconfigure',
+//        'bind' => 'bind',

I think it's OK to allow both these, autoconfiguration would be nice in the future (e.g. not having to tag event subscriber services manually) as would argument binding (so we can autowire container parameters). We might as well move towards allowing everything Symfony allows unless there is good reason not to.

I guess we could add tests for both binding and autoconfiguration, as both are handled by Symfony compiler passes, and with a bit of support here should hopefully just work. Autoconfiguration would also need us to support _instanceof though - maybe we should leave these to a followup?

Rerolled with some minor changes to make tests pass following #3187074: [Symfony 5] Services are private by default

Still need to remove the bind and autoconfigure parts, and Symfony's YamlFileLoader has also moved on since the original patch - I think we should bring any new additions here in line with latest Symfony.

Make cspell happy.

Copied the relevant parts of parseDefaults() and parseDefinition() directly from Symfony 4.4 so we are closer to upstream. We can do more, but that's out of scope of this issue and may have some BC concerns (Symfony does a lot of additional validation that we don't currently do, for example).

NW for MR feedback.

Added unit tests for all exceptions touched here in YamlFileLoader.

1. +++ b/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
   @@ -121,12 +125,71 @@ private function parseDefinitions($content, $file)
   +                if (!\is_string($name) || '' === $name) {
   
   @@ -271,31 +347,37 @@ private function parseDefinition($id, $service, $file)
   +            if (!\is_string($name) || '' === $name) {
   

   I am missing testing for when $name is set and is not a string.

2. +++ b/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
   @@ -121,12 +125,71 @@ private function parseDefinitions($content, $file)
   +                    if (!is_scalar($value) && null !== $value) {
   
   @@ -271,31 +347,37 @@ private function parseDefinition($id, $service, $file)
   +                if (!is_scalar($value) && null !== $value) {
   

   Missing testing for the $value is null.

3. +++ b/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
   @@ -174,6 +242,17 @@ private function parseDefinition($id, $service, $file)
   +            // Drupal services are public by default.
   +            $definition->setPublic(true);
   
   @@ -195,9 +274,6 @@ private function parseDefinition($id, $service, $file)
   -            $definition->setPublic(true);
   

   With this change will, when the $service['parent'] is set, not be set public by default. Is that what we want? And will this be a BC break? Not sure, just asking the question.

4. +++ b/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
   @@ -121,12 +125,71 @@ private function parseDefinitions($content, $file)
   +    /**
   +     * @param array  $content
   +     * @param string $file
   +     *
   +     * @return array
   +     *
   +     * @throws InvalidArgumentException
   +     */
   +    private function parseDefaults(array &$content, string $file): array
   

   Can we improve the docblock? It is not very informing now.

5. Again just asking the question: Can the added InvalidArgumentExceptions to the method YamlFileLoader::parseDefinition() result is a BC break?

6. +++ b/core/modules/system/tests/modules/services_defaults_test/services_defaults_test.services.yml
   @@ -0,0 +1,16 @@
   +    autowire: true
   

   Can we add a test to see if overriding the _default autowire is true works.

7. +++ b/core/tests/Drupal/KernelTests/Core/DependencyInjection/ServicesDefaultsTest.php
   @@ -0,0 +1,62 @@
   +    $this->assertSame(
   +      $testServiceDefinition->getTags(),
   +      $testInjection1Definition->getTags());
   

   Nitpick: Can we put this on a single line or move the last ");" to the next line.

8. +++ b/core/tests/Drupal/KernelTests/Core/DependencyInjection/ServicesDefaultsTest.php
   @@ -0,0 +1,62 @@
   +    // Ensure overridden tag works.
   +    $this->assertTrue($testInjection2Definition->hasTag('zee.bang'));
   

   Can we add testing that the _default tags are still there too and are not lost.

9. +++ b/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
   @@ -31,6 +31,11 @@
   +    private const DEFAULTS_KEYWORDS = [
   

   Needs a docblock.

10. Does this issue need a CR?

All code changes look good to me.
All my points have been addressed.
Testing has been added.
I have created a CR.
For me it is RTBC.

Testbot is happy again. Back to RTBC.

Committed/pushed/cherry-picked the respective patches to 10.1.x/10.0.x and 9.5.x, thanks!

Since this isn't disruptive or an API change etc., I don't think it needs a release note, just the CR seems fine. If we start changing core's service definitions, we should probably do one then.

74dbb97f - Remove support for autoconfigure and bind for now.

Do we have a follow-up issue to re-add the "bind"?
I think it would be quite useful to have :)

EDIT: Yes, this is the follow-up: #3108565: Update the Yaml file Loader functionality to simplify service configuration