[DoctrineBridge] Abstract Doctrine Subscribers with tags #11160
Conversation
|
I would rather throw an exception instead of ignoring it silently, to be consistent with the way Symfony listeners work |
|
So it isn't a supported use case to tag an abstract service? |
|
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 |
|
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? |
|
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) |
|
And what message should be presented to the developer? "An abstract service cannot be tagged as a doctrine listener or subscriber."? |
|
This looks good, except that the message should contain the id of the service to make it easier to fix it. |
|
Updated to throw an exception |
|
|
||
| $this->process($container); | ||
|
|
||
| $this->assertEmpty($this->getServiceOrder($container, 'addEventSubscriber'), 'Subscriber Tagged abstract service was not processed'); |
There was a problem hiding this comment.
this will never be reached as you expect an exception during process()
|
👍 |
1 similar comment
|
👍 |
|
Thank you @merk. |
…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
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
findTaggedServiceIdsinstead?