Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#44123 closed task (blessed) (wontfix)

Utilize `is_countable()` and `is_iterable()` Polyfills

Reported by: desrosj's profile desrosj Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.9.6
Component: General Keywords:
Focuses: Cc:

Description (last modified by desrosj)

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)

#1 @netweb
6 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.9.7

Indeed @desrosj, let's work toward getting what can be into 4.9.7 and go from there 👍

#2 @desrosj
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 @jrf
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).

#4 @ianbelanger
6 years ago

Added is_countable() to /wp-admin/custom-header.php file, ticket #44124

#5 @netweb
6 years ago

Related: #44124 - Adding is_countable() check to custom-header.php

#6 @thrijith
6 years ago

Ticket #44162, added is_countable() to wp-admin/edit-form-advanced.php.

#7 @thrijith
6 years ago

Ticket #44167, added is_countable() to wp-admin/edit.php.

#8 @thrijith
6 years ago

Ticket #44168, added is_countable() to wp-admin/includes/ajax-actions.php.

#9 follow-up: @jrf
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 @thrijith
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 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.

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 @johnbillion
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).

#12 @desrosj
6 years ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

#13 @desrosj
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.

Note: See TracTickets for help on using tickets.