bug #39203 [DI] Fix not working if only "default_index_method" used (malteschlueter)

This PR was merged into the 4.4 branch. Discussion ---------- [DI] Fix not working if only "default_index_method" used | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #35349 | License | MIT The default index method wasn't used if the "index_by" attribute is missing. The documentation is showing an example, see https://symfony.com/doc/current/service_container/tags.html#tagged-services-with-index. This problem also appears in symfony 5. I created two example projects, the first in the current behaviour and the second with my bugfix branch. Current 4.4: https://github.com/malteschlueter/symfony-reproducers/blob/bugfix/dependency-injection-default-index-method-not-working--not-fixed/tests/HandlerCollectionTest.php This bugfix branch: https://github.com/malteschlueter/symfony-reproducers/blob/bugfix/dependency-injection-default-index-method-not-working--with-fix/tests/HandlerCollectionTest.php Commits ------- eb25d5c275 [DI] The default index method wasn't used if the "index_by" attribute is missing
2020-12-10 08:05:48 +01:00 · 2020-12-10 08:05:48 +01:00 · 2dd4561d3f
parent 52a0233905 eb25d5c275
commit 2dd4561d3f
4 changed files with 90 additions and 10 deletions
--- a/src/Symfony/Component/DependencyInjection/Argument/TaggedIteratorArgument.php
+++ b/src/Symfony/Component/DependencyInjection/Argument/TaggedIteratorArgument.php
@ -41,9 +41,9 @@ class TaggedIteratorArgument extends IteratorArgument

        $this->tag = $tag;
        $this->indexAttribute = $indexAttribute;
-        $this->defaultIndexMethod = $defaultIndexMethod ?: ('getDefault'.str_replace(' ', '', ucwords(preg_replace('/[^a-zA-Z0-9\x7f-\xff]++/', ' ', $indexAttribute ?? ''))).'Name');
+        $this->defaultIndexMethod = $defaultIndexMethod ?: ($indexAttribute ? 'getDefault'.str_replace(' ', '', ucwords(preg_replace('/[^a-zA-Z0-9\x7f-\xff]++/', ' ', $indexAttribute))).'Name' : null);
        $this->needsIndexes = $needsIndexes;
-        $this->defaultPriorityMethod = $defaultPriorityMethod ?: ('getDefault'.str_replace(' ', '', ucwords(preg_replace('/[^a-zA-Z0-9\x7f-\xff]++/', ' ', $indexAttribute ?? ''))).'Priority');
+        $this->defaultPriorityMethod = $defaultPriorityMethod ?: ($indexAttribute ? 'getDefault'.str_replace(' ', '', ucwords(preg_replace('/[^a-zA-Z0-9\x7f-\xff]++/', ' ', $indexAttribute))).'Priority' : null);
    }

    public function getTag()
--- a/src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php
+++ b/src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php
@ -46,7 +46,7 @@ trait PriorityTaggedServiceTrait
            $indexAttribute = $tagName->getIndexAttribute();
            $defaultIndexMethod = $tagName->getDefaultIndexMethod();
            $needsIndexes = $tagName->needsIndexes();
-            $defaultPriorityMethod = $tagName->getDefaultPriorityMethod();
+            $defaultPriorityMethod = $tagName->getDefaultPriorityMethod() ?? 'getDefaultPriority';
            $tagName = $tagName->getTag();
        }

@ -69,15 +69,15 @@ trait PriorityTaggedServiceTrait
                }
                $priority = $priority ?? $defaultPriority ?? $defaultPriority = 0;

-                if (null === $indexAttribute && !$needsIndexes) {
+                if (null === $indexAttribute && !$defaultIndexMethod && !$needsIndexes) {
                    $services[] = [$priority, ++$i, null, $serviceId, null];
                    continue 2;
                }

                if (null !== $indexAttribute && isset($attribute[$indexAttribute])) {
                    $index = $attribute[$indexAttribute];
-                } elseif (null === $defaultIndex && $defaultIndexMethod && $class) {
-                    $defaultIndex = PriorityTaggedServiceUtil::getDefaultIndex($container, $serviceId, $class, $defaultIndexMethod, $tagName, $indexAttribute);
+                } elseif (null === $defaultIndex && $defaultPriorityMethod && $class) {
+                    $defaultIndex = PriorityTaggedServiceUtil::getDefaultIndex($container, $serviceId, $class, $defaultIndexMethod ?? 'getDefaultName', $tagName, $indexAttribute);
                }
                $index = $index ?? $defaultIndex ?? $defaultIndex = $serviceId;

@ -116,24 +116,31 @@ class PriorityTaggedServiceUtil
    /**
     * Gets the index defined by the default index method.
     */
-    public static function getDefaultIndex(ContainerBuilder $container, string $serviceId, string $class, string $defaultIndexMethod, string $tagName, string $indexAttribute): ?string
+    public static function getDefaultIndex(ContainerBuilder $container, string $serviceId, string $class, string $defaultIndexMethod, string $tagName, ?string $indexAttribute): ?string
    {
        if (!($r = $container->getReflectionClass($class)) || !$r->hasMethod($defaultIndexMethod)) {
            return null;
        }

+        if (null !== $indexAttribute) {
+            $service = $class !== $serviceId ? sprintf('service "%s"', $serviceId) : 'on the corresponding service';
+            $message = [sprintf('Either method "%s::%s()" should ', $class, $defaultIndexMethod), sprintf(' or tag "%s" on %s is missing attribute "%s".', $tagName, $service, $indexAttribute)];
+        } else {
+            $message = [sprintf('Method "%s::%s()" should ', $class, $defaultIndexMethod), '.'];
+        }
+
        if (!($rm = $r->getMethod($defaultIndexMethod))->isStatic()) {
-            throw new InvalidArgumentException(sprintf('Either method "%s::%s()" should be static or tag "%s" on service "%s" is missing attribute "%s".', $class, $defaultIndexMethod, $tagName, $serviceId, $indexAttribute));
+            throw new InvalidArgumentException(implode('be static', $message));
        }

        if (!$rm->isPublic()) {
-            throw new InvalidArgumentException(sprintf('Either method "%s::%s()" should be public or tag "%s" on service "%s" is missing attribute "%s".', $class, $defaultIndexMethod, $tagName, $serviceId, $indexAttribute));
+            throw new InvalidArgumentException(implode('be public', $message));
        }

        $defaultIndex = $rm->invoke(null);

        if (!\is_string($defaultIndex)) {
-            throw new InvalidArgumentException(sprintf('Either method "%s::%s()" should return a string (got "%s") or tag "%s" on service "%s" is missing attribute "%s".', $class, $defaultIndexMethod, \gettype($defaultIndex), $tagName, $serviceId, $indexAttribute));
+            throw new InvalidArgumentException(implode(sprintf('return a string (got "%s")', \gettype($defaultIndex)), $message));
        }

        return $defaultIndex;
--- a/src/Symfony/Component/DependencyInjection/Tests/Compiler/PriorityTaggedServiceTraitTest.php
+++ b/src/Symfony/Component/DependencyInjection/Tests/Compiler/PriorityTaggedServiceTraitTest.php
@ -15,8 +15,11 @@ use PHPUnit\Framework\TestCase;
 use Symfony\Component\DependencyInjection\Argument\TaggedIteratorArgument;
 use Symfony\Component\DependencyInjection\Compiler\PriorityTaggedServiceTrait;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
 use Symfony\Component\DependencyInjection\Reference;
 use Symfony\Component\DependencyInjection\Tests\Fixtures\BarTagClass;
+use Symfony\Component\DependencyInjection\Tests\Fixtures\FooTagClass;
+use Symfony\Component\DependencyInjection\Tests\Fixtures\FooTaggedForInvalidDefaultMethodClass;
 use Symfony\Component\DependencyInjection\TypedReference;

 class PriorityTaggedServiceTraitTest extends TestCase
@ -132,6 +135,55 @@ class PriorityTaggedServiceTraitTest extends TestCase
        $this->assertSame(array_keys($expected), array_keys($services));
        $this->assertEquals($expected, $priorityTaggedServiceTraitImplementation->test($tag, $container));
    }
+
+    public function testTheIndexedTagsByDefaultIndexMethod()
+    {
+        $container = new ContainerBuilder();
+        $container->register('service1', FooTagClass::class)->addTag('my_custom_tag');
+
+        $definition = $container->register('service2', BarTagClass::class);
+        $definition->addTag('my_custom_tag', ['priority' => 100]);
+        $definition->addTag('my_custom_tag', []);
+
+        $priorityTaggedServiceTraitImplementation = new PriorityTaggedServiceTraitImplementation();
+
+        $tag = new TaggedIteratorArgument('my_custom_tag', 'foo', 'getFooBar');
+        $expected = [
+            'bar_tab_class_with_defaultmethod' => new TypedReference('service2', BarTagClass::class),
+            'service1' => new TypedReference('service1', FooTagClass::class),
+        ];
+        $services = $priorityTaggedServiceTraitImplementation->test($tag, $container);
+        $this->assertSame(array_keys($expected), array_keys($services));
+        $this->assertEquals($expected, $priorityTaggedServiceTraitImplementation->test($tag, $container));
+    }
+
+    /**
+     * @dataProvider provideInvalidDefaultMethods
+     */
+    public function testTheIndexedTagsByDefaultIndexMethodFailure(string $defaultIndexMethod, ?string $indexAttribute, string $expectedExceptionMessage)
+    {
+        $this->expectException(InvalidArgumentException::class);
+        $this->expectExceptionMessage($expectedExceptionMessage);
+
+        $container = new ContainerBuilder();
+
+        $container->register('service1', FooTaggedForInvalidDefaultMethodClass::class)->addTag('my_custom_tag');
+
+        $priorityTaggedServiceTraitImplementation = new PriorityTaggedServiceTraitImplementation();
+
+        $tag = new TaggedIteratorArgument('my_custom_tag', $indexAttribute, $defaultIndexMethod);
+        $priorityTaggedServiceTraitImplementation->test($tag, $container);
+    }
+
+    public function provideInvalidDefaultMethods(): iterable
+    {
+        yield ['getMethodShouldBeStatic', null, sprintf('Method "%s::getMethodShouldBeStatic()" should be static.', FooTaggedForInvalidDefaultMethodClass::class)];
+        yield ['getMethodShouldBeStatic', 'foo', sprintf('Either method "%s::getMethodShouldBeStatic()" should be static or tag "my_custom_tag" on service "service1" is missing attribute "foo".', FooTaggedForInvalidDefaultMethodClass::class)];
+        yield ['getMethodShouldBePublicInsteadProtected', null, sprintf('Method "%s::getMethodShouldBePublicInsteadProtected()" should be public.', FooTaggedForInvalidDefaultMethodClass::class)];
+        yield ['getMethodShouldBePublicInsteadProtected', 'foo', sprintf('Either method "%s::getMethodShouldBePublicInsteadProtected()" should be public or tag "my_custom_tag" on service "service1" is missing attribute "foo".', FooTaggedForInvalidDefaultMethodClass::class)];
+        yield ['getMethodShouldBePublicInsteadPrivate', null, sprintf('Method "%s::getMethodShouldBePublicInsteadPrivate()" should be public.', FooTaggedForInvalidDefaultMethodClass::class)];
+        yield ['getMethodShouldBePublicInsteadPrivate', 'foo', sprintf('Either method "%s::getMethodShouldBePublicInsteadPrivate()" should be public or tag "my_custom_tag" on service "service1" is missing attribute "foo".', FooTaggedForInvalidDefaultMethodClass::class)];
+    }
 }

 class PriorityTaggedServiceTraitImplementation
--- a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/FooTaggedForInvalidDefaultMethodClass.php
+++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/FooTaggedForInvalidDefaultMethodClass.php
@ -0,0 +1,21 @@
+<?php
+
+namespace Symfony\Component\DependencyInjection\Tests\Fixtures;
+
+class FooTaggedForInvalidDefaultMethodClass
+{
+    public function getMethodShouldBeStatic()
+    {
+        return 'anonymous_foo_class_with_default_method';
+    }
+
+    protected static function getMethodShouldBePublicInsteadProtected()
+    {
+        return 'anonymous_foo_class_with_default_method';
+    }
+
+    private static function getMethodShouldBePublicInsteadPrivate()
+    {
+        return 'anonymous_foo_class_with_default_method';
+    }
+}