Add missing return types to magic methods #50842
Conversation
wouterj
requested review from
chalasr,
jderusse,
xabbuh and
yceruto
as code owners
wouterj
force-pushed
the
magic-methods-return-types
branch
2 times, most recently
from
463bb02
to
b3dc556
Compare
wouterj
force-pushed
the
magic-methods-return-types
branch
from
b3dc556
to
8cf1e3b
Compare
| @@ -149,11 +149,17 @@ private function sendToElasticsearch(array $records): void | |||
| $this->wait(false); | |||
| } | |||
|
|
|||
| /** | |||
| * @return never | |||
There was a problem hiding this comment.
I'm not a fan of the added never types to sleep+wakeup. To me, this is motivated by the internal implementation, but the implementation shouldn't drive the contract. Since these methods are magic, they're not supposed to be called by any userland code directly. This more tailored type doesn't serve any purpose. Actually, it only prevents potential child classes from providing their own implementation if they'd like to. I don't have a use case in mind, but that's not a reason to close the API here to me.
There was a problem hiding this comment.
I'm also a bit hesitant to add never as a return type, as it blocks the user from implementing any other behavior.
But in this case, I added them because all these sleep+wakeup calls are never to prevent insecure deserialization. I thought it would be a good idea to actually prevent user-code from loosing this requirement, as that could unknowingly make them vulnerable for this type of attacks.
If you still disagree, I'm open to reverting these to array and void again.
There was a problem hiding this comment.
Sure and I understand that. That's on the edge of the open/closed principle, just a bit too much on the closed side to me :)
There was a problem hiding this comment.
I realized that the security argument doesn't stand: implementing __serialize still allows working around never; this adds to "open" side.
nicolas-grekas
force-pushed
the
magic-methods-return-types
branch
from
8cf1e3b
to
3ba730d
Compare
nicolas-grekas
force-pushed
the
magic-methods-return-types
branch
from
3ba730d
to
6b27001
Compare
|
Thank you @wouterj. |
…erj) This PR was merged into the 6.4 branch. Discussion ---------- Add missing return types to magic methods | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | no | Deprecations? | yes | Tickets | - | License | MIT | Doc PR | - I promise, these 72 methods really are the last methods that did not have a return type already 🙃 Commits ------- 6b27001 Add missing return types to magic methods
I promise, these 72 methods really are the last methods that did not have a return type already 🙃