WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 13 days ago

Last modified 13 days ago

#43583 closed enhancement (fixed)

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

Reported by: jrf Owned by: SergeyBiryukov
Milestone: 4.9.6 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch has-unit-tests needs-dev-note commit
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 2 months 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 2 months 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 5 weeks ago.
43583.2.patch (2.4 KB) - added by desrosj 5 weeks ago.
Missed one unit test function @ticket tag.
43583-improvements.patch (1.1 KB) - added by ayeshrajans 3 weeks ago.
43583-improvements2.patch (1.1 KB) - added by ayeshrajans 2 weeks 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 2 weeks ago.
43583.2.diff (2.2 KB) - added by desrosj 2 weeks ago.

Download all attachments as: .zip

Change History (43)

#1 @netweb
2 months 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
2 months 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
2 months 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
2 months ago

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

#4 @ayeshrajans
2 months ago

  • Keywords needs-patch removed

@ayeshrajans
2 months 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
2 months 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.


5 weeks ago

@desrosj
5 weeks ago

#7 @ayeshrajans
5 weeks 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
5 weeks 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
5 weeks ago

Awesome! Thanks.

@desrosj
5 weeks ago

Missed one unit test function @ticket tag.

#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
3 weeks ago

In 43034:

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

Props jrf, ayeshrajans, desrosj.
See #43583.

#14 @SergeyBiryukov
3 weeks 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
3 weeks 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
3 weeks ago

  • Keywords needs-dev-note added

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

#17 @ayeshrajans
3 weeks 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
3 weeks ago

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

#19 @jrf
3 weeks 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
2 weeks 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.


2 weeks ago

@desrosj
2 weeks ago

#21 @desrosj
2 weeks 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
2 weeks 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
2 weeks ago

  • Keywords needs-refresh added; commit removed

@desrosj
2 weeks ago

#24 @desrosj
2 weeks ago

  • Keywords needs-refresh removed

Thanks, @jrf! Updated the patch.

#25 @desrosj
2 weeks ago

  • Keywords commit added

#26 @jrf
2 weeks 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 2 weeks ago by jrf (previous) (diff)

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


13 days ago

#28 @SergeyBiryukov
13 days 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
13 days 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
13 days 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
13 days 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
13 days ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#33 in reply to: ↑ 30 @SergeyBiryukov
13 days 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
13 days 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
13 days 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.

Note: See TracTickets for help on using tickets.