Skip to content

Promote the warning of array_key_exists() to exception #4887

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 2 commits into from

Conversation

kocsismate
Copy link
Member

@kocsismate kocsismate commented Nov 4, 2019

I am not exactly sure if these promotions fall into the scope of the Engine Warnings RFC (are these errors suppressed commonly?), so feel free to close the PR in this case.

Also, I made two more promotions in the standard/array.c file, but these doesn't seem likely to happen (one of them is in an apparently not used macro and the other should never happen according to its comment).

@nikic
Copy link
Member

nikic commented Nov 4, 2019

See #4572.

@kocsismate
Copy link
Member Author

kocsismate commented Nov 4, 2019

Yes, I should have remembered about this PR (or at least checked for it). :D OK, not doing warning promotions certainly makes sense for count(), but what about array_key_exists()? It seems to be a less severe BC break. I can understand if it is also a no-go.

@nikic
Copy link
Member

nikic commented Nov 4, 2019

@kocsismate Chaning array_key_exists is generally fine, but please make sure it has consistent type behavior with isset($array[$key]) first. I remember there were some discrepancies (maybe with handling of float keys?)

@nikic
Copy link
Member

nikic commented Nov 4, 2019

That is, the argument should not be "string or integer", it should work based on the normal array key coercions that are used everywhere else where array accesses are performed. Only arrays and objects should result in a TypeError.

@kocsismate kocsismate force-pushed the standard-array-exceptions branch from d065f42 to 04d25cd Compare November 5, 2019 00:38
@kocsismate kocsismate changed the title Promote warnings of count() and array_key_exists() to exceptions Promote warnings of array_key_exists() to exceptions Nov 5, 2019
Copy link
Member Author

@kocsismate kocsismate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikic I tried my best to do what you suggested.

@kocsismate kocsismate changed the title Promote warnings of array_key_exists() to exceptions Promote the warning of array_key_exists() to exception Nov 5, 2019
@kocsismate kocsismate force-pushed the standard-array-exceptions branch from 04d25cd to 7e3a206 Compare November 5, 2019 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants