bug #31463 [DI] default to service id - *not* FQCN - when building tagged locators (nicolas-grekas)

This PR was merged into the 4.3 branch.

Discussion
----------

[DI] default to service id - *not* FQCN - when building tagged locators

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | no
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

While reviewing #30926 I realized that defaulting to the FQCN is a shortcut that isn't useful enough.
Defaulting to the service id provides the same experience in practice because service ids are FQCN by default.
But when they aren't, the service id is the proper index to default to when building the locator.

Commits
-------

52e827c9cf [DI] default to service id - *not* FQCN - when building tagged locators

src/Symfony/Component/DependencyInjection/Argument/TaggedIteratorArgument.php

@@ -21,22 +21,26 @@ class TaggedIteratorArgument extends IteratorArgument
     private $tag;
     private $indexAttribute;
     private $defaultIndexMethod;
-    private $useFqcnAsFallback = false;
+    private $needsIndexes = false;
 
     /**
      * @param string      $tag                The name of the tag identifying the target services
      * @param string|null $indexAttribute     The name of the attribute that defines the key referencing each service in the tagged collection
      * @param string|null $defaultIndexMethod The static method that should be called to get each service's key when their tag doesn't define the previous attribute
-     * @param bool        $useFqcnAsFallback  Whether the FQCN of the service should be used as index when neither the attribute nor the method are defined
+     * @param bool        $needsIndexes       Whether indexes are required and should be generated when computing the map
      */
-    public function __construct(string $tag, string $indexAttribute = null, string $defaultIndexMethod = null, bool $useFqcnAsFallback = false)
+    public function __construct(string $tag, string $indexAttribute = null, string $defaultIndexMethod = null, bool $needsIndexes = false)
     {
         parent::__construct([]);
 
+        if (null === $indexAttribute && $needsIndexes) {
+            $indexAttribute = preg_match('/[^.]++$/', $tag, $m) ? $m[0] : $tag;
+        }
+
         $this->tag = $tag;
         $this->indexAttribute = $indexAttribute;
         $this->defaultIndexMethod = $defaultIndexMethod ?: ('getDefault'.str_replace(' ', '', ucwords(preg_replace('/[^a-zA-Z0-9\x7f-\xff]++/', ' ', $indexAttribute ?? ''))).'Name');
-        $this->useFqcnAsFallback = $useFqcnAsFallback;
+        $this->needsIndexes = $needsIndexes;
     }
 
     public function getTag()
@@ -54,8 +58,8 @@ class TaggedIteratorArgument extends IteratorArgument
         return $this->defaultIndexMethod;
     }
 
-    public function useFqcnAsFallback(): bool
+    public function needsIndexes(): bool
     {
-        return $this->useFqcnAsFallback;
+        return $this->needsIndexes;
     }
 }

src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php

@@ -41,12 +41,12 @@ trait PriorityTaggedServiceTrait
      */
     private function findAndSortTaggedServices($tagName, ContainerBuilder $container)
     {
-        $indexAttribute = $defaultIndexMethod = $useFqcnAsFallback = null;
+        $indexAttribute = $defaultIndexMethod = $needsIndexes = null;
 
         if ($tagName instanceof TaggedIteratorArgument) {
             $indexAttribute = $tagName->getIndexAttribute();
             $defaultIndexMethod = $tagName->getDefaultIndexMethod();
-            $useFqcnAsFallback = $tagName->useFqcnAsFallback();
+            $needsIndexes = $tagName->needsIndexes();
             $tagName = $tagName->getTag();
         }
 
@@ -55,7 +55,7 @@ trait PriorityTaggedServiceTrait
         foreach ($container->findTaggedServiceIds($tagName, true) as $serviceId => $attributes) {
             $priority = isset($attributes[0]['priority']) ? $attributes[0]['priority'] : 0;
 
-            if (null === $indexAttribute && !$useFqcnAsFallback) {
+            if (null === $indexAttribute && !$needsIndexes) {
                 $services[$priority][] = new Reference($serviceId);
 
                 continue;
@@ -77,8 +77,8 @@ trait PriorityTaggedServiceTrait
             $class = $r->name;
 
             if (!$r->hasMethod($defaultIndexMethod)) {
-                if ($useFqcnAsFallback) {
+                if ($needsIndexes) {
-                    $services[$priority][$class] = new TypedReference($serviceId, $class);
+                    $services[$priority][$serviceId] = new TypedReference($serviceId, $class);
 
                     continue;
                 }

src/Symfony/Component/DependencyInjection/Tests/Compiler/IntegrationTest.php

@@ -292,22 +292,22 @@ class IntegrationTest extends TestCase
     public function testTaggedServiceLocatorWithIndexAttribute()
     {
         $container = new ContainerBuilder();
-        $container->register(BarTagClass::class)
+        $container->register('bar_tag', BarTagClass::class)
             ->setPublic(true)
             ->addTag('foo_bar', ['foo' => 'bar'])
         ;
-        $container->register(FooTagClass::class)
+        $container->register('foo_tag', FooTagClass::class)
             ->setPublic(true)
             ->addTag('foo_bar')
         ;
-        $container->register(FooBarTaggedClass::class)
+        $container->register('foo_bar_tagged', FooBarTaggedClass::class)
             ->addArgument(new ServiceLocatorArgument(new TaggedIteratorArgument('foo_bar', 'foo')))
             ->setPublic(true)
         ;
 
         $container->compile();
 
-        $s = $container->get(FooBarTaggedClass::class);
+        $s = $container->get('foo_bar_tagged');
 
         /** @var ServiceLocator $serviceLocator */
         $serviceLocator = $s->getParam();
@@ -317,28 +317,28 @@ class IntegrationTest extends TestCase
             'bar' => $serviceLocator->get('bar'),
             'foo_tag_class' => $serviceLocator->get('foo_tag_class'),
         ];
-        $this->assertSame(['bar' => $container->get(BarTagClass::class), 'foo_tag_class' => $container->get(FooTagClass::class)], $same);
+        $this->assertSame(['bar' => $container->get('bar_tag'), 'foo_tag_class' => $container->get('foo_tag')], $same);
     }
 
     public function testTaggedServiceLocatorWithIndexAttributeAndDefaultMethod()
     {
         $container = new ContainerBuilder();
-        $container->register(BarTagClass::class)
+        $container->register('bar_tag', BarTagClass::class)
             ->setPublic(true)
             ->addTag('foo_bar')
         ;
-        $container->register(FooTagClass::class)
+        $container->register('foo_tag', FooTagClass::class)
             ->setPublic(true)
             ->addTag('foo_bar', ['foo' => 'foo'])
         ;
-        $container->register(FooBarTaggedClass::class)
+        $container->register('foo_bar_tagged', FooBarTaggedClass::class)
             ->addArgument(new ServiceLocatorArgument(new TaggedIteratorArgument('foo_bar', 'foo', 'getFooBar')))
             ->setPublic(true)
         ;
 
         $container->compile();
 
-        $s = $container->get(FooBarTaggedClass::class);
+        $s = $container->get('foo_bar_tagged');
 
         /** @var ServiceLocator $serviceLocator */
         $serviceLocator = $s->getParam();
@@ -348,33 +348,59 @@ class IntegrationTest extends TestCase
             'bar_tab_class_with_defaultmethod' => $serviceLocator->get('bar_tab_class_with_defaultmethod'),
             'foo' => $serviceLocator->get('foo'),
         ];
-        $this->assertSame(['bar_tab_class_with_defaultmethod' => $container->get(BarTagClass::class), 'foo' => $container->get(FooTagClass::class)], $same);
+        $this->assertSame(['bar_tab_class_with_defaultmethod' => $container->get('bar_tag'), 'foo' => $container->get('foo_tag')], $same);
     }
 
-    public function testTaggedServiceLocatorWithFqcnFallback()
+    public function testTaggedServiceLocatorWithFallback()
     {
         $container = new ContainerBuilder();
-        $container->register(BarTagClass::class)
+        $container->register('bar_tag', BarTagClass::class)
             ->setPublic(true)
             ->addTag('foo_bar')
         ;
-        $container->register(FooBarTaggedClass::class)
+        $container->register('foo_bar_tagged', FooBarTaggedClass::class)
             ->addArgument(new ServiceLocatorArgument(new TaggedIteratorArgument('foo_bar', null, null, true)))
             ->setPublic(true)
         ;
 
         $container->compile();
 
-        $s = $container->get(FooBarTaggedClass::class);
+        $s = $container->get('foo_bar_tagged');
 
         /** @var ServiceLocator $serviceLocator */
         $serviceLocator = $s->getParam();
         $this->assertTrue($s->getParam() instanceof ServiceLocator, sprintf('Wrong instance, should be an instance of ServiceLocator, %s given', \is_object($serviceLocator) ? \get_class($serviceLocator) : \gettype($serviceLocator)));
 
-        $same = [
+        $expected = [
-            BarTagClass::class => $serviceLocator->get(BarTagClass::class),
+            'bar_tag' => $container->get('bar_tag'),
         ];
-        $this->assertSame([BarTagClass::class => $container->get(BarTagClass::class)], $same);
+        $this->assertSame($expected, ['bar_tag' => $serviceLocator->get('bar_tag')]);
+    }
+
+    public function testTaggedServiceLocatorWithDefaultIndex()
+    {
+        $container = new ContainerBuilder();
+        $container->register('bar_tag', BarTagClass::class)
+            ->setPublic(true)
+            ->addTag('app.foo_bar', ['foo_bar' => 'baz'])
+        ;
+        $container->register('foo_bar_tagged', FooBarTaggedClass::class)
+            ->addArgument(new ServiceLocatorArgument(new TaggedIteratorArgument('app.foo_bar', null, null, true)))
+            ->setPublic(true)
+        ;
+
+        $container->compile();
+
+        $s = $container->get('foo_bar_tagged');
+
+        /** @var ServiceLocator $serviceLocator */
+        $serviceLocator = $s->getParam();
+        $this->assertTrue($s->getParam() instanceof ServiceLocator, sprintf('Wrong instance, should be an instance of ServiceLocator, %s given', \is_object($serviceLocator) ? \get_class($serviceLocator) : \gettype($serviceLocator)));
+
+        $expected = [
+            'baz' => $container->get('bar_tag'),
+        ];
+        $this->assertSame($expected, ['baz' => $serviceLocator->get('baz')]);
     }
 }