Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

[DoctrineBridge] Abstract Doctrine Subscribers with tags #11160

Merged
merged 1 commit into from

3 participants

@merk
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets this one
License MIT
Doc PR N/A

I've hit a problem with some doctrine listeners, built by decorating an abstract definition.

I want the abstract definition to hold the tag, however because the RegisterEventListenersAndSubscribersPass runs before abstract definitions are removed, they get added as method calls to the EventManager definition, which once the abstract service is removed, we end up with a method call that breaks the container.

I don't know if this is the best approach, it might be better not to return abstract services when calling findTaggedServiceIds instead?

@stof
Collaborator

I would rather throw an exception instead of ignoring it silently, to be consistent with the way Symfony listeners work

@merk

So it isn't a supported use case to tag an abstract service?

@stof
Collaborator

well, not for registering it as a Doctrine listener, as the abstract service cannot be referenced later (there are some tags for which it can make sense to apply them on abstract definitions, for instance monolog.logger, but not tags used to aggregate dependencies)

@merk

Are the reasons the same for the Doctrine and Symfony event dispatchers? The Doctrine one doesnt appear to lazily load its listeners (which means it can accept private services?)

I'm happy to update the PR for whatever solution so people can avoid the time it took me to work it out. This exposed a deficiency in the design of the FOSElasticaBundle listeners where we should only register one, not one per type.

Should it just throw an exception that abstract services cannot be tagged as doctrine listeners or subscribers?

@stof
Collaborator

the lazyness is the reason why symfony listeners must be public. But we are not talking about private services here but about abstract services. Abstract services don't exist at runtime anymore. they are just prototypes for child definitions.

so yes, it should be an exception (it currently also leads to an exception later because the service does not exist, so it would not change the behavior but simply give a much better error message)

@stof stof added DX Doctrine labels
@merk

And what message should be presented to the developer? "An abstract service cannot be tagged as a doctrine listener or subscriber."?

@stof
Collaborator

This looks good, except that the message should contain the id of the service to make it easier to fix it.

@merk

Updated to throw an exception

...Pass/RegisterEventListenersAndSubscribersPassTest.php
((4 lines not shown))
+ /**
+ * @expectedException InvalidArgumentException
+ */
+ public function testExceptionOnAbstractTaggedSubscriber()
+ {
+ $container = $this->createBuilder();
+
+ $abstractDefinition = new Definition('stdClass');
+ $abstractDefinition->setAbstract(true);
+ $abstractDefinition->addTag('doctrine.event_subscriber');
+
+ $container->setDefinition('a', $abstractDefinition);
+
+ $this->process($container);
+
+ $this->assertEmpty($this->getServiceOrder($container, 'addEventSubscriber'), 'Subscriber Tagged abstract service was not processed');
@stof Collaborator
stof added a note

this will never be reached as you expect an exception during process()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...Pass/RegisterEventListenersAndSubscribersPassTest.php
((22 lines not shown))
+ /**
+ * @expectedException InvalidArgumentException
+ */
+ public function testExceptionOnAbstractTaggedListener()
+ {
+ $container = $this->createBuilder();
+
+ $abstractDefinition = new Definition('stdClass');
+ $abstractDefinition->setAbstract(true);
+ $abstractDefinition->addTag('doctrine.event_listener', array('event' => 'test'));
+
+ $container->setDefinition('a', $abstractDefinition);
+
+ $this->process($container);
+
+ $this->assertEmpty($this->getServiceOrder($container, 'addEventListener'), 'Listener Tagged abstract service was not processed');
@stof Collaborator
stof added a note

same here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@merk merk Disallow abstract definitions from doctrine event listener registration
cbcf513
@fabpot
Owner

:+1:

@stof
Collaborator

:+1:

@stof
Collaborator

Thank you @merk.

@stof stof merged commit cbcf513 into symfony:2.3
@stof stof referenced this pull request from a commit
@stof stof bug #11160 [DoctrineBridge] Abstract Doctrine Subscribers with tags (…
…merk)

This PR was merged into the 2.3 branch.

Discussion
----------

[DoctrineBridge] Abstract Doctrine Subscribers with tags

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | this one
| License       | MIT
| Doc PR        | N/A

I've hit a problem with some doctrine listeners, built by decorating an abstract definition.

I want the abstract definition to hold the tag, however because the RegisterEventListenersAndSubscribersPass runs before abstract definitions are removed, they get added as method calls to the EventManager definition, which once the abstract service is removed, we end up with a method call that breaks the container.

I don't know if this is the best approach, it might be better not to return abstract services when calling `findTaggedServiceIds` instead?

Commits
-------

cbcf513 Disallow abstract definitions from doctrine event listener registration
37f2c3d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 19, 2014
  1. @merk

    Disallow abstract definitions from doctrine event listener registration

    merk authored
This page is out of date. Refresh to see the latest.
View
8 ...ge/Doctrine/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPass.php
@@ -68,6 +68,10 @@ public function process(ContainerBuilder $container)
uasort($subscribers, $sortFunc);
foreach ($subscribers as $id => $instance) {
+ if ($container->getDefinition($id)->isAbstract()) {
+ throw new \InvalidArgumentException(sprintf('The abstract service "%s" cannot be tagged as a doctrine event subscriber.', $id));
+ }
+
$em->addMethodCall('addEventSubscriber', array(new Reference($id)));
}
}
@@ -78,6 +82,10 @@ public function process(ContainerBuilder $container)
uasort($listeners, $sortFunc);
foreach ($listeners as $id => $instance) {
+ if ($container->getDefinition($id)->isAbstract()) {
+ throw new \InvalidArgumentException(sprintf('The abstract service "%s" cannot be tagged as a doctrine event listener.', $id));
+ }
+
$em->addMethodCall('addEventListener', array(
array_unique($instance['event']),
isset($instance['lazy']) && $instance['lazy'] ? $id : new Reference($id),
View
33 ...e/Tests/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPassTest.php
@@ -13,6 +13,7 @@
use Symfony\Bridge\Doctrine\DependencyInjection\CompilerPass\RegisterEventListenersAndSubscribersPass;
use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Definition;
class RegisterEventListenersAndSubscribersPassTest extends \PHPUnit_Framework_TestCase
{
@@ -23,6 +24,38 @@ protected function setUp()
}
}
+ /**
+ * @expectedException InvalidArgumentException
+ */
+ public function testExceptionOnAbstractTaggedSubscriber()
+ {
+ $container = $this->createBuilder();
+
+ $abstractDefinition = new Definition('stdClass');
+ $abstractDefinition->setAbstract(true);
+ $abstractDefinition->addTag('doctrine.event_subscriber');
+
+ $container->setDefinition('a', $abstractDefinition);
+
+ $this->process($container);
+ }
+
+ /**
+ * @expectedException InvalidArgumentException
+ */
+ public function testExceptionOnAbstractTaggedListener()
+ {
+ $container = $this->createBuilder();
+
+ $abstractDefinition = new Definition('stdClass');
+ $abstractDefinition->setAbstract(true);
+ $abstractDefinition->addTag('doctrine.event_listener', array('event' => 'test'));
+
+ $container->setDefinition('a', $abstractDefinition);
+
+ $this->process($container);
+ }
+
public function testProcessEventListenersWithPriorities()
{
$container = $this->createBuilder();
Something went wrong with that request. Please try again.