feature #22420 [DI] Make tagged abstract services throw earlier (nicolas-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 https://github.com/symfony/symfony/pull/22388#issuecomment-293565243, 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
-------

388e4b3389 [DI] Make tagged abstract services throw earlier
cd06c1297b Revert "minor #22039 Skip abstract definitions in compiler passes (chalasr)"
This commit is contained in:
Fabien Potencier 2017-04-13 08:28:14 -07:00
commit 4f0daa740a
30 changed files with 117 additions and 90 deletions

View File

@ -55,8 +55,8 @@ class RegisterEventListenersAndSubscribersPass implements CompilerPassInterface
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 @@ class RegisterEventListenersAndSubscribersPass implements CompilerPassInterface
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 @@ class RegisterEventListenersAndSubscribersPass implements CompilerPassInterface
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),

View File

@ -18,6 +18,9 @@ use Symfony\Component\DependencyInjection\Definition;
class RegisterEventListenersAndSubscribersPassTest extends TestCase
{
/**
* @expectedException \InvalidArgumentException
*/
public function testExceptionOnAbstractTaggedSubscriber()
{
$container = $this->createBuilder();
@ -29,10 +32,12 @@ class RegisterEventListenersAndSubscribersPassTest extends TestCase
$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 @@ class RegisterEventListenersAndSubscribersPassTest extends TestCase
$container->setDefinition('a', $abstractDefinition);
$this->process($container);
$this->assertSame(array(), $container->getDefinition('doctrine.dbal.default_connection.event_manager')->getMethodCalls());
}
public function testProcessEventListenersWithPriorities()

View File

@ -32,7 +32,7 @@ class AddCacheClearerPass implements CompilerPassInterface
}
$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);
}

View File

@ -30,7 +30,7 @@ class AddExpressionLanguageProvidersPass implements CompilerPassInterface
// 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)));
}
}
@ -38,7 +38,7 @@ class AddExpressionLanguageProvidersPass implements CompilerPassInterface
// 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)));
}
}

View File

@ -33,7 +33,7 @@ class ProfilerPass implements CompilerPassInterface
$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;

View File

@ -32,7 +32,7 @@ class TemplatingPass implements CompilerPassInterface
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;
}

View File

@ -28,7 +28,7 @@ class TranslationDumperPass implements CompilerPassInterface
$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)));
}
}

View File

@ -29,7 +29,7 @@ class TranslationExtractorPass implements CompilerPassInterface
$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));
}

View File

@ -26,7 +26,7 @@ class TranslatorPass implements CompilerPassInterface
$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'])) {

View File

@ -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) {

View File

@ -62,6 +62,10 @@ class AddConsoleCommandPassTest extends TestCase
);
}
/**
* @expectedException \InvalidArgumentException
* @expectedExceptionMessage The service "my-command" tagged "console.command" must not be abstract.
*/
public function testProcessThrowAnExceptionIfTheServiceIsAbstract()
{
$container = new ContainerBuilder();
@ -73,8 +77,6 @@ class AddConsoleCommandPassTest extends TestCase
$container->setDefinition('my-command', $definition);
$container->compile();
$this->assertSame(array(), $container->getParameter('console.command.ids'));
}
/**

View File

@ -34,9 +34,6 @@ class AddConstraintValidatorsPassTest extends TestCase
->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 @@ class AddConstraintValidatorsPassTest extends TestCase
$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();

View File

@ -29,13 +29,8 @@ class RuntimeLoaderPass implements CompilerPassInterface
$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);
}

View File

@ -36,7 +36,7 @@ class TwigEnvironmentPass implements CompilerPassInterface
// 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));

View File

@ -32,7 +32,7 @@ class TwigLoaderPass implements CompilerPassInterface
$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;

View File

@ -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)) {

View File

@ -50,7 +50,11 @@ class AddConsoleCommandPassTest extends TestCase
);
}
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 @@ class AddConsoleCommandPassTest extends TestCase
$container->setDefinition('my-command', $definition);
$container->compile();
$this->assertSame(array(), $container->getParameter('console.command.ids'));
}
/**

View File

@ -40,7 +40,7 @@ trait PriorityTaggedServiceTrait
{
$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);
}

View File

@ -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 @@ use Symfony\Component\DependencyInjection\Exception\RuntimeException;
*/
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 @@ class ResolveTagsInheritancePass extends AbstractRecursivePass
}
$parentDef = $this->container->findDefinition($parent);
if ($parentDef->isAbstract()) {
$this->abstractInheritedParents[$parent] = $parent;
}
if ($parentDef instanceof ChildDefinition) {
$this->processValue($parentDef);

View File

@ -1200,16 +1200,20 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
* }
* }
*
* @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);
}
}

View File

@ -22,13 +22,13 @@ class ResolveTagsInheritancePassTest extends TestCase
{
$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());
}
}

View File

@ -60,11 +60,8 @@ class RegisterListenersPass implements CompilerPassInterface
$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 @@ class RegisterListenersPass implements CompilerPassInterface
$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());

View File

@ -87,6 +87,10 @@ class RegisterListenersPassTest extends TestCase
$registerListenersPass->process($builder);
}
/**
* @expectedException \InvalidArgumentException
* @expectedExceptionMessage The service "foo" tagged "kernel.event_listener" must not be abstract.
*/
public function testAbstractEventListener()
{
$container = new ContainerBuilder();
@ -95,11 +99,13 @@ class RegisterListenersPassTest extends TestCase
$registerListenersPass = new RegisterListenersPass();
$registerListenersPass->process($container);
$this->assertSame(array(), $container->getDefinition('event_dispatcher')->getMethodCalls());
}
public function testAbstractEventSubscriberIsSkipped()
/**
* @expectedException \InvalidArgumentException
* @expectedExceptionMessage The service "foo" tagged "kernel.event_subscriber" must not be abstract.
*/
public function testAbstractEventSubscriber()
{
$container = new ContainerBuilder();
$container->register('foo', 'stdClass')->setAbstract(true)->addTag('kernel.event_subscriber', array());
@ -107,8 +113,6 @@ class RegisterListenersPassTest extends TestCase
$registerListenersPass = new RegisterListenersPass();
$registerListenersPass->process($container);
$this->assertSame(array(), $container->getDefinition('event_dispatcher')->getMethodCalls());
}
public function testEventSubscriberResolvableClassName()

View File

@ -61,10 +61,9 @@ class FormPass implements CompilerPassInterface
$servicesMap = array();
// Builds an array with fully-qualified type class names as keys and service IDs as values
foreach ($container->findTaggedServiceIds($this->formTypeTag) as $serviceId => $tag) {
$serviceDefinition = $container->getDefinition($serviceId);
foreach ($container->findTaggedServiceIds($this->formTypeTag, true) as $serviceId => $tag) {
// Add form type service to the service locator
$serviceDefinition = $container->getDefinition($serviceId);
$servicesMap[$serviceDefinition->getClass()] = new Reference($serviceId);
}
@ -98,7 +97,7 @@ class FormPass implements CompilerPassInterface
private function processFormTypeGuessers(ContainerBuilder $container)
{
$guessers = array();
foreach ($container->findTaggedServiceIds($this->formTypeGuesserTag) as $serviceId => $tags) {
foreach ($container->findTaggedServiceIds($this->formTypeGuesserTag, true) as $serviceId => $tags) {
$guessers[] = new Reference($serviceId);
}

View File

@ -46,12 +46,8 @@ class FragmentRendererPass implements CompilerPassInterface
$definition = $container->getDefinition($this->handlerService);
$renderers = array();
foreach ($container->findTaggedServiceIds($this->rendererTag) as $id => $tags) {
foreach ($container->findTaggedServiceIds($this->rendererTag, true) as $id => $tags) {
$def = $container->getDefinition($id);
if ($def->isAbstract()) {
continue;
}
$class = $container->getParameterBag()->resolveValue($def->getClass());
if (!$r = $container->getReflectionClass($class)) {

View File

@ -48,12 +48,8 @@ class RegisterControllerArgumentLocatorsPass implements CompilerPassInterface
$parameterBag = $container->getParameterBag();
$controllers = array();
foreach ($container->findTaggedServiceIds($this->controllerTag) as $id => $tags) {
foreach ($container->findTaggedServiceIds($this->controllerTag, true) as $id => $tags) {
$def = $container->getDefinition($id);
if ($def->isAbstract()) {
continue;
}
$class = $def->getClass();
$autowire = $def->isAutowired();

View File

@ -39,7 +39,7 @@ class RoutingResolverPass implements CompilerPassInterface
$definition = $container->getDefinition($this->resolverServiceId);
foreach ($container->findTaggedServiceIds($this->loaderTag) as $id => $attributes) {
foreach ($container->findTaggedServiceIds($this->loaderTag, true) as $id => $attributes) {
$definition->addMethodCall('addLoader', array(new Reference($id)));
}
}

View File

@ -38,13 +38,9 @@ class AddConstraintValidatorsPass implements CompilerPassInterface
}
$validators = array();
foreach ($container->findTaggedServiceIds($this->constraintValidatorTag) as $id => $attributes) {
foreach ($container->findTaggedServiceIds($this->constraintValidatorTag, true) as $id => $attributes) {
$definition = $container->getDefinition($id);
if ($definition->isAbstract()) {
continue;
}
if (isset($attributes[0]['alias'])) {
$validators[$attributes[0]['alias']] = new Reference($id);
}

View File

@ -37,11 +37,7 @@ class AddValidatorInitializersPass implements CompilerPassInterface
}
$initializers = array();
foreach ($container->findTaggedServiceIds($this->initializerTag) as $id => $attributes) {
if ($container->getDefinition($id)->isAbstract()) {
continue;
}
foreach ($container->findTaggedServiceIds($this->initializerTag, true) as $id => $attributes) {
$initializers[] = new Reference($id);
}

View File

@ -31,9 +31,6 @@ class AddConstraintValidatorsPassTest extends TestCase
->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);
@ -46,6 +43,24 @@ class AddConstraintValidatorsPassTest extends TestCase
$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();