8000 [EventDispatcher] Fix TraceableEventDispatcher FC/BC layer by chalasr · Pull Request #31254 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[EventDispatcher] Fix TraceableEventDispatcher FC/BC layer #31254

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
Merged
merged 1 commit into from
Apr 27, 2019
Merged

[EventDispatcher] Fix TraceableEventDispatcher FC/BC layer #31254

merged 1 commit into from
Apr 27, 2019

Conversation

@chalasr
Copy link
Member
@chalasr chalasr commented Apr 25, 2019 •
edited
Loading
Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? n/a
Tests pass? yes
Fixed tickets #31221
License MIT
Doc PR n/a

also renames WrappedEvent to LegacyEventProxy

@chalasr chalasr added this to the next milestone Apr 25, 2019
@chalasr chalasr mentioned this pull request Apr 25, 2019
Closed
yceruto
yceruto approved these changes Apr 25, 2019
View reviewed changes
Copy link
Member
@yceruto yceruto left a comment

Choose a reason for hiding this comment

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

Yes, this solves my issue, thanks!

@chalasr chalasr force-pushed the event-prepostdispatch branch 2 times, most recently from 0bef732 to 1add058 Compare April 25, 2019 19:31
@chalasr chalasr force-pushed the event-prepostdispatch branch 2 times, most recently from adef564 to fd24888 Compare April 25, 2019 21:23
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Apr 26, 2019

See alternative in #31258

@chalasr chalasr force-pushed the event-prepostdispatch branch from fd24888 to 6a78e46 Compare April 26, 2019 21:27
chalasr
chalasr commented Apr 26, 2019
View reviewed changes
src/Symfony/Component/EventDispatcher/EventDispatcher.php
};
} else {
$closure = $listener instanceof \Closure ? $listener : \Closure::fromCallable($listener);
$closure = $listener instanceof \Closure || $listener instanceof WrappedListener ? $listener : \Closure::fromCallable($listener);
Copy link
Member Author
8000
@chalasr chalasr Apr 26, 2019

Choose a reason for hiding this comment

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

WrappedListener handles this optimization internally

@chalasr chalasr mentioned this pull request Apr 26, 2019
Closed
@chalasr
Copy link
Member Author
chalasr commented Apr 26, 2019

Remaining issue fixed, ready.

@chalasr chalasr force-pushed the event-prepostdispatch branch 2 times, most recently from a14a4ed to c34910e Compare April 26, 2019 22:14
@xabbuh
Copy link
Member
xabbuh commented Apr 27, 2019

What about also adding an assertion to the test which checks that the event instance passed to the listener actually is an instance of the ContractsEvent?

8000
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Apr 27, 2019

the event instance passed to the listener actually is an instance of the ContractsEvent?

implementing this interface is not a requirement

@chalasr chalasr force-pushed the event-prepostdispatch branch from c34910e to c5b3b34 Compare April 27, 2019 13:05
@chalasr
Copy link
Member Author
chalasr commented Apr 27, 2019

Assertion added to ensure that the event passed to the listener is the same as the dispatched one (and not a proxy), also rebased to make the CI green.

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Apr 27, 2019
< 8000 task-lists disabled sortable>

Thank you @chalasr.

@nicolas-grekas nicolas-grekas merged commit c5b3b34 into symfony:master Apr 27, 2019
nicolas-grekas added a commit that referenced this pull request Apr 27, 2019
@nicolas-grekas
bug #31254 [EventDispatcher] Fix TraceableEventDispatcher FC/BC layer…
6843905 
… (chalasr)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[EventDispatcher] Fix TraceableEventDispatcher FC/BC layer

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | n/a
| Tests pass?   | yes
| Fixed tickets | #31221
| License       | MIT
| Doc PR        | n/a

also renames `WrappedEvent` to `LegacyEventProxy`

Commits
-------

c5b3b34 [EventDispatcher] Fix TraceableEventDispatcher FC/BC layer
@chalasr chalasr deleted the event-prepostdispatch branch April 27, 2019 13:43
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@yceruto yceruto yceruto approved these changes

+1 more reviewer

@Simperfit Simperfit Simperfit approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug EventDispatcher Status: Reviewed

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

6 participants

@chalasr @nicolas-grekas @xabbuh @yceruto @Simperfit @carsonbot
0