WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 4 weeks ago

Last modified 3 weeks ago

#43619 closed enhancement (fixed)

Introduce new PHP cross-version compat function `is_iterable()`

Reported by: jrf Owned by: SergeyBiryukov
Milestone: 4.9.6 Priority: normal
Severity: normal Version: trunk
Component: General Keywords: has-patch has-unit-tests commit needs-dev-note
Focuses: Cc:

Description

Similar to and related to #43583

PHP 7.2 introduced a warning when count() is used on something non-countable.

PHP 7.1 introduced a new function called is_iterable() which basically checks:

if (is_array($foo) || $foo instanceof Traversable) {
    // $foo is traversable
}

See: http://php.net/manual/en/function.is-iterable.php

When count() is used to check if an array is traverable, i.e. usable in a foreach(){}, it would be better to replace the count() check with a call to is_iterable().

To that end, I would like to suggest adding this new function to the wp-includes/compat.php file, like so:

if ( ! function_exists( 'is_iterable' ) ) :
        function is_iterable( $var ) {
                return ( ( is_array( $var ) && ! empty( $var ) || $var instanceof Traversable );
        }
}
endif;

This seems like low hanging fruit. Both the instanceof operator as well as the Traversable interface were already available in PHP 5.2.4, so there should be no cross-version compatibility issues with introducing this function.

The function would provide an easy helper function to help fix the various issues where the warning is currently being reported - both for Core as well as for plugin/theme devs -.

Existing code like this:

if ( count( $var ) > 0 ) {
        foreach( $var as $key => $value) {
                // Do something.
        }
}

can then be replaced by:

if ( is_iterable( $var ) ) {
        foreach( $var as $key => $value) {
                // Do something.
        }
}

The function would also still allow for code refactoring from arrays to objects. /cc @schlessera

I would suggest introducing this function ASAP and not to wait for a major release.

Related issues (there are probably more, but a quick search yielded these): #43583, #43374, #43368, #43312, #43216, #43201, #43216, #43201, #42860, #42814, #42147, #42498

Attachments (3)

43619.diff (1.7 KB) - added by schlessera 2 months ago.
Polyfill implementation with basic unit tests
43619.2.diff (1.8 KB) - added by desrosj 5 weeks ago.
43619.3.diff (1.8 KB) - added by desrosj 5 weeks ago.

Download all attachments as: .zip

Change History (19)

#1 @schlessera
2 months ago

Related to this & #43583: Considering the growing need to add PHP polyfills and other compat code to Core (due to the fact that PHP has a higher velocity than WordPress), we should look into using a more systematic approach to compatibility code come 5.0, like described in #42264

#2 @jrf
2 months ago

@schlessera I hadn't seen that ticket, thanks for pointing it out. I totally agree with the approach. All the same, the need to make backfills load in a more modular manner is independent of the need for the functionality proposed here to be added.

Last edited 2 months ago by jrf (previous) (diff)

#3 @schlessera
2 months ago

@jrf Yes, of course, the functionality proposed here can and should already go in with a next minor release. I just thought your proposed polyfills demonstrate the growing need for a more systematic approach, that's why I linked.

Using functions like the above makes the code more future-proof and less error-prone, so there's no reason not to add this small polyfill.

#4 @netweb
2 months ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.9.6

@schlessera
2 months ago

Polyfill implementation with basic unit tests

#5 @schlessera
2 months ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

I added a patch with a polyfill implementation and some basic unit tests that are taken from the PHP source (except for the Generator test): https://github.com/php/php-src/blob/6053987bc27e8dede37f437193a5cad448f99bce/ext/standard/tests/general_functions/is_iterable.phpt

This ticket was mentioned in Slack in #core by desrosj. View the logs.


5 weeks ago

@desrosj
5 weeks ago

#7 @desrosj
5 weeks ago

  • Keywords commit added

43619.2.diff changes the @since version to 4.9.6 and adds @ticket tags to the unit tests. Everything looks good to go for this one.

#8 @jrf
5 weeks ago

  • Keywords commit removed

I think we still need one more change here - mea culpa.

When I created the ticket I did something (not so) "clever". I added && ! empty( $var ) to the conditional in the function

<?php
return ( ( is_array( $var ) && ! empty( $var ) || $var instanceof Traversable );

However, the PHP native function does not do such a check, so as it stands, there will be situations where the WP back-fill does not give the same result as the PHP native function which is undesirable and will be the cause of bugs in the long run.

Sorry about that.

The latest patch contains the && ! empty( $var ), so it will need to be removed and if no unit tests are failing after that, we need to add a unit test for an empty array.

Last edited 3 weeks ago by jrf (previous) (diff)

@desrosj
5 weeks ago

#9 @desrosj
5 weeks ago

Thanks for pointing that out, @jrf. I updated the patch and added a test for an empty array in 43619.3.diff.

Did you have a plan in mind for updating Core to use this new function? There are several tickets referenced above, but wondering if making a singular ticket (or handling that on this ticket) is better. Same for #43583.

Since these produce warnings, I think these should be fixed in 4.9.6, if possible.

#10 @desrosj
5 weeks ago

  • Keywords commit added

This ticket was mentioned in Slack in #core by desrosj. View the logs.


4 weeks ago

#12 @SergeyBiryukov
4 weeks ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#13 @SergeyBiryukov
4 weeks ago

In 43036:

General: Introduce a polyfill for is_iterable() function added in PHP 7.1.

Props jrf, schlessera, desrosj.
See #43619.

#14 @SergeyBiryukov
4 weeks ago

In 43037:

General: Introduce a polyfill for is_iterable() function added in PHP 7.1.

Props jrf, schlessera, desrosj.
Merges [43036] to the 4.9 branch.
See #43619.

#15 @SergeyBiryukov
4 weeks ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

Let's create a new ticket for using is_iterable() throughout core where appropriate.

#16 @desrosj
3 weeks ago

  • Keywords needs-dev-note added

This and #43583 would be great to mention in a dev note.

Note: See TracTickets for help on using tickets.