store $this->id in local $id to avoid errors when this value is changed #9748
Conversation
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #9727 |
| License | MIT |
…ed during execution of preDispatch and postDispatch method
|
closing in favor of #9737 |
|
@fabpot This should still be considered. It is about preserving the id in different methods during which the id may change than However, I would like to see a testcase reproducing a case where the id is modified during these methods to demonstrate the fix |
|
@pjordaan Can you fork the Symfony Standard Edition to show some code that shows the issue fixed here? |
|
I'll see if I can make a working case, but I'll probably post if somewhere in the new year. The code I'm currently working on is proprietary. The test case written in pull request #9750 is applicable. In my case the code I have a service which does something like this: class MyService
{
public function __construct(EventDispatcherInterface $dispatcher) {
$this->dispatcher = $dispatcher;
$this->update()
}
public function update() {
//some code
$this->dispatcher->dispatch("<event>");
}
} |
|
A probably better way to fix it is to ensure that the wrappedListener item exists before using it. I've replaced : foreach ($listeners as $listener) {
$this->dispatcher->removeListener($eventName, $listener);
$wrapped = $this->wrapListener($eventName, $listener);
$this->wrappedListeners[$this->id][$wrapped] = $listener;
$this->dispatcher->addListener($eventName, $wrapped);
}by foreach ($listeners as $listener) {
$this->dispatcher->removeListener($eventName, $listener);
$wrapped = $this->wrapListener($eventName, $listener);
if (!array_key_exists($this->id, $this->wrappedListeners)) {
$this->wrappedListeners[$this->id] = new \SplObjectStorage();
}
$this->wrappedListeners[$this->id][$wrapped] = $listener;
$this->dispatcher->addListener($eventName, $wrapped);
}and it works like a charm. But I'm not able to reproduce my previous bug in an unit test. i still don't understand why the unit test described in #9750 works and do not throw this kind of warning. Maybe the PHP warning is just ignored by PHPUnit. |
|
ok, seems I still have cases in dev where this warning is still being produced, but it doesn't happen all the time, which is really annoying. I could fine one case where it goes wrong and see if I can write a unit test for it. some class dispatches event1 |
|
+1 to @remialvado solution, i had the same problem with the TraceableEventDispatcher (#9727), and with that piece of code it works perfectly now. |
|
I think that I can reproduce the error. The steps are:
I hope you can understand the steps It has the fix of @remialvado. |
…cher (fabpot) This PR was merged into the 2.3 branch. Discussion ---------- [HttpKernel] fixed wrong reference in TraceableEventDispatcher | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #9748, #9727 | License | MIT | Doc PR | n/a This PR fixes #9748 and #9727 by removing the `id` state. Only private method signatures have been changed, so that qualifies for a fix in 2.3. The `getNotCalledListeners()` is a bit special as it tries to get non-called listeners. It passes `null` as the event id as if a listener has been called more than once, getting the first call is enough. Commits ------- acd3317 [HttpKernel] fixed wrong reference in TraceableEventDispatcher
|
closing in favor of #10191 |