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

Display orphaned events in profiler #24392

Closed
wants to merge 5 commits into
base: master
from

Conversation

@kejwmen
Contributor

kejwmen commented Oct 1, 2017

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

This comment has been minimized.

Member

nicolas-grekas commented Oct 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 nicolas-grekas added this to the 3.4 milestone Oct 2, 2017

@stof

This comment has been minimized.

Member

stof commented Oct 2, 2017

I think we could deprecate the interface

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

This comment has been minimized.

@stof

stof Oct 2, 2017

Member

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

@kejwmen

This comment has been minimized.

Contributor

kejwmen commented Oct 2, 2017

@stof @nicolas-grekas
Deprecated TraceableEventDispatcherInterface.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Oct 8, 2017

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

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

@kejwmen

This comment has been minimized.

Contributor

kejwmen commented Oct 14, 2017

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

@kejwmen

This comment has been minimized.

Contributor

kejwmen commented Oct 22, 2017

@stof could you check updated code?

Tests failure looks unrelated.

@@ -52,9 +53,10 @@ public function reset()
public function lateCollect()
{
if ($this->dispatcher instanceof TraceableEventDispatcherInterface) {
if ($this->dispatcher instanceof TraceableEventDispatcher) {

This comment has been minimized.

@ro0NL

ro0NL Oct 26, 2017

Contributor

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

This comment has been minimized.

@kejwmen

kejwmen Oct 26, 2017

Contributor

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

@nicolas-grekas, what do you think?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 26, 2017

Member

Legacy code should continue to work

@@ -252,6 +254,10 @@ protected function postDispatch($eventName, Event $event)
private function preProcess($eventName)
{
if (false === $this->dispatcher->hasListeners($eventName)) {

This comment has been minimized.

@ro0NL

ro0NL Oct 26, 2017

Contributor

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

@@ -252,6 +254,10 @@ protected function postDispatch($eventName, Event $event)
private function preProcess($eventName)
{
if (false === $this->dispatcher->hasListeners($eventName)) {
$this->orphanedEvents[] = $eventName;

This comment has been minimized.

@ro0NL

ro0NL Oct 26, 2017

Contributor

return; here

*/
public function setCalledListeners(array $listeners)
{
$this->data['called_listeners'] = $listeners;
}
/**
* Gets the called listeners.

This comment has been minimized.

@ro0NL

ro0NL Oct 26, 2017

Contributor

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

@kejwmen

This comment has been minimized.

Contributor

kejwmen commented Oct 26, 2017

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

@ro0NL

ro0NL approved these changes Oct 27, 2017

LGTM 👍

@@ -88,7 +95,7 @@ public function getCalledListeners()
*
* @param array $listeners An array of not called listeners

This comment has been minimized.

@Simperfit

Simperfit Oct 30, 2017

Contributor

this seems useless

*/
public function getNotCalledListeners()
{
return $this->data['not_called_listeners'];
}
/**
* Sets the orphaned events.
*
* @param array $events An array of orphaned events

This comment has been minimized.

@Simperfit

Simperfit Oct 30, 2017

Contributor

this seems useless too

@Simperfit

left 2 minor comments but LGTM

@kejwmen

This comment has been minimized.

Contributor

kejwmen commented Nov 1, 2017

rebased

@xabbuh

This comment has been minimized.

Member

xabbuh commented Nov 7, 2017

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

@kejwmen

This comment has been minimized.

Contributor

kejwmen commented Dec 3, 2017

Rebased, deprecations documented in UPGRADE.

@@ -1,6 +1,11 @@
UPGRADE FROM 4.0 to 4.1
=======================
Event Dispatcher

This comment has been minimized.

@xabbuh

xabbuh Dec 4, 2017

Member

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

@@ -1,6 +1,11 @@
UPGRADE FROM 4.0 to 4.1
=======================
Event Dispatcher
-----------

This comment has been minimized.

@xabbuh

xabbuh Dec 4, 2017

Member

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

kejwmen added some commits Oct 1, 2017

@kejwmen

This comment has been minimized.

Contributor

kejwmen commented Dec 28, 2017

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

Rebased and corrected.

@xabbuh

xabbuh approved these changes Dec 31, 2017

@fabpot

fabpot approved these changes Jan 19, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Jan 19, 2018

Thank you @kejwmen.

@fabpot fabpot closed this in 0a56a37 Jan 19, 2018

ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request Mar 25, 2018

feature symfony#24392 Display orphaned events in profiler (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

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

fabpot added a commit that referenced this pull request Jul 12, 2018

bug #27913 [EventDispatcher] Clear orphaned events on reset (acasadem…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment