Display orphaned events in profiler #24392

mateuszsip · 2017-10-01T20:57:30Z

Q	A
Branch?	4.1
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#24347
License	MIT

Implements #24347.

Deprecating TraceableEventDispatcherInterface.

nicolas-grekas · 2017-10-02T09:11:58Z

The BC break should be removed, as we have a policy of not doing hard BC break: any future BC break needs to be announced with a runtime deprecation notice in 3.4.
So, the addition to the interface must be reverted, and another way should be found.

Which makes me wonder BTW: do we need TraceableEventDispatcherInterface at all? Is it really something we want ppl to be able to implement on their own? If no, then let's just deprecate it.

stof · 2017-10-02T09:20:48Z

I think we could deprecate the interface

stof · 2017-10-02T09:21:54Z

src/Symfony/Component/EventDispatcher/Tests/Debug/TraceableEventDispatcherTest.php

+        // THEN
+        $this->assertEmpty($events);
+
+        // SCENARIO - Returns orphaned event


different scenarios should be different tests (so that a failure in the first oen does not prevent others from running)

mateuszsip · 2017-10-02T17:45:05Z

@stof @nicolas-grekas
Deprecated TraceableEventDispatcherInterface.

nicolas-grekas · 2017-10-08T16:34:14Z

Moving to 4.1. Rebase on master might be needed, where PHP 7.1 features can be used btw.

mateuszsip · 2017-10-14T00:00:27Z

Rebased, updated changelogs and TraceableEventDispatcherInterface deprecation notice with new target version.

mateuszsip · 2017-10-22T22:46:11Z

@stof could you check updated code?

Tests failure looks unrelated.

ro0NL · 2017-10-26T09:52:46Z

src/Symfony/Component/HttpKernel/DataCollector/EventDataCollector.php

    public function lateCollect()
    {
-        if ($this->dispatcher instanceof TraceableEventDispatcherInterface) {
+        if ($this->dispatcher instanceof TraceableEventDispatcher) {


wanted? What about an additional instanceof check before calling setOrphanedEvents.

Good question.
I went this way because TraceableEventDispatcherInterface gets depracted anyway.

@nicolas-grekas, what do you think?

Legacy code should continue to work

ro0NL · 2017-10-26T19:37:47Z

src/Symfony/Component/EventDispatcher/Debug/TraceableEventDispatcher.php


    private function preProcess($eventName)
    {
+        if (false === $this->dispatcher->hasListeners($eventName)) {


!$this->dispatcher->has...

ro0NL · 2017-10-26T19:38:06Z

src/Symfony/Component/EventDispatcher/Debug/TraceableEventDispatcher.php

    private function preProcess($eventName)
    {
+        if (false === $this->dispatcher->hasListeners($eventName)) {
+            $this->orphanedEvents[] = $eventName;


return; here

ro0NL · 2017-10-26T19:41:52Z

src/Symfony/Component/HttpKernel/DataCollector/EventDataCollector.php

    }

    /**
-     * Gets the called listeners.


the diff is kinda messy... cant we avoid it by simply appending set+get (which seem to be the pattern anyway).

mateuszsip · 2017-10-26T20:57:05Z

Thanks @ro0NL for review.
Fixed everything you catched, including separate TraceableEventDispatcher check to not break code relying on TraceableEventDispatcherInterface.

ro0NL

LGTM 👍

Simperfit · 2017-10-30T05:42:08Z

src/Symfony/Component/HttpKernel/DataCollector/EventDataCollector.php

    /**
     * Sets the not called listeners.
     *
     * @param array $listeners An array of not called listeners


this seems useless

Simperfit · 2017-10-30T05:42:42Z

src/Symfony/Component/HttpKernel/DataCollector/EventDataCollector.php

+    /**
+     * Sets the orphaned events.
+     *
+     * @param array $events An array of orphaned events


this seems useless too

Simperfit

left 2 minor comments but LGTM

mateuszsip · 2017-11-01T00:25:51Z

rebased

xabbuh · 2017-11-07T14:41:04Z

deprecations will have to be documented in the UPGRADE-4.1.md and UPGRADE-5.0.md(to be created) files

mateuszsip · 2017-12-03T17:44:45Z

Rebased, deprecations documented in UPGRADE.

xabbuh · 2017-12-04T08:32:34Z

UPGRADE-4.1.md

 UPGRADE FROM 4.0 to 4.1
 =======================

+Event Dispatcher


EventDispatcher (that's the name of the component :))

xabbuh · 2017-12-04T08:32:50Z

UPGRADE-4.1.md

 =======================

+Event Dispatcher
+-----------


please use as many dashes as the length of the component's name

… in collector

…implementation

…roduced interface deprecation

mateuszsip · 2017-12-28T02:59:17Z

Thanks @xabbuh, I wasn't aware of that.

Rebased and corrected.

fabpot · 2018-01-19T07:23:56Z

Thank you @kejwmen.

This PR was squashed before being merged into the 4.1-dev branch (closes symfony#24392). Discussion ---------- Display orphaned events in profiler | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | symfony#24347 | License | MIT Implements symfony#24347. Deprecating `TraceableEventDispatcherInterface`. Commits ------- 509f9a9 Display orphaned events in profiler

…ont) This PR was merged into the 4.1 branch. Discussion ---------- [EventDispatcher] Clear orphaned events on reset | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | NA | License | MIT | Doc PR | NA When the Orphaned Events feature was added in #24392 it was forgotten to also clear them when the `reset` method on the `TraceableEventDispatcher` is called. This makes the Orphaned Events tab on the Event profiler an evergrowing list when using PHP-PM (or other event loop implementations). Commits ------- d3260df [EventDispatcher] Clear orphaned events on TraceableEventDispatcher::reset

carsonbot added Status: Needs Review Feature BC Break Deprecation labels Oct 1, 2017

nicolas-grekas added this to the 3.4 milestone Oct 2, 2017

carsonbot added Status: Needs Work and removed Status: Needs Review labels Oct 2, 2017

stof requested changes Oct 2, 2017

View reviewed changes

xabbuh mentioned this pull request Oct 5, 2017

Log orphan events on the profiler #24347

Closed

nicolas-grekas modified the milestones: 3.4, 4.1 Oct 8, 2017

mateuszsip force-pushed the feature/orphaned-events branch from ed8fa01 to 5fdb763 Compare October 13, 2017 23:54

ro0NL reviewed Oct 26, 2017

View reviewed changes

ro0NL approved these changes Oct 27, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Oct 27, 2017

Simperfit reviewed Oct 30, 2017

View reviewed changes

Simperfit approved these changes Oct 30, 2017

View reviewed changes

mateuszsip force-pushed the feature/orphaned-events branch from 09a0fa1 to e18f77c Compare November 1, 2017 00:25

mateuszsip force-pushed the feature/orphaned-events branch from e18f77c to fb16cfa Compare December 3, 2017 17:34

xabbuh reviewed Dec 4, 2017

View reviewed changes

mateuszsip added 4 commits December 28, 2017 03:50

Added orphaned events to TraceableEventDispatcherInterface, supported…

6fa817b

… in collector

Drop usage of TraceableEventDispatcherInterface in favor of concrete …

e637fb3

…implementation

removed getOrphanedEvents from TraceableEventDispatcherInterface, int…

59c8adf

…roduced interface deprecation

Deprecations documented in UPGRADE

e53b7f3

mateuszsip force-pushed the feature/orphaned-events branch from fb16cfa to e53b7f3 Compare December 28, 2017 02:53

Corrected component name in UPGRADE

9d5cae5

xabbuh approved these changes Dec 31, 2017

View reviewed changes

fabpot approved these changes Jan 19, 2018

View reviewed changes

fabpot closed this in 0a56a37 Jan 19, 2018

nicolas-grekas removed the BC Break label Feb 22, 2018

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

acasademont mentioned this pull request Jul 10, 2018

[EventDispatcher] Clear orphaned events on reset #27913

Merged

TavoNiievez mentioned this pull request Dec 9, 2020

Added seeEventTriggered function Codeception/module-symfony#68

Merged

TavoNiievez mentioned this pull request Jan 8, 2021

Use traits for methods that belong to the public API Codeception/module-symfony#94

Merged

Uh oh!

Display orphaned events in profiler #24392

Display orphaned events in profiler #24392

Uh oh!

Conversation

mateuszsip commented Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Oct 2, 2017

Uh oh!

stof commented Oct 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateuszsip commented Oct 2, 2017

Uh oh!

nicolas-grekas commented Oct 8, 2017

Uh oh!

mateuszsip commented Oct 14, 2017

Uh oh!

mateuszsip commented Oct 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateuszsip commented Oct 26, 2017

Uh oh!

ro0NL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Simperfit left a comment

Choose a reason for hiding this comment

Uh oh!

mateuszsip commented Nov 1, 2017

Uh oh!

xabbuh commented Nov 7, 2017

Uh oh!

mateuszsip commented Dec 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateuszsip commented Dec 28, 2017

Uh oh!

fabpot commented Jan 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mateuszsip commented Oct 1, 2017 •

edited

Loading