8000 [HttpKernel] Nested events in TraceableEventDispatcher · Issue #9710 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpKernel] Nested events in TraceableEventDispatcher #9710

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
evillemez opened this issue Dec 5, 2013 · 8 comments
Closed

[HttpKernel] Nested events in TraceableEventDispatcher #9710

evillemez opened this issue Dec 5, 2013 · 8 comments
Labels
HttpKernel

Comments

@evillemez
Copy link

The HttpKernel\Debug\TraceableEventDispatcher cannot handle nested dispatches of the same event instance. This behavior differs from the default implementation in EventDispatcher\EventDispatcher, which does not care what event is being dispatched.

I have some tests demonstrating the issue in my own fork here:
https://github.com/evillemez/symfony/blob/events/src/Symfony/Component/HttpKernel/Tests/Debug/TraceableEventDispatcherTest.php#L151

A related issue I discovered is that, when handling an event dispatched by TraceableEventDispatcher calls to $e->getDispatcher() return the wrapped EventDispatcher instance, not the wrapping TraceableEventDispatcher instance. So, if an event is being re-dispatched under a new name, those calls are not being traced. I'm not sure if that should be a separate issue, or not.

I'll start working on a PR to make the behaviors match, but would like some feedback.
evillemez pushed a commit to evillemez/WebServicesBundle that referenced this issue Dec 5, 2013
temporary hack around symfony/symfony#9710 regarding nested events in…
ae2463c 
… the TraceableEventDispatcher
@stof
Copy link
Member
stof commented Dec 5, 2013

which version are you using ? I think we already fixed the issue about the dispatcher instance in Symfony 2.4

@evillemez
Copy link
Author

The tests in my fork are against the current master - should I rebase them onto 2.4?

@evillemez
Copy link
Author

Either way though, the issue should still be present in 2.4. It's happening because TraceableEventDispatcher is using spl_object_hash to set the id for the wrapped event, and then unsetting it. So, if it the same event is dispatched twice, the hash is the same and it tries to unset something that doesn't exist.

@evillemez
Copy link
Author

Sorry for the comment spam - the specific line where the issue pops up is:
https://github.com/symfony/symfony/blob/2.4/src/Symfony/Component/HttpKernel/Debug/TraceableEventDispatcher.php#L376

@stof
Copy link
Member
stof commented Dec 5, 2013

oh, you mean you are using the same Event instance twice when dispatching the events ? be careful with this as it can creates weird issues if a listener calls $event->stopPropagation() when you reuse the instance.

@evillemez
Copy link
Author

Yeah, you have to manually ensure that propagation has not stopped, or be sure to return immediately if it has.

Do you think I should go ahead and submit a PR? Also, should I add a test in the EventDispatcher component that demonstrates the default behavior? What I have linked above in my fork was a quick example test so I put it all in one spot.

@stof
Copy link
Member
stof commented Dec 6, 2013

@evillemez what I mean is that stopping the propagation for 1 event will stop it for the other dispatching as well, which may not be what you expect.

However, fixing the issue should be easy IMO: you just need to generate a random id in the dispatch method, assign it to a local variable and to $this->id, and then assign the local variable again into $this->id.
We just need to avoid reusing the same id.
I think we could build the id using the object hash concatenated with a random string ($id = spl_object_hash($event) . mt_rand() for instance). this way, we ensure it is always different for the common case, but we still support your edge case.
what do you think ?

@evillemez
Copy link
Author

@stof Ah, right, yeah that is the behavior I was expecting, though.

I think your solution is more straight forward than mine, I was going implement some sort of reference counting based on the id. A random number would probably be fine though and less work. I'll put together a PR when I get to work in the morning.

This was referenced Dec 11, 2013
Closed
Merged
fabpot added a commit that referenced this issue Dec 12, 2013
@fabpot
bug #9750 allow TraceableEventDispatcher to reuse event instance in n…
d490e5a 
…ested events (evillemez)

This PR was merged into the 2.3 branch.

Discussion
----------

allow TraceableEventDispatcher to reuse event instance in nested events

| Q                       | A
| -------------            | ---
| Bug fix?             | yes
| New feature?      | no
| BC breaks?        | no
| Deprecations?    | no
| Tests pass?       | yes
| Fixed tickets      | #9710, #9727
| License              | MIT

Commits
-------

454ce16 allow TraceableEventDispatcher to reuse event instance in nested events
@fabpot fabpot closed this as completed Dec 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
HttpKernel
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@fabpot @evillemez @stof
0