-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add ZPP checks to count() #4940
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
Conversation
1df1a42
to
26d2e35
Compare
a2e1f6f
to
7b6dc7b
Compare
7b6dc7b
to
3a11256
Compare
3a11256
to
30b482f
Compare
I am not sure if it is really an argument or a counter-argument, but I'd like to share this with you. So the other day I found a piece of code in our codebase which says: I am sure that there are tons of such code in the wild... So on one hand, all these code would result in an exception all of the sudden by this PR, but on the other hand, such issues would be almost impossible to commit in the first place. |
Isn't this already producing a "type error" warning (as of 7.2)? The rationale for this PR (and the previous one) is to align its behaviour as per the Consistent Type Error RFC. [1] However it is always good to know that there is surprising code in the wild, because I have no clue what are the expected result of The previous on was closed as using |
I haven't seen any warning (but we are not yet on PHP 7.2).
Having forgotten about your PR, I also tried to promote this warning to an exception in October. ^^ All in all, I am also in favour of promoting this warning, I just wanted to highlight this aspect of the change. |
For reference: https://www.php.net/manual/en/migration72.incompatible.php#migration72.incompatible.warn-on-non-countable-types
Makes sense :) |
What I really wanted to say is that I hadn't seen any warnings in our logs 😅 But the reason is obvious then ^^ |
I know just pointing out the relevant section of the upgrading document (because I wasn't sure myself if it was 7.2) :) |
30b482f
to
f02be57
Compare
f02be57
to
ed90588
Compare
Add ZPP checks for the first argument to ensure it is an array or a Countable object. Due to earlier discussion NULL is special cased and deprecated.
d05a057
to
cbece37
Compare
A new take on #4572 as it was declined due to the prevalence of using
count
onNULL
.This allows to use
NULL
with count but deprecates its usage (which I consider valid as it was a warning in PHP 7.2+).It also adds some checks on the $mode argument.
However I needed to change the test runner again because apparently there is a weird edge case which I cannot reproduce in a test file.