Skip to content
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.

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

TraceableEventDispatcher reset should instantiate SplObjectStorage #29703

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

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
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
…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
Projects
None yet
Development

No branches or pull requests

4 participants