Skip to content

Promote the warning of array_key_exists() to exception - #4887

Closed
kocsismate wants to merge 2 commits into
php:masterfrom
kocsismate:standard-array-exceptions
Closed

Promote the warning of array_key_exists() to exception#4887
kocsismate wants to merge 2 commits into
php:masterfrom
kocsismate:standard-array-exceptions

Conversation

@kocsismate

@kocsismatekocsismate commented Nov 4, 2019

Copy link
Copy Markdown
Member

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

nikic commented Nov 4, 2019

Copy link
Copy Markdown
Member

See #4572.

@kocsismate

kocsismate commented Nov 4, 2019

Copy link
Copy Markdown
MemberAuthor

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

nikic commented Nov 4, 2019

Copy link
Copy Markdown
Member

@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

nikic commented Nov 4, 2019

Copy link
Copy Markdown
Member

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
kocsismateforce-pushed the standard-array-exceptions branch from d065f42 to 04d25cdCompareNovember 5, 2019 00:38
@kocsismatekocsismate changed the title Promote warnings of count() and array_key_exists() to exceptionsPromote warnings of array_key_exists() to exceptionsNov 5, 2019
Comment threadZend/zend_API.c Outdated

@kocsismatekocsismate left a comment

Copy link
Copy Markdown
MemberAuthor

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.

Comment threadZend/zend_execute.c Outdated
Comment threadext/standard/array.c Outdated
Comment threadext/standard/array.c Outdated
Comment threadext/standard/tests/array/array_key_exists_variation1.phpt Outdated
@kocsismatekocsismate changed the title Promote warnings of array_key_exists() to exceptionsPromote the warning of array_key_exists() to exceptionNov 5, 2019
@kocsismate
kocsismateforce-pushed the standard-array-exceptions branch from 04d25cd to 7e3a206CompareNovember 5, 2019 12:24
Comment threadext/standard/array.c Outdated
Comment threadZend/zend_ast.c
Comment threadext/opcache/jit/zend_jit_helpers.c
Comment threadZend/tests/isset_array.phpt
Sign up for freeto 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

@kocsismate@nikic@carusogabriel