[EventDispatcher] Add an interface for the ContainerAwareEventDispatcher #11870
Conversation
| * @param Stopwatch $stopwatch A Stopwatch instance | ||
| * @param LoggerInterface $logger A LoggerInterface instance | ||
| */ | ||
| public function __construct(ContainerAwareEventDispatcher $dispatcher, Stopwatch $stopwatch, LoggerInterface $logger = null) |
There was a problem hiding this comment.
For what override constructor here?
There was a problem hiding this comment.
To change the type hinting, so we are sure to have a ContainerAwareEventDispatcherInterface.
Nicofuma
force-pushed
the
ev_containerawareinterface
branch
from
efb5fe7 to
c808dfb
Compare
| */ | ||
| public function __construct(ContainerAwareEventDispatcherInterface $dispatcher, Stopwatch $stopwatch, LoggerInterface $logger = null) | ||
| { | ||
| parent::__construct($dispatcher, $stopwatch, $logger); |
There was a problem hiding this comment.
Is it really necessary to implement this constructor?
There was a problem hiding this comment.
In that case, a StopwatchInterface would solve BC causes in the future ;)
didn't see the comment for some reason
There was a problem hiding this comment.
But a such interface does not exist and I'm not sure that it's in the scope of this PR
There was a problem hiding this comment.
Then I stand by my question, is it really necessary to implement then?
There was a problem hiding this comment.
The goal here is not to prevent BC but to prevent the use of this class with an instance of EventDispatcherInterface instead of an instance of ContainerAwareEventDispatcherInterface (or we could throw a Fatal error late and "randomly" at the runtime)
Why would you want a class to depend on the |
|
Each time a service is registered as a listener/subscriber but without using a compiler pass. |
|
(And this case can happen often because the RegisterListenersPass was added only in 2.5) |
|
@Nicofuma the RegisterListenersPass was already there in previous version, just in a different component. |
|
@stof Thanks, I didn't find it in the HttpKernel component... |
Could you tell us what would be an use case for this? |
|
I sent the PR to fix this mistake in phpBB so... |
|
By adding listeners on run time, it makes it unreliable when checking registered listeners using |
|
and when adding listeners are runtime, registering services lazily is porbably less important |
| * | ||
| * @author Tristan Darricau <tristan@darricau.eu> | ||
| * | ||
| * @api |
Nicofuma
force-pushed
the
ev_containerawareinterface
branch
from
c808dfb to
d7718dc
Compare
| * Defaults to 0. | ||
| * | ||
| * @throws \InvalidArgumentException | ||
| * @see ContainerAwareEventDispatcherInterface::addListenerService |
There was a problem hiding this comment.
should be @{inheritdoc} instead.
|
@Nicofuma Can you share a URL discussing the issue for phpBB? |
|
@fabpot It hasn't been discussed properly speaking because I discovered it while testing some of my PR together and I fixed it in phpbb/phpbb#2948 after the first comment of Stof. |
Nicofuma
force-pushed
the
ev_containerawareinterface
branch
from
d7718dc to
5a51ef8
Compare
Nicofuma
force-pushed
the
ev_containerawareinterface
branch
from
5a51ef8 to
ef3d6bd
Compare
|
So, does this PR still make sense? |
|
It's the question I asked before, and because I don't know if it still make sense (currently I see one use case for that: be able to identify the lazy loaded listeners in a TraceableEventDIspatcher) I kept updating it. So feel free to close it if you want. |
|
Ok, I'm closing it for now. |
|
being able to identify lazy-loaded listeners in the TraceableEventDispatcher does not need this once #12019 is merged (we could have a way to identify them) |
Currently if a class depends on the ContainerAwareEventDispatcher it's not possible to replace it by the TraceableEventDispatcher without rewriting the class.