NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

Display orphaned events in profiler#24392

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

mateuszsip wants to merge5 commits intosymfony:masterfrommateuszsip:feature/orphaned-events

Closed

Display orphaned events in profiler#24392

mateuszsip wants to merge5 commits intosymfony:masterfrommateuszsip:feature/orphaned-events

Conversation

Copy link

Contributor

mateuszsip commentedOct 1, 2017•
edited
Loading

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.

DeprecatingTraceableEventDispatcherInterface.

carsonbot added Status: Needs Review Feature BC Break Deprecation labels

Oct 1, 2017

Copy link

Member

nicolas-grekas commentedOct 2, 2017

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.

nicolas-grekas added this to the3.4 milestone

Oct 2, 2017

Copy link

Member

stof commentedOct 2, 2017

I think we could deprecate the interface

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Oct 2, 2017

stof requested changes

Oct 2, 2017

View reviewed changes

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

		// THEN
		$this->assertEmpty($events);

		// SCENARIO - Returns orphaned event

Copy link

Member

stofOct 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

mateuszsip commentedOct 2, 2017

@stof @nicolas-grekas
DeprecatedTraceableEventDispatcherInterface.

xabbuh mentioned this pull request

Oct 5, 2017

Log orphan events on the profiler#24347

Closed

Copy link

Member

nicolas-grekas commentedOct 8, 2017

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

nicolas-grekas modified the milestones:3.4,4.1

Oct 8, 2017

mateuszsip force-pushed thefeature/orphaned-events branch fromed8fa01 to5fdb763Compare

October 13, 2017 23:54

Copy link

ContributorAuthor

mateuszsip commentedOct 14, 2017

Rebased, updated changelogs andTraceableEventDispatcherInterface deprecation notice with new target version.

Copy link

ContributorAuthor

mateuszsip commentedOct 22, 2017

@stof could you check updated code?

Tests failure looks unrelated.

ro0NL reviewed

Oct 26, 2017

View reviewed changes

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

		publicfunctionlateCollect()
		{
		if ($this->dispatcherinstanceofTraceableEventDispatcherInterface) {
		if ($this->dispatcherinstanceofTraceableEventDispatcher) {

Copy link

Contributor

ro0NLOct 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

wanted? What about an additional instanceof check before callingsetOrphanedEvents.

Copy link

ContributorAuthor

mateuszsipOct 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Good question.
I went this way becauseTraceableEventDispatcherInterface gets depracted anyway.

@nicolas-grekas, what do you think?

Copy link

Member

nicolas-grekasOct 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Legacy code should continue to work

ro0NL reviewed

Oct 26, 2017

View reviewed changes

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


		privatefunctionpreProcess($eventName)
		{
		if (false ===$this->dispatcher->hasListeners($eventName)) {

Copy link

Contributor

ro0NLOct 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

		privatefunctionpreProcess($eventName)
		{
		if (false ===$this->dispatcher->hasListeners($eventName)) {
		$this->orphanedEvents[] =$eventName;

Copy link

Contributor

ro0NLOct 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

return; here

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

		}

		/**
		*Gets the called listeners.

Copy link

Contributor

ro0NLOct 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

mateuszsip commentedOct 26, 2017

Thanks@ro0NL for review.
Fixed everything you catched, including separateTraceableEventDispatcher check to not break code relying onTraceableEventDispatcherInterface.

ro0NL approved these changes

Oct 27, 2017

View reviewed changes

Copy link

Contributor

ro0NL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM 👍

carsonbot added Status: Reviewed and removed Status: Needs Work labels

Oct 27, 2017

Simperfit reviewed

Oct 30, 2017

View reviewed changes

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

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

Copy link

Contributor

SimperfitOct 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

this seems useless

Simperfit reviewed

Oct 30, 2017

View reviewed changes

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

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

Copy link

Contributor

SimperfitOct 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

this seems useless too

Simperfit approved these changes

Oct 30, 2017

View reviewed changes

Copy link

Contributor

Simperfit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

left 2 minor comments but LGTM

mateuszsip force-pushed thefeature/orphaned-events branch from09a0fa1 toe18f77cCompare

November 1, 2017 00:25

Copy link

ContributorAuthor

mateuszsip commentedNov 1, 2017

rebased

Copy link

Member

xabbuh commentedNov 7, 2017

deprecations will have to be documented in theUPGRADE-4.1.md andUPGRADE-5.0.md(to be created) files

mateuszsip force-pushed thefeature/orphaned-events branch frome18f77c tofb16cfaCompare

December 3, 2017 17:34

Copy link

ContributorAuthor

mateuszsip commentedDec 3, 2017

Rebased, deprecations documented inUPGRADE.

xabbuh reviewed

Dec 4, 2017

View reviewed changes

UPGRADE-4.1.md Outdated

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

		Event Dispatcher

Copy link

Member

xabbuhDec 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

xabbuh reviewed

Dec 4, 2017

View reviewed changes

UPGRADE-4.1.md Outdated

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

		Event Dispatcher
		-----------

Copy link

Member

xabbuhDec 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

mateuszsip added4 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 thefeature/orphaned-events branch fromfb16cfa toe53b7f3Compare

December 28, 2017 02:53

Corrected component name in UPGRADE

9d5cae5

Copy link

ContributorAuthor

mateuszsip commentedDec 28, 2017

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

Rebased and corrected.

xabbuh approved these changes

Dec 31, 2017

View reviewed changes

fabpot approved these changes

Jan 19, 2018

View reviewed changes

Copy link

Member

fabpot commentedJan 19, 2018

Thank you @kejwmen.

fabpot closed this in0a56a37

Jan 19, 2018

nicolas-grekas removed the BC Break label

Feb 22, 2018

ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request

Mar 25, 2018

featuresymfony#24392Display orphaned events in profiler (kejwmen)

341200a

This PR was squashed before being merged into the 4.1-dev branch (closessymfony#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       | MITImplementssymfony#24347.Deprecating `TraceableEventDispatcherInterface`.Commits-------509f9a9 Display orphaned events in profiler

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

fabpot added a commit that referenced this pull request

Jul 12, 2018

bug#27913[EventDispatcher] Clear orphaned events on reset (acasadem…

10f7dcc

…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        | NAWhen 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