Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#43583 closed enhancement (fixed)

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

Reported by: jrf's profile jrf Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 4.9.6 Priority: normal
Severity: normal Version:
Component: General Keywords: php73 has-patch has-unit-tests has-dev-note
Focuses: Cc:

Description

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

PHP 7.3 will introduce a new function called is_countable() which will basically check:

if (is_array($foo) || $foo instanceof Countable) {
    // $foo is countable
}

See: https://wiki.php.net/rfc/is-countable
Note: the RFC has passed the vote and the function has already been implemented and merged into PHP Core.

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

if ( ! function_exists( 'is_countable' ) ) :
        function is_countable( $var ) {
                return ( is_array( $var ) || $var instanceof Countable );
        }
}
endif;

This seems like low hanging fruit. Both the `instanceof` operator as well as the `Countable` 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 ) {
        // Do something.
}

can then be replaced by:

if ( is_countable( $var ) && count( $var ) > 0 ) {
        // Do something.
}

This would prevent new bugs from being introduced by people "fixing" these issues using an extra check with empty() which I've seen being suggested a couple of times now and which really is not a good idea as empty() will also return false when a non-zero integer, a boolean true or a non-zero string is passed. All of which would still result in the PHP 7.2 warning being thrown.

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):
#43374, #43368, #43312, #43216, #43201, #43216, #43201, #42860, #42814, #42147, #42498

Attachments (8)

is_countable-polyfill-43583.patch (828 bytes) - added by ayeshrajans 7 years ago.
Pretty straight forward, so I did a try. Function signature and docblock is based on the RFC's example.
is_countable-polyfill-tests-43583.patch (1.6 KB) - added by ayeshrajans 7 years ago.
Adding tests to complement the compat. Majority of the tests are taken from the is_countable tests from php-src itself (https://github.com/php/php-src/pull/3026/commits/587fcc504f8ad2b07ac28c3335cd0fe3ac39b503).
43583.patch (2.4 KB) - added by desrosj 7 years ago.
43583.2.patch (2.4 KB) - added by desrosj 7 years ago.
Missed one unit test function @ticket tag.
43583-improvements.patch (1.1 KB) - added by ayeshrajans 7 years ago.
43583-improvements2.patch (1.1 KB) - added by ayeshrajans 7 years ago.
Thanks for the suggestions. Appreciate if you could review the updated one. Please note that if xml or intl extensions are not loaded, this test will issue a warning. Ideally, this test should split for these edge cases and skip the test in PHPUnit if the classes are not available.
43583.diff (2.1 KB) - added by desrosj 7 years ago.
43583.2.diff (2.2 KB) - added by desrosj 7 years ago.

Download all attachments as: .zip

Change History (48)

#1 @netweb
7 years ago

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

Sounds good to me 👍

#2 @dd32
7 years ago

This also gets a +1 from me, I'm also good with it being in a 4.9.x release, as making it easier to avoid PHP 7.2 notices and using modern functions to do so seems advantageous to everyone.

#3 @jrf
7 years ago

If nobody picks this up in the mean time, I'll try and get a patch + unit tests sorted at the WC Rotterdam contributors day this Friday.

@ayeshrajans
7 years ago

Pretty straight forward, so I did a try. Function signature and docblock is based on the RFC's example.

#4 @ayeshrajans
7 years ago

  • Keywords needs-patch removed

@ayeshrajans
7 years ago

Adding tests to complement the compat. Majority of the tests are taken from the is_countable tests from php-src itself (https://github.com/php/php-src/pull/3026/commits/587fcc504f8ad2b07ac28c3335cd0fe3ac39b503).

#5 @ayeshrajans
7 years ago

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

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


7 years ago

@desrosj
7 years ago

#7 @ayeshrajans
7 years ago

@desrosj isn't the last patch a combination of the 2 patches before it? A combined patch sure helps it I just wanted to make sure of you wanted to changed something.

#8 @desrosj
7 years ago

This is looking good to me. I made a few adjustments in 43583.patch:

  • Combined the two patches above into one patch.
  • Switched to brackets instead of the alternative syntax.
  • Added @since tag to the inline documentation.
  • Added the @ticket tag to the unit test inline documentation.
  • Changed booleans in the documentation to lowercase to match core.

#9 @ayeshrajans
7 years ago

Awesome! Thanks.

@desrosj
7 years ago

Missed one unit test function @ticket tag.

#10 @desrosj
7 years ago

  • Keywords commit added

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


7 years ago

#12 @SergeyBiryukov
7 years ago

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

#13 @SergeyBiryukov
7 years ago

In 43034:

General: Introduce a polyfill for is_countable() function added in PHP 7.3.

Props jrf, ayeshrajans, desrosj.
See #43583.

#14 @SergeyBiryukov
7 years ago

In 43035:

General: Introduce a polyfill for is_countable() function added in PHP 7.3.

Props jrf, ayeshrajans, desrosj.
Merges [43034] to the 4.9 branch.
See #43583.

#15 @SergeyBiryukov
7 years ago

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

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

#16 @desrosj
7 years ago

  • Keywords needs-dev-note added

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

#17 @ayeshrajans
7 years ago

  • Keywords dev-feedback added

I maintain a standalone PHP polyfill for the is_countable function, and Craig, who was involved in the PHP RFC to bring this function to PHP 7.3 opened a PR there about an edge-case: https://github.com/Ayesh/is_countable-polyfill/pull/4

Turns out PHP classes added by extensions can internally implement a count_elements handler that allows PHP engine to successfully return a value without triggerring any warnings, which makes all objects from that class are countable.

I tested this with a compiled php-src master.

The attached patch updates the is_countable compat code and expand its test coverage for this edge-case. As of now, SimpleXMLElement and ResourceBundle from Intl extension are the only classes that implement this handler. The patch is loosely based on https://github.com/Ayesh/is_countable-polyfill/commit/d8c20caa4abd8ef8de5128d3a62d4ee385f51174.

#18 @ocean90
7 years ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

#19 @jrf
7 years ago

  • Keywords needs-refresh added; dev-feedback removed

@ayeshrajans Thank you for this additional input and the updated patch.

I've just read up on the latest discussion threads about this in php-src and other polyfills and agree this case should be covered by the WP compat polyfill.

While hard-coding the class names is not pretty, it does seem the only viable way to cover this properly at this time.

Regarding the updated patch:

  • The is_object() check is superfluous.
  • The \ to indicate the global namespace for the interface/class names needs to be removed. WP still supports PHP 5.2 and this would cause a parse error for PHP 5.2.
  • Making this a proper multi-line return with one condition on each line seems prudent for readability as the line is getting quite long.
  • I'd like to suggest adding an additional unit test covering ResourceBundle as this is currently not covered by any test.

https://github.com/symfony/polyfill/pull/119/files looks like a quite clean version of what I'm suggesting.

@ayeshrajans
7 years ago

Thanks for the suggestions. Appreciate if you could review the updated one. Please note that if xml or intl extensions are not loaded, this test will issue a warning. Ideally, this test should split for these edge cases and skip the test in PHPUnit if the classes are not available.

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


7 years ago

@desrosj
7 years ago

#21 @desrosj
7 years ago

  • Keywords commit added; needs-refresh removed

In 43583.diff:

  • Fix alignment of new conditional statements.
  • Added parameter inline documentation for test_is_countable_functionality().
  • Split out SimpleXMLElement and ResourceBundle tests into their own test methods so that they can be marked as skipped if their respective extensions are not loaded.
  • Change assertEquals() to assertSame() in test_is_countable_functionality() for stricter type checking.

#22 @jrf
7 years ago

@desrosj Thanks for the follow-up patch. Looking good.

A couple of small remarks:

Fix alignment of new conditional statements.

It looks like the follow on conditions have a unintentional space precision alignment (which should be removed). Alignment should be three tabs and it looks like the current patch has three tabs + one space.

Added parameter inline documentation for test_is_countable_functionality().

These should use @param instead of @type. @type is only used when specifying the details for individual keys in an array return.

Otherwise 👍.

Split out SimpleXMLElement and ResourceBundle tests into their own test methods so that they can be marked as skipped if their respective extensions are not loaded.

👍

May I suggest moving these two new test functions to below the data provider for the test_is_countable_functionality() function ? That way the test + data provider stay together, making them easier to find/connect for other devs looking at the tests.

Change assertEquals() to assertSame()

👍

#23 @jrf
7 years ago

  • Keywords needs-refresh added; commit removed

@desrosj
7 years ago

#24 @desrosj
7 years ago

  • Keywords needs-refresh removed

Thanks, @jrf! Updated the patch.

#25 @desrosj
7 years ago

  • Keywords commit added

#26 @jrf
7 years ago

@desrosj Thanks! this is great.

For the committer: just one little nitpick left: There should be a blank line after the test_is_countable_SimpleXMLElement() function and before the next function docblock (line 263/264). Could you take care of that ?

Last edited 7 years ago by jrf (previous) (diff)

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


7 years ago

#28 @SergeyBiryukov
7 years ago

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

In 43220:

General: In the is_countable() polyfill, if the provided object implements SimpleXMLElement or ResourceBundle, consider it countable.

Props ayeshrajans, jrf, desrosj.
Fixes #43583.

#29 @SergeyBiryukov
7 years ago

In 43221:

General: In the is_countable() polyfill, if the provided object implements SimpleXMLElement or ResourceBundle, consider it countable.

Props ayeshrajans, jrf, desrosj.
Merges [43220] to the 4.9 branch.
Fixes #43583.

#30 follow-up: @jrf
7 years ago

The Travis build is failing for PHP 5.3: https://travis-ci.org/WordPress/wordpress-develop/jobs/377390901#L670-L675

ResourceBundle was added in PHP 5.3.2, but I know the PHP 5.4 release(s) contained a lot of improvements to the implementation and integration of the Intl extension.
To verify what's going on here, someone would need to digging into PHP core source deeply to check the implementation details of ResourceBundle between 5.3 vs 5.4.

To be fair, as PHP 5.3 is ancient, I think that would be over the top and wasting people's time.

I suggest adding a PHPUnit @requires PHP 5.4 annotation to the docblock of the test_is_countable_ResourceBundle() unit test to just skip the test on PHP 5.3.

The fact that for ResourceBundle the Polyfill does not work the same as the PHP 7.2 function on PHP 5.3 is something I'm not to concerned about as it's such an edge case.

Opinions ?

#31 @ayeshrajans
7 years ago

Thumbs up for skipping the test in pre-5.4 PHP versions and calling it a day. The polyfill simply returns true if the object is of class ResourceBundle and doesn't inspect anything deeper. Until PHP provides means to inspect the internal class implementations for count_elements hook, is_countable polyfills won't be perfect anyway.

#32 @SergeyBiryukov
7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#33 in reply to: ↑ 30 @SergeyBiryukov
7 years ago

Replying to jrf:

I suggest adding a PHPUnit @requires PHP 5.4 annotation to the docblock of the test_is_countable_ResourceBundle() unit test to just skip the test on PHP 5.3.

Looks like we don't use the @requires annotation anywhere else in the suite, and check PHP_VERSION instead. Going to do the latter here as well for consistency. Would be great look into any previous discussions on this when I have a bit more time :)

#34 @SergeyBiryukov
7 years ago

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

In 43226:

General: Skip test_is_countable_ResourceBundle() on PHP 5.3 and below.

ResourceBundle is only countable in PHP 5.4+, which can be considered an acceptable edge case for WordPress core purposes.

Props jrf, ayeshrajans.
Fixes #43583.

#35 @SergeyBiryukov
7 years ago

In 43227:

General: Skip test_is_countable_ResourceBundle() on PHP 5.3 and below.

ResourceBundle is only countable in PHP 5.4+, which can be considered an acceptable edge case for WordPress core purposes.

Props jrf, ayeshrajans.
Merges [43226] to the 4.9 branch.
Fixes #43583.

#36 @desrosj
6 years ago

  • Keywords php73 added

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


6 years ago

#38 @johnjamesjacoby
6 years ago

Cross-commenting from #43619

Motion to reopen for future discussion.

Should we backport those to earlier WordPress versions?

I can think of plugins that would like to be PHP7.3 ready without the need to bump their minimum WordPress version to 4.9.6. (I know a few issues were fixed in core, and I’d argue we should backport those also.)

Without backporting these in core, plugins need to implement their own back-fills until they’re ready to bump their minimum version, defeating the purpose of the core back-fills for everything but core.

#39 @desrosj
6 years ago

  • Keywords has-dev-note added; needs-dev-note commit removed

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


5 years ago

Note: See TracTickets for help on using tickets.