Opened 6 years ago
Closed 6 years ago
#44123 closed task (blessed) (wontfix)
Utilize `is_countable()` and `is_iterable()` Polyfills
Reported by: | desrosj | Owned by: | |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | 4.9.6 |
Component: | General | Keywords: | |
Focuses: | Cc: |
Description (last modified by )
In 4.9.6, polyfills for is_countable()
and is_iterable()
were introduced (#43583 and #43619). These should be utilized in core for better variable validation and forwards compatibility.
This is a meta ticket to track overall progress making replacements throughout core. Because there will be many instances and each requires validation, individual tickets should be opened for each patch.
If you have reviewed any core files, please add a note in this ticket about which files you have reviewed and reference any tickets with patches that you have opened as a result of your review.
Dev note detailing the new polyfills: https://make.wordpress.org/core/2018/05/17/new-php-polyfills-in-4-9-6/
Using is_countable()
Old Way
if ( count( $var ) > 0 ) { // Do something. }
New Way
if ( is_countable( $var ) && count( $var ) > 0 ) { // Do something. }
Using is_iterable()
Old Way
if ( count( $var ) > 0 ) { foreach( $var as $key => $value) { // Do something. } }
New Way
if ( is_iterable( $var ) ) { foreach( $var as $key => $value) { // Do something. } }
Change History (13)
#2
@
6 years ago
- Description modified (diff)
- Focuses coding-standards removed
Description edits
- Added link to the 4.9.6 dev note.
- Added examples from the dev note.
- Added clarification of the ticket's purpose.
- Remove
coding-standards
focus (this is more "best practices").
#3
@
6 years ago
This may also be a nice task for some people at the WCEU contributor day (only experienced PHP devs as reviewing this is not a beginners task).
#9
follow-up:
↓ 10
@
6 years ago
@thrijith Thank you for your contributions to this! I'll try to review the patches later this week.
As a general - but important - side-note (without having reviewed the patches): is_countable()
should only be added when needed, not blindly to every count()
.
If an array is expected somewhere and the variable being tested is not a parameter being passed in or the output of a apply_filters()
call either in the function in question or in a function from which the output is being used, is_countable()
should not be added as it should not be needed and adding it may hide bugs which should be solved instead.
#10
in reply to:
↑ 9
@
6 years ago
Replying to jrf:
@thrijith Thank you for your contributions to this! I'll try to review the patches later this week.
As a general - but important - side-note (without having reviewed the patches):
is_countable()
should only be added when needed, not blindly to everycount()
.
If an array is expected somewhere and the variable being tested is not a parameter being passed in or the output of aapply_filters()
call either in the function in question or in a function from which the output is being used,is_countable()
should not be added as it should not be needed and adding it may hide bugs which should be solved instead.
Noted. Thanks for the input, I have added comments on tickets where I had my doubts, Please let me know if changes are needed from my end.
#11
@
6 years ago
- Keywords close added; needs-patch removed
This is an important point by @jrf.
The is_countable()
function should only be used before counting a value where it's valid for either a countable or non-countable value to be passed. Avoiding counting non-countable values where only countable values are expected will definitely introduce bugs. The same goes for is_iterable()
and iterable values. See discussion on #42814 and #42860 as examples.
There's an inherent expectation in the hooks and filters API that the return values of filters are of the correct type, and core shouldn't silence warnings about non-iterable values being iterated, or non-countable values being counted, because this can mask bugs introduced by plugins.
PHP warnings are there for a reason and are not something to be avoided at all costs. If a non-countable/iterable value is passed to a piece of code where one is expected, it is correct behaviour for a PHP warning to be triggered.
I think this meta ticket should be closed in favour of individual tickets where it's been demonstrated that core is attempting to count/iterate a value where it's valid for that value to either be countable/iterable or not (which I'd hope is not very common).
#13
@
6 years ago
- Keywords close removed
- Milestone 4.9.8 deleted
- Resolution set to wontfix
- Status changed from new to closed
- Version set to 4.9.6
@johnbillion that works for me.
Moving forward, please use individual tickets to address instances where is_countable()
and is_iterable()
should be utilized being mindful of the ideals above.
Indeed @desrosj, let's work toward getting what can be into 4.9.7 and go from there 👍