#42918 closed enhancement (fixed)
Replace intval(), strval(), ... function calls with type casts
Reported by: | ayeshrajans | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 5.6 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | has-patch has-dev-note |
Focuses: | performance | Cc: |
Description
PHP's intval()
, strval()
, floatval()
and boolval()
are from PHP 4 ages. PHP 5 and later has the type hinting pattern (e.g (int) $var
) that is ~6x faster than their function-call counterpart.
There are over 250 such calls, and I'll upload a patch to replace them with type hints. The patch itself is large, but because intval()
and (int)
are identical in usage, I hope you can review it. The test suit passed (https://travis-ci.org/Ayesh/wordpress-develop/builds/317469175).
I have not changed anything in third party libraries (such as random_compat).
Thank you.
Attachments (4)
Change History (20)
@
7 years ago
Please disregard the previous patch. Uploading a new one with proper WP code styling. Thanks to https://github.com/Mahjouba91 for the review and additional fixes.
#2
@
7 years ago
Travis tests for the last attached patch: https://travis-ci.org/Ayesh/wordpress-develop/builds/317731953
#5
@
4 years ago
- Milestone changed from Awaiting Review to 5.6
- Owner set to SergeyBiryukov
- Status changed from new to reviewing
#6
@
4 years ago
- Summary changed from Replace intval(), strval(), ... function calls with type hints to Replace intval(), strval(), ... function calls with type casts
@
4 years ago
Thanks a lot for picking this up. I rebased and added several more cases to the list. Tests: https://travis-ci.com/github/Ayesh/wordpress-develop/builds/184027317 (PHPCS linting fails at the moment), but unit tests pass in all versions.
@
4 years ago
Similar to the last patch, but fixes all phpcs errors as well. Tests (all passing): https://travis-ci.com/github/Ayesh/wordpress-develop/builds/184028745
This ticket was mentioned in Slack in #core by noisysocks. View the logs.
4 years ago
#10
follow-up:
↓ 11
@
4 years ago
Hey @SergeyBiryukov!
r49108 is causing some breakage for me. I think the related code change was unintended.
The Sugar Calendar plugin has used this approach for adding its Top Level menu page for a few years now:
add_menu_page( esc_html__( 'Calendar', 'sugar-calendar' ), esc_html__( 'Calendar', 'sugar-calendar' ), 'read_calendar', 'sugar-calendar', 'Sugar_Calendar\\Admin\\Menu\\calendar_page', 'dashicons-calendar-alt', 2 );
Note the position: 2
. It matches the position of core's "Dashboard" menu item.
- Before this change, WordPress would automatically reassign a float position of
2.06668
and put our "Calendar" item directly below "Dashboard" ✅ - After this change, it gets set to
2
and replaces "Dashboard" 💥
The flaw in r49108 is in add_menu_page()
:
$menu[ "$position" ] = $new_menu;
...was changed to use...
$menu[ $position ] = $new_menu;
Here specifically: https://core.trac.wordpress.org/changeset/49108/trunk/src/wp-admin/includes/plugin.php
PHP doesn't allow floats to be array keys, but it will allow strings to be, and WordPress is (perhaps unintentionally) smart enough to continue to order ints and strings-with-float-contents in the correct numerical sequence.
I'd recommend reverting those lines, perhaps to this to be even more clear that this is intentional:
$menu[ "{$position}" ] = $new_menu;
Thoughts?
#11
in reply to:
↑ 10
@
4 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Replying to johnjamesjacoby:
The flaw in r49108 is in
add_menu_page()
:
$menu[ "$position" ] = $new_menu;...was changed to use...
$menu[ $position ] = $new_menu;Here specifically: https://core.trac.wordpress.org/changeset/49108/trunk/src/wp-admin/includes/plugin.php
PHP doesn't allow floats to be array keys, but it will allow strings to be, and WordPress is (perhaps unintentionally) smart enough to continue to order ints and strings-with-float-contents in the correct numerical sequence.
Good catch, thanks!
I only tested this with integers and numeric strings representing an integer, which produced the same results, as also described in this Stack Overflow thread: A numeric string as array key. But I forgot about floats :)
#13
@
4 years ago
- Keywords needs-dev-note added
Let's mention that this was done for core in the miscellaneous dev note and encourage others to consider making the change in their code.
#16
@
4 years ago
- Keywords has-dev-note added; needs-dev-note removed
Dev note published: https://make.wordpress.org/core/2020/11/23/wordpress-and-php-8-0/
ONe big patch with all changes