[EventDispatcher] Remove unneded count() #22237
Conversation
| Q | A |
|---|---|
| Branch? | 2.7 |
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | - |
| License | MIT |
| Doc PR | - |
|
Anything that's not a bug fix should be based on master, not stable branches. |
| @@ -80,7 +80,7 @@ public function getListeners($eventName = null) | |||
| */ | |||
| public function hasListeners($eventName = null) | |||
| { | |||
| return (bool) count($this->getListeners($eventName)); | |||
| return (bool) $this->getListeners($eventName); | |||
There was a problem hiding this comment.
Personally I would prefer to see return 0 !== count($this->getListeners($eventName)); If for some reason, it's decided to return an object instead, it will always return true, as the object is casted to a boolean, and empty objects are also true.
There was a problem hiding this comment.
In that case you would have same problem with count
https://3v4l.org/goLns
Point of this PR is that hasListeners counts number of listeners needlessly. It only needs to know if it's not empty, it doesn't need to count all of them first to determine that.
We can change this to return array() !== $this->getListeners... if you prefer that. Just don't do a count if you don't need it.
There was a problem hiding this comment.
count works on everything that implements \Countable, so if it returns an object while the method name implies a collection of some sorts, I expect it to also be countable.
There was a problem hiding this comment.
If you plan to replace current implementation with collection (which I doubt, therefore premature optimization), at that point it should be replaced with ->isEmpty then, not count. You don't need to know the count. Contract specifies you don't support this use case anyway. If you did, interface for getListeners would specify array|\Countable.
There was a problem hiding this comment.
Note that this can't happen really: that'd be a BC break.
In any case, there is a test case on this method, so this won't happen silently.
|
Thank you @ostrolucky. |
This PR was merged into the 2.7 branch. Discussion ---------- [EventDispatcher] Remove unneded count() | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Commits ------- 9a1915f [EventDispatcher] Remove unneded count()