feature #22420 [DI] Make tagged abstract services throw earlier (nico…

…las-grekas) This PR was squashed before being merged into the 3.3-dev branch (closes #22420). Discussion ---------- [DI] Make tagged abstract services throw earlier | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - As spotted by @stof in #22388 (comment), skipping abstract tagged services removes an opportunity to report config mistakes to users. Instead of skipping them, let's throw as done before (thus reverting #22039, ping @chalasr). I made `$container->findTaggedServiceIds()` accept a 2nd arg to make this more systematic. To keep the possibility to have abstract tagged services *for the purpose of tag inheritance*, `ResolveTagsInheritancePass` now resets their tags. Commits ------- 388e4b3 [DI] Make tagged abstract services throw earlier cd06c12 Revert "minor #22039 Skip abstract definitions in compiler passes (chalasr)"
symfony · Apr 13, 2017 · 4f0daa7 · 4f0daa7
2 parents 64b715b + 388e4b3
commit 4f0daa7
Show file tree

Hide file tree

Showing 30 changed files with 117 additions and 90 deletions.
diff --git a/...ge/Doctrine/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPass.php b/...ge/Doctrine/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPass.php
@@ -55,8 +55,8 @@ public function process(ContainerBuilder $container)
             return;
         }
 
-        $taggedSubscribers = $container->findTaggedServiceIds($this->tagPrefix.'.event_subscriber');
-        $taggedListeners = $container->findTaggedServiceIds($this->tagPrefix.'.event_listener');
+        $taggedSubscribers = $container->findTaggedServiceIds($this->tagPrefix.'.event_subscriber', true);
+        $taggedListeners = $container->findTaggedServiceIds($this->tagPrefix.'.event_listener', true);
 
         if (empty($taggedSubscribers) && empty($taggedListeners)) {
             return;
@@ -78,10 +78,6 @@ public function process(ContainerBuilder $container)
 
                 uasort($subscribers, $sortFunc);
                 foreach ($subscribers as $id => $instance) {
-                    if ($container->getDefinition($id)->isAbstract()) {
-                        continue;
-                    }
-
                     $em->addMethodCall('addEventSubscriber', array(new Reference($id)));
                 }
             }
@@ -94,10 +90,6 @@ public function process(ContainerBuilder $container)
 
                 uasort($listeners, $sortFunc);
                 foreach ($listeners as $id => $instance) {
-                    if ($container->getDefinition($id)->isAbstract()) {
-                        continue;
-                    }
-
                     $em->addMethodCall('addEventListener', array(
                         array_unique($instance['event']),
                         isset($instance['lazy']) && $instance['lazy'] ? $id : new Reference($id),

diff --git a/...e/Tests/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPassTest.php b/...e/Tests/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPassTest.php
@@ -18,6 +18,9 @@
 
 class RegisterEventListenersAndSubscribersPassTest extends TestCase
 {
+    /**
+     * @expectedException \InvalidArgumentException
+     */
     public function testExceptionOnAbstractTaggedSubscriber()
     {
         $container = $this->createBuilder();
@@ -29,10 +32,12 @@ public function testExceptionOnAbstractTaggedSubscriber()
         $container->setDefinition('a', $abstractDefinition);
 
         $this->process($container);
-        $this->assertSame(array(), $container->getDefinition('doctrine.dbal.default_connection.event_manager')->getMethodCalls());
     }
 
-    public function testAbstractTaggedListenerIsSkipped()
+    /**
+     * @expectedException \InvalidArgumentException
+     */
+    public function testExceptionOnAbstractTaggedListener()
     {
         $container = $this->createBuilder();
 
@@ -43,7 +48,6 @@ public function testAbstractTaggedListenerIsSkipped()
         $container->setDefinition('a', $abstractDefinition);
 
         $this->process($container);
-        $this->assertSame(array(), $container->getDefinition('doctrine.dbal.default_connection.event_manager')->getMethodCalls());
     }
 
     public function testProcessEventListenersWithPriorities()

diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/AddCacheClearerPass.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/AddCacheClearerPass.php
@@ -32,7 +32,7 @@ public function process(ContainerBuilder $container)
         }
 
         $clearers = array();
-        foreach ($container->findTaggedServiceIds('kernel.cache_clearer') as $id => $attributes) {
+        foreach ($container->findTaggedServiceIds('kernel.cache_clearer', true) as $id => $attributes) {
             $clearers[] = new Reference($id);
         }
 

diff --git a/...undle/FrameworkBundle/DependencyInjection/Compiler/AddExpressionLanguageProvidersPass.php b/...undle/FrameworkBundle/DependencyInjection/Compiler/AddExpressionLanguageProvidersPass.php
@@ -30,15 +30,15 @@ public function process(ContainerBuilder $container)
         // routing
         if ($container->has('router')) {
             $definition = $container->findDefinition('router');
-            foreach ($container->findTaggedServiceIds('routing.expression_language_provider') as $id => $attributes) {
+            foreach ($container->findTaggedServiceIds('routing.expression_language_provider', true) as $id => $attributes) {
                 $definition->addMethodCall('addExpressionLanguageProvider', array(new Reference($id)));
             }
         }
 
         // security
         if ($container->has('security.access.expression_voter')) {
             $definition = $container->findDefinition('security.access.expression_voter');
-            foreach ($container->findTaggedServiceIds('security.expression_language_provider') as $id => $attributes) {
+            foreach ($container->findTaggedServiceIds('security.expression_language_provider', true) as $id => $attributes) {
                 $definition->addMethodCall('addExpressionLanguageProvider', array(new Reference($id)));
             }
         }

diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/ProfilerPass.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/ProfilerPass.php
@@ -33,7 +33,7 @@ public function process(ContainerBuilder $container)
 
         $collectors = new \SplPriorityQueue();
         $order = PHP_INT_MAX;
-        foreach ($container->findTaggedServiceIds('data_collector') as $id => $attributes) {
+        foreach ($container->findTaggedServiceIds('data_collector', true) as $id => $attributes) {
             $priority = isset($attributes[0]['priority']) ? $attributes[0]['priority'] : 0;
             $template = null;
 

diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TemplatingPass.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TemplatingPass.php
@@ -32,7 +32,7 @@ public function process(ContainerBuilder $container)
 
         if ($container->hasDefinition('templating.engine.php')) {
             $helpers = array();
-            foreach ($container->findTaggedServiceIds('templating.helper') as $id => $attributes) {
+            foreach ($container->findTaggedServiceIds('templating.helper', true) as $id => $attributes) {
                 if (isset($attributes[0]['alias'])) {
                     $helpers[$attributes[0]['alias']] = $id;
                 }

diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TranslationDumperPass.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TranslationDumperPass.php
@@ -28,7 +28,7 @@ public function process(ContainerBuilder $container)
 
         $definition = $container->getDefinition('translation.writer');
 
-        foreach ($container->findTaggedServiceIds('translation.dumper') as $id => $attributes) {
+        foreach ($container->findTaggedServiceIds('translation.dumper', true) as $id => $attributes) {
             $definition->addMethodCall('addDumper', array($attributes[0]['alias'], new Reference($id)));
         }
     }

diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TranslationExtractorPass.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TranslationExtractorPass.php
@@ -29,7 +29,7 @@ public function process(ContainerBuilder $container)
 
         $definition = $container->getDefinition('translation.extractor');
 
-        foreach ($container->findTaggedServiceIds('translation.extractor') as $id => $attributes) {
+        foreach ($container->findTaggedServiceIds('translation.extractor', true) as $id => $attributes) {
             if (!isset($attributes[0]['alias'])) {
                 throw new RuntimeException(sprintf('The alias for the tag "translation.extractor" of service "%s" must be set.', $id));
             }

diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TranslatorPass.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TranslatorPass.php
@@ -26,7 +26,7 @@ public function process(ContainerBuilder $container)
 
         $loaders = array();
         $loaderRefs = array();
-        foreach ($container->findTaggedServiceIds('translation.loader') as $id => $attributes) {
+        foreach ($container->findTaggedServiceIds('translation.loader', true) as $id => $attributes) {
             $loaderRefs[$id] = new Reference($id);
             $loaders[$id][] = $attributes[0]['alias'];
             if (isset($attributes[0]['legacy-alias'])) {

diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/ValidateWorkflowsPass.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/ValidateWorkflowsPass.php
@@ -25,7 +25,7 @@ class ValidateWorkflowsPass implements CompilerPassInterface
 {
     public function process(ContainerBuilder $container)
     {
-        $taggedServices = $container->findTaggedServiceIds('workflow.definition');
+        $taggedServices = $container->findTaggedServiceIds('workflow.definition', true);
         foreach ($taggedServices as $id => $tags) {
             $definition = $container->get($id);
             foreach ($tags as $tag) {

diff --git a/...y/Bundle/FrameworkBundle/Tests/DependencyInjection/Compiler/AddConsoleCommandPassTest.php b/...y/Bundle/FrameworkBundle/Tests/DependencyInjection/Compiler/AddConsoleCommandPassTest.php
@@ -62,6 +62,10 @@ public function visibilityProvider()
         );
     }
 
+    /**
+     * @expectedException \InvalidArgumentException
+     * @expectedExceptionMessage The service "my-command" tagged "console.command" must not be abstract.
+     */
     public function testProcessThrowAnExceptionIfTheServiceIsAbstract()
     {
         $container = new ContainerBuilder();
@@ -73,8 +77,6 @@ public function testProcessThrowAnExceptionIfTheServiceIsAbstract()
         $container->setDefinition('my-command', $definition);
 
         $container->compile();
-
-        $this->assertSame(array(), $container->getParameter('console.command.ids'));
     }
 
     /**

diff --git a/...le/FrameworkBundle/Tests/DependencyInjection/Compiler/AddConstraintValidatorsPassTest.php b/...le/FrameworkBundle/Tests/DependencyInjection/Compiler/AddConstraintValidatorsPassTest.php
@@ -34,9 +34,6 @@ public function testThatConstraintValidatorServicesAreProcessed()
             ->addTag('validator.constraint_validator', array('alias' => 'my_constraint_validator_alias1'));
         $container->register('my_constraint_validator_service2', Validator2::class)
             ->addTag('validator.constraint_validator');
-        $container->register('my_abstract_constraint_validator')
-            ->setAbstract(true)
-            ->addTag('validator.constraint_validator');
 
         $addConstraintValidatorsPass = new AddConstraintValidatorsPass();
         $addConstraintValidatorsPass->process($container);
@@ -49,6 +46,24 @@ public function testThatConstraintValidatorServicesAreProcessed()
         $this->assertEquals($expected, $container->getDefinition((string) $validatorFactory->getArgument(0)));
     }
 
+    /**
+     * @expectedException \InvalidArgumentException
+     * @expectedExceptionMessage The service "my_abstract_constraint_validator" tagged "validator.constraint_validator" must not be abstract.
+     */
+    public function testAbstractConstraintValidator()
+    {
+        $container = new ContainerBuilder();
+        $validatorFactory = $container->register('validator.validator_factory')
+            ->addArgument(array());
+
+        $container->register('my_abstract_constraint_validator')
+            ->setAbstract(true)
+            ->addTag('validator.constraint_validator');
+
+        $addConstraintValidatorsPass = new AddConstraintValidatorsPass();
+        $addConstraintValidatorsPass->process($container);
+    }
+
     public function testThatCompilerPassIsIgnoredIfThereIsNoConstraintValidatorFactoryDefinition()
     {
         $definition = $this->getMockBuilder('Symfony\Component\DependencyInjection\Definition')->getMock();

diff --git a/src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/RuntimeLoaderPass.php b/src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/RuntimeLoaderPass.php
@@ -29,13 +29,8 @@ public function process(ContainerBuilder $container)
 
         $definition = $container->getDefinition('twig.runtime_loader');
         $mapping = array();
-        foreach ($container->findTaggedServiceIds('twig.runtime') as $id => $attributes) {
+        foreach ($container->findTaggedServiceIds('twig.runtime', true) as $id => $attributes) {
             $def = $container->getDefinition($id);
-
-            if ($def->isAbstract()) {
-                continue;
-            }
-
             $mapping[$def->getClass()] = new Reference($id);
         }
 

diff --git a/src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/TwigEnvironmentPass.php b/src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/TwigEnvironmentPass.php
@@ -36,7 +36,7 @@ public function process(ContainerBuilder $container)
         // be registered.
         $calls = $definition->getMethodCalls();
         $definition->setMethodCalls(array());
-        foreach ($container->findTaggedServiceIds('twig.extension') as $id => $attributes) {
+        foreach ($container->findTaggedServiceIds('twig.extension', true) as $id => $attributes) {
             $definition->addMethodCall('addExtension', array(new Reference($id)));
         }
         $definition->setMethodCalls(array_merge($definition->getMethodCalls(), $calls));

diff --git a/src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/TwigLoaderPass.php b/src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/TwigLoaderPass.php
@@ -32,7 +32,7 @@ public function process(ContainerBuilder $container)
         $prioritizedLoaders = array();
         $found = 0;
 
-        foreach ($container->findTaggedServiceIds('twig.loader') as $id => $attributes) {
+        foreach ($container->findTaggedServiceIds('twig.loader', true) as $id => $attributes) {
             $priority = isset($attributes[0]['priority']) ? $attributes[0]['priority'] : 0;
             $prioritizedLoaders[$priority][] = $id;
             ++$found;

diff --git a/src/Symfony/Component/Console/DependencyInjection/AddConsoleCommandPass.php b/src/Symfony/Component/Console/DependencyInjection/AddConsoleCommandPass.php
@@ -25,16 +25,11 @@ class AddConsoleCommandPass implements CompilerPassInterface
 {
     public function process(ContainerBuilder $container)
     {
-        $commandServices = $container->findTaggedServiceIds('console.command');
+        $commandServices = $container->findTaggedServiceIds('console.command', true);
         $serviceIds = array();
 
         foreach ($commandServices as $id => $tags) {
             $definition = $container->getDefinition($id);
-
-            if ($definition->isAbstract()) {
-                continue;
-            }
-
             $class = $container->getParameterBag()->resolveValue($definition->getClass());
 
             if (!$r = $container->getReflectionClass($class)) {

diff --git a/src/Symfony/Component/Console/Tests/DependencyInjection/AddConsoleCommandPassTest.php b/src/Symfony/Component/Console/Tests/DependencyInjection/AddConsoleCommandPassTest.php
@@ -50,7 +50,11 @@ public function visibilityProvider()
         );
     }
 
-    public function testProcessSkipAbstractDefinitions()
+    /**
+     * @expectedException \InvalidArgumentException
+     * @expectedExceptionMessage The service "my-command" tagged "console.command" must not be abstract.
+     */
+    public function testProcessThrowAnExceptionIfTheServiceIsAbstract()
     {
         $container = new ContainerBuilder();
         $container->setResourceTracking(false);
@@ -62,8 +66,6 @@ public function testProcessSkipAbstractDefinitions()
         $container->setDefinition('my-command', $definition);
 
         $container->compile();
-
-        $this->assertSame(array(), $container->getParameter('console.command.ids'));
     }
 
     /**

diff --git a/src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php b/src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php
@@ -40,7 +40,7 @@ private function findAndSortTaggedServices($tagName, ContainerBuilder $container
     {
         $services = array();
 
-        foreach ($container->findTaggedServiceIds($tagName) as $serviceId => $attributes) {
+        foreach ($container->findTaggedServiceIds($tagName, true) as $serviceId => $attributes) {
             $priority = isset($attributes[0]['priority']) ? $attributes[0]['priority'] : 0;
             $services[$priority][] = new Reference($serviceId);
         }

diff --git a/src/Symfony/Component/DependencyInjection/Compiler/ResolveTagsInheritancePass.php b/src/Symfony/Component/DependencyInjection/Compiler/ResolveTagsInheritancePass.php
@@ -12,6 +12,7 @@
 namespace Symfony\Component\DependencyInjection\Compiler;
 
 use Symfony\Component\DependencyInjection\ChildDefinition;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Exception\RuntimeException;
 
 /**
@@ -21,6 +22,24 @@
  */
 class ResolveTagsInheritancePass extends AbstractRecursivePass
 {
+    private $abstractInheritedParents = array();
+
+    /**
+     * {@inheritdoc}
+     */
+    public function process(ContainerBuilder $container)
+    {
+        try {
+            parent::process($container);
+
+            foreach ($this->abstractInheritedParents as $id) {
+                $container->findDefinition($id)->setTags(array());
+            }
+        } finally {
+            $this->abstractInheritedParents = array();
+        }
+    }
+
     /**
      * {@inheritdoc}
      */
@@ -36,6 +55,9 @@ protected function processValue($value, $isRoot = false)
         }
 
         $parentDef = $this->container->findDefinition($parent);
+        if ($parentDef->isAbstract()) {
+            $this->abstractInheritedParents[$parent] = $parent;
+        }
 
         if ($parentDef instanceof ChildDefinition) {
             $this->processValue($parentDef);

diff --git a/src/Symfony/Component/DependencyInjection/ContainerBuilder.php b/src/Symfony/Component/DependencyInjection/ContainerBuilder.php
@@ -1200,16 +1200,20 @@ public function resolveServices($value)
      *     }
      * }
      *
-     * @param string $name The tag name
+     * @param string $name
+     * @param bool   $throwOnAbstract
      *
      * @return array An array of tags with the tagged service as key, holding a list of attribute arrays
      */
-    public function findTaggedServiceIds($name)
+    public function findTaggedServiceIds($name, $throwOnAbstract = false)
     {
         $this->usedTags[] = $name;
         $tags = array();
         foreach ($this->getDefinitions() as $id => $definition) {
             if ($definition->hasTag($name)) {
+                if ($throwOnAbstract && $definition->isAbstract()) {
+                    throw new InvalidArgumentException(sprintf('The service "%s" tagged "%s" must not be abstract.', $id, $name));
+                }
                 $tags[$id] = $definition->getTag($name);
             }
         }

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Compiler/ResolveTagsInheritancePassTest.php b/src/Symfony/Component/DependencyInjection/Tests/Compiler/ResolveTagsInheritancePassTest.php
@@ -22,13 +22,13 @@ public function testProcess()
     {
         $container = new ContainerBuilder();
         $container->register('grandpa', self::class)->addTag('g');
-        $container->setDefinition('parent', new ChildDefinition('grandpa'))->addTag('p')->setInheritTags(true);
+        $container->setDefinition('parent', new ChildDefinition('grandpa'))->addTag('p')->setInheritTags(true)->setAbstract(true);
         $container->setDefinition('child', new ChildDefinition('parent'))->setInheritTags(true);
 
         (new ResolveTagsInheritancePass())->process($container);
 
         $expected = array('p' => array(array()), 'g' => array(array()));
-        $this->assertSame($expected, $container->getDefinition('parent')->getTags());
         $this->assertSame($expected, $container->getDefinition('child')->getTags());
+        $this->assertSame(array(), $container->getDefinition('parent')->getTags());
     }
 }
diff --git a/src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php b/src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php
@@ -60,11 +60,8 @@ public function process(ContainerBuilder $container)
 
         $definition = $container->findDefinition($this->dispatcherService);
 
-        foreach ($container->findTaggedServiceIds($this->listenerTag) as $id => $events) {
+        foreach ($container->findTaggedServiceIds($this->listenerTag, true) as $id => $events) {
             $def = $container->getDefinition($id);
-            if ($def->isAbstract()) {
-                continue;
-            }
 
             foreach ($events as $event) {
                 $priority = isset($event['priority']) ? $event['priority'] : 0;
@@ -87,11 +84,8 @@ public function process(ContainerBuilder $container)
 
         $extractingDispatcher = new ExtractingEventDispatcher();
 
-        foreach ($container->findTaggedServiceIds($this->subscriberTag) as $id => $attributes) {
+        foreach ($container->findTaggedServiceIds($this->subscriberTag, true) as $id => $attributes) {
             $def = $container->getDefinition($id);
-            if ($def->isAbstract()) {
-                continue;
-            }
 
             // We must assume that the class value has been correctly filled, even if the service is created by a factory
             $class = $container->getParameterBag()->resolveValue($def->getClass());