#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: | 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)
Change History (48)
#1
@
7 years ago
- Keywords needs-patch needs-unit-tests added
- Milestone changed from Awaiting Review to 4.9.6
- Version trunk deleted
#2
@
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
@
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.
@
7 years ago
Pretty straight forward, so I did a try. Function signature and docblock is based on the RFC's example.
@
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).
This ticket was mentioned in Slack in #core by desrosj. View the logs.
7 years ago
#7
@
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
@
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.
This ticket was mentioned in Slack in #core by desrosj. View the logs.
7 years ago
#15
@
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
@
7 years ago
- Keywords needs-dev-note added
This and #43619 would be great to mention in a dev note.
#17
@
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
@
7 years ago
- Keywords commit removed
- Resolution fixed deleted
- Status changed from closed to reopened
#19
@
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.
@
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
#21
@
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
andResourceBundle
tests into their own test methods so that they can be marked as skipped if their respective extensions are not loaded. - Change
assertEquals()
toassertSame()
intest_is_countable_functionality()
for stricter type checking.
#22
@
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()
👍
#26
@
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 ?
This ticket was mentioned in Slack in #core by desrosj. View the logs.
7 years ago
#30
follow-up:
↓ 33
@
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
@
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.
#33
in reply to:
↑ 30
@
7 years ago
Replying to jrf:
I suggest adding a PHPUnit
@requires PHP 5.4
annotation to the docblock of thetest_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 :)
This ticket was mentioned in Slack in #core by desrosj. View the logs.
6 years ago
#38
@
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.
Sounds good to me 👍