10000 TraceableEventDispatcher reset should instantiate SplObjectStorage · Issue #29703 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

TraceableEventDispatcher reset should instantiate SplObjectStorage #29703

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
lievertz opened this issue Dec 27, 2018 · 3 comments
Closed

TraceableEventDispatcher reset should instantiate SplObjectStorage #29703

lievertz opened this issue Dec 27, 2018 · 3 comments
Labels
Bug EventDispatcher Status: Needs Review

Comments

@lievertz
Copy link

Symfony version(s) affected: 3.4.x-dev (from #29411)

Description
TraceableEventDispatcher was rewritten to use callStack as SplObjectStorage instead of array, but #reset() still assigns callStack to array(), which throws error at line 268 with 'Call to a member function attach() on array' on next call to dispatch().

How to reproduce
Just grab the TraceableEventDispatcher, call reset(), and call dispatch() on anything.

<?php
class BugReproduceTest extends \Codeception\Test\Unit
{
    /**
     * @group repro
     */
    public function testReproduce()
    {
        $dispatcher = $this->getModule( 'Symfony' )->grabService( 'debug.event_dispatcher' );
        $dispatcher->reset();
        $dispatcher->dispatch( 'kernel.request' );
    }
}

Possible Solution
Suggest a simple change
from (starting line 222):

    public function reset()
    {
        $this->callStack = array();
    }

to (starting line 222)

    public function reset()
    {
        $this->callStack = new \SplObjectStorage();
    }

Additional context
N/A
@lievertz
Copy link
Author

@ro0NL it looks like you may know more about the context of this, so you may be interested to review.

@xabbuh
Copy link
Member
xabbuh commented Dec 27, 2018

I think we just need to reset $this->callStack to null.

@lievertz Would you like to create a pull request?

@lievertz lievertz mentioned this issue Dec 28, 2018
Merged
@lievertz
Copy link
Author

Good call. Thank you! Created #29714

@fabpot fabpot closed this as completed Dec 29, 2018
fabpot added a commit that referenced this issue Dec 29, 2018
@fabpot
bug #29714 [Event Dispatcher] fixed 29703: TraceableEventDispatcher r…
f82beb5 
…eset() callStack to null (mlievertz)

This PR was merged into the 3.4 branch.

Discussion
----------

[Event Dispatcher] fixed 29703: TraceableEventDispatcher reset() callStack to null

[Event Dispatcher] fixed 29703: TraceableEventDispatcher reset now sets callStack to null with test to dispatch after reset.

| Q             | A
| ------------- | ---
| Branch?       | 3.4 <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #29703    <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A <!-- required for new features -->

[Event Dispatcher] fixed 29703: TraceableEventDispatcher reset now sets callStack to null with test to dispatch after reset. Basically #29411 introduced an issue where calling dispatch after a reset would throw an error because now reset set callStack to array but dispatch expected either SplObjectStorage or null. Now reset sets null. Also added a test to verify dispatch works following a call to reset.

Commits
-------

51bcdb8 [Event Dispatcher] fixed 29703: TraceableEventDispatcher reset now sets callStack to null with test to dispatch after reset.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug EventDispatcher Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@fabpot @xabbuh @carsonbot @lievertz
0