Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[EventDispatcher] Handle laziness internally instead of relying on ClosureProxyArgument#23008
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
chalasr left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think its a nice have for the dispatcher itself and that the bit of complexity added here isn't a big deal as it allows removing much more from DI. 👍
(adding some tests for this would be nice I guess)
ded0bb4 tob2a71e2Comparenicolas-grekas commentedJun 1, 2017
now with simplified logic |
Gemineye commentedJun 1, 2017
This patch seems to fix my Issue in#22970. 👍 |
b2a71e2 to96b7bf0Compare| if ($listeners) { | ||
| $this->listeners[$eventName][$priority] =$listeners; | ||
| }else { | ||
| unset($this->listeners[$eventName][$priority]); |
nicolas-grekasJun 1, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this "unset" allows better cleanup of the$this->listeners property and explains the replacement of "isset" by "empty" in a few places above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
oh, and this most importantly helps the new implem of hasListeners (which is lazy-compliant now)
weaverryan commentedJun 1, 2017
This also appears to fix#23011 |
weaverryan left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Theisset ->empty change added some complexity, at least for reviewing (could that be done in a separate PR?). But, it makes sense to me, looks good and obviously all the existing tests still pass
| }else { | ||
| $listeners[$k] =$v; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this identical to the code down insortListeners()? If so, perhaps aprivate function resolveListeners($eventName) or something similar? I'm finding some of the logic a bit hard to follow - organizing into a private function(s) might also help me understand things :)
nicolas-grekasJun 1, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
not sure about the private: performance is critical in this components
nicolas-grekas commentedJun 1, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
not without conflicts, which means process overhead, I'd prefer not |
weaverryan commentedJun 1, 2017
👍 |
dmaicher commentedJun 1, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Some tests would be good for it I think 😄 👍 Edit: ah did not see your comment 😉
|
| if ($v ===$listener) { | ||
| unset($listeners[$k],$this->sorted[$eventName]); | ||
| }else { | ||
| $listeners[$k] =$v; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
can we not useforeach ($listeners as $k => &$v) { instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
references don't play well with php 5, I try to avoid them as much as possible
96b7bf0 todd68442Comparenicolas-grekas commentedJun 1, 2017
Now with tests |
dd68442 toc17a009Comparefabpot commentedJun 1, 2017
Thank you@nicolas-grekas. |
…lying on ClosureProxyArgument (nicolas-grekas)This PR was merged into the 3.3 branch.Discussion----------[EventDispatcher] Handle laziness internally instead of relying on ClosureProxyArgument| Q | A| ------------- | ---| Branch? | 3.3| Bug fix? | yes| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#22970| License | MIT| Doc PR | -If we decide to go this way, we might drop ClosureProxyArgument entirely because we won't need it internally. I'll propose it in another PR for discussion if this one is accepted.This PR allows "high-order" listeners, as `array(Closure, method)`. Closure is called to get the actual instance when needed only.How does it look to you? (I'll add tests once confirmed)Commits-------c17a009 [EventDispatcher] Handle laziness internally instead of relying on ClosureProxyArgument
This PR was merged into the 3.3 branch.Discussion----------[Di] Remove closure-proxy arguments| Q | A| ------------- | ---| Branch? | 3.3| Bug fix? | no| New feature? | no| BC breaks? | yes| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -With#23008, we don't need this in core anymore.Let's drop it now.Technically that's a BC break, but for a feature that is very new, and still quite hidden.Doing this now would save us from maintaining this code, and help reduce the overall complexity.Basically reverts#20953Commits-------57daadb [Di] Remove closure-proxy arguments
vudaltsov commentedOct 20, 2017
@nicolas-grekas , am I right that now if I create a subscriber like this: <?phpnamespaceAppBundle\EventListener;useSymfony\Component\EventDispatcher\EventSubscriberInterface;class TestListenerimplements EventSubscriberInterface{publicfunction__construct() {dump('constructor'); }publicstaticfunctiongetSubscribedEvents() {return ['test' =>'test', ]; }publicfunctiontest() {dump('fired'); }} I should not see |
stof commentedOct 20, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@vudaltsov not exactly. The profiler itself triggers initialization currently, due to collecting not-called listeners, which gets all of them. |
vudaltsov commentedOct 20, 2017
@stof , thank you! Now I understand what the problem was... With profiler disabled everything works as expected, so I can rely on this in production. |
Uh oh!
There was an error while loading.Please reload this page.
If we decide to go this way, we might drop ClosureProxyArgument entirely because we won't need it internally. I'll propose it in another PR for discussion if this one is accepted.
This PR allows "high-order" listeners, as
array(Closure, method). Closure is called to get the actual instance when needed only.How does it look to you? (I'll add tests once confirmed)