WordPress.org

Make WordPress Core

Opened 5 days ago

Last modified 6 hours ago

#44123 new task (blessed)

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

Reported by: desrosj Owned by:
Milestone: 4.9.8 Priority: normal
Severity: normal Version:
Component: General Keywords: close
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 (12)

#1 @netweb
5 days 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
5 days 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
4 days 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
4 days ago

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

#5 @netweb
4 days ago

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

#6 @thrijith
35 hours ago

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

#7 @thrijith
30 hours ago

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

#8 @thrijith
30 hours ago

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

#9 follow-up: @jrf
30 hours 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
29 hours 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
20 hours 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 hours ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

Note: See TracTickets for help on using tickets.