Make WordPress Core

Opened 7 years ago

Closed 4 years ago

Last modified 4 years ago

#42918 closed enhancement (fixed)

Replace intval(), strval(), ... function calls with type casts

Reported by: ayeshrajans's profile ayeshrajans Owned by: sergeybiryukov's profile 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)

42918-php-typehints.patch (95.5 KB) - added by ayeshrajans 7 years ago.
ONe big patch with all changes
42918-2.patch (91.6 KB) - added by ayeshrajans 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.
42918-php-typehints-2.patch (116.8 KB) - added by ayeshrajans 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.
42918-php-typehints-3.patch (116.6 KB) - added by ayeshrajans 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

Download all attachments as: .zip

Change History (20)

@ayeshrajans
7 years ago

ONe big patch with all changes

#1 @desrosj
7 years ago

  • Focuses performance added
  • Keywords has-patch needs-testing added

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

#3 @pento
6 years ago

  • Version trunk deleted

#4 @sabernhardt
4 years ago

  • Keywords needs-refresh added; has-patch needs-testing removed

#5 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.6
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#6 @johnbillion
4 years ago

  • Summary changed from Replace intval(), strval(), ... function calls with type hints to Replace intval(), strval(), ... function calls with type casts

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

#7 @ayeshrajans
4 years ago

  • Keywords has-patch added; needs-refresh removed

@ayeshrajans
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

#9 @SergeyBiryukov
4 years ago

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

In 49108:

General: Replace older-style PHP type conversion functions with type casts.

This improves performance, readability, and consistency throughout core.

  • intval()(int)
  • strval()(string)
  • floatval()(float)

Props ayeshrajans.
Fixes #42918.

#10 follow-up: @johnjamesjacoby
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?

Last edited 4 years ago by johnjamesjacoby (previous) (diff)

#11 in reply to: ↑ 10 @SergeyBiryukov
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 :)

#12 @ocean90
4 years ago

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

In 49138:

Administration: Restore alternative admin menu position for menu items with the same position value as an existing menu item.

Reverts parts of [49108].

Props johnjamesjacoby.
Fixes #42918.

#13 @desrosj
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 @daisyo
4 years ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.