Skip to content

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

Closed
wants to merge 1 commit into from
Closed

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Nov 23, 2019

A new take on #4572 as it was declined due to the prevalence of using count on NULL.
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.

@Girgias Girgias force-pushed the count-errors-v2 branch 3 times, most recently from 1df1a42 to 26d2e35 Compare November 23, 2019 20:22
@Girgias Girgias requested a review from nikic January 20, 2020 03:16
@kocsismate
Copy link
Member

kocsismate commented Jan 21, 2020

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:
if (count($userIds > 0)) {

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.

@Girgias
Copy link
Member Author

Girgias commented Jan 21, 2020

I am not sure if it is really an argument or a counter-argument, but I'd to share this with you. So the other day I found a piece of code in our codebase which says:
if (count($userIds > 0)) {

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 count() on a boolean.

The previous on was closed as using count() like empty() to check for null seems to be pretty prevalent. However the boolean one is new to me :D

[1] https://wiki.php.net/rfc/consistent_type_errors

@kocsismate
Copy link
Member

kocsismate commented Jan 21, 2020

Isn't this already producing a "type error" warning (as of 7.2)?

I haven't seen any warning (but we are not yet on PHP 7.2).

The previous on was closed as using count() like empty() to check for null seems to be pretty prevalent.

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.

@Girgias
Copy link
Member Author

Girgias commented Jan 21, 2020

Isn't this already producing a "type error" warning (as of 7.2)?

I haven't seen any warning (but we are not yet on PHP 7.2).

For reference: https://www.php.net/manual/en/migration72.incompatible.php#migration72.incompatible.warn-on-non-countable-types

The previous on was closed as using count() like empty() to check for null seems to be pretty prevalent.

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.

Makes sense :)

@kocsismate
Copy link
Member

For reference:

What I really wanted to say is that I hadn't seen any warnings in our logs 😅 But the reason is obvious then ^^

@Girgias
Copy link
Member Author

Girgias commented Jan 21, 2020

For reference:

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) :)

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.
@Girgias Girgias closed this Jul 11, 2020
@Girgias Girgias deleted the count-errors-v2 branch July 11, 2020 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants