WordPress.org

Make WordPress Core

Opened 10 days ago

Last modified 8 hours ago

#47678 assigned enhancement

Modernize/simplify current_user_can()

Reported by: jrf Owned by: pento
Milestone: 5.3 Priority: normal
Severity: normal Version: trunk
Component: Role/Capability Keywords: has-patch has-unit-tests commit needs-dev-note
Focuses: performance, coding-standards Cc:

Description

As support for PHP < 5.6.20 has been dropped, certain modernizations can now be applied to WP.

The `current_user_can()` function is used pretty often during a regular WP page-load and is a good candidate for modernization.

The patch I'm attaching contains this modernization.

To understand what I'm doing here, let's step through it.

Step 1: simplify the function.

The function contains the following redundant code:

<?php
        $args = array_slice( func_get_args(), 1 );
        $args = array_merge( array( $capability ), $args );

What is happening here is that:

  1. All arguments passed to the function are retrieved.
  2. The first argument - the named parameter $capability - is sliced off, so $args is now only the arguments after $capability.
  3. The arguments are merged back together in the same order as they were originally.

I've not done the code archeology to see how this code ever made into the code base as I'm not here to name and shame, but let's get rid of it.

Replacing this part of the code with the below has the exact same effect and removes two (badly performing) function calls:

<?php
        $args = func_get_args();

To prove the point, see: https://3v4l.org/BqYY0

Step 2: modernize the function declaration.

The function currently declares one named, required parameter $capability and via the above mentioned func_get_args() allows for additional arguments to be passed.
This is also documented as such in the function documentation.

<?php
/**
 * ...
 * @param string $capability Capability name.
 * @param mixed  ...$args    Optional further parameters, typically starting with an object ID.
 * @return bool Whether the current user has the given capability. If `$capability` is a meta cap and `$object_id` is
 *              passed, whether the current user has the given meta capability for the given object.
 */
function current_user_can( $capability ) {

PHP 5.6 introduced variadic functions using the spread operator.

Now, we could make $capability variadic and remove the call to func_get_args(), but that would make the $capability parameter optional.

<?php
function current_user_can( ...$capability ) {

Making the $capability parameter optional is not the intention as it:

  • Changes the function signature.
  • Could break the subsequent function call to WP_User::has_cap() to which $capability is passed, where $capability is not optional.

So, instead, we add a new variadic (and therefore by nature optional) parameter $args.

<?php
function current_user_can( $capability, ...$args ) {

This is completely in line with the existing function documentation, so no changes are needed to the docs.

As things stand at this moment, we would still be overwritting $args in the function via the call to func_get_args(), but that's ok for now, we'll get to that in step 3.

Step 3: modernize the return.

Now, let's look at the part of the function which is returning the result:

<?php
        $args = func_get_args();

        return call_user_func_array( array( $current_user, 'has_cap' ), $args );

call_user_func_array() is used here as $args would contain a variable number of arguments and prior to PHP 5.6, there was no other way to pass those on to another function which expected multiple arguments.

However, PHP 5.6 also introduced argument unpacking using the spread operator.

Now suddenly, the call to the badly performing call_user_func_array() is no longer necessary and we can call $current_user->has_cap() directly, unpacking the variadic $args in the function call:

<?php
        return $current_user->has_cap( $capability, ...$args );

So, that's how we get to the actual patch.

All together, this patch gets rid of four redundant function calls, three of which were calls to functions with a bad reputation for performance.

So, even though small, this patch should give WP a very tiny performance boost.

Unit tests

There are numerous tests in the `Tests_User_Capabilities` class covering this change already.

All of these tests still pass after this change.

And once I've uploaded this patch, I will start a Travis run just to make sure and quieten any nerves anyone may have about this patch.

/cc @pento

Attachments (52)

patch-47678.patch (1.2 KB) - added by jrf 10 days ago.
47678-map_meta_cap.patch (880 bytes) - added by jrf 10 days ago.
Use spread operator in map_meta_cap()
47678-current_user_can_for_blog.patch (1.4 KB) - added by jrf 10 days ago.
Simplify & modernize current_user_can_for_blog()
47678-author_can.patch (1.2 KB) - added by jrf 10 days ago.
Simplify & modernize author_can()
47678-user_can.patch (1.2 KB) - added by jrf 10 days ago.
Simplify & modernize user_can()
47678-WP_User-has_cap.patch (1.6 KB) - added by jrf 10 days ago.
Simplify & modernize WP_User::has_cap()
47678-Walker-walk.patch (1.0 KB) - added by jrf 10 days ago.
Use spread operator in Walker::walk()
47678-Walker-paged_walk.patch (1.3 KB) - added by jrf 10 days ago.
Use spread operator in Walker::paged_walk()
47678-add_post_type_support.patch (1.2 KB) - added by jrf 10 days ago.
Simplify & modernize add_post_type_support()
47678-walk_page_dropdown_tree.patch (1.0 KB) - added by jrf 10 days ago.
Simplify & modernize walk_page_dropdown_tree()
47678-add_theme_support.patch (1002 bytes) - added by jrf 10 days ago.
Simplify & modernize add_theme_support()
47678-get_theme_support.patch (1.1 KB) - added by jrf 10 days ago.
Simplify & modernize get_theme_support()
47678-current_theme_supports.patch (1.2 KB) - added by jrf 10 days ago.
Simplify & modernize current_theme_supports()
47678-wp_register_sidebar_widget.patch (1.3 KB) - added by jrf 10 days ago.
Simplify & modernize wp_register_sidebar_widget()
47678-wp_register_widget_control.patch (1.4 KB) - added by jrf 10 days ago.
Simplify & modernize wp_register_widget_control()
47678-_register_widget_update_callback.patch (1.4 KB) - added by jrf 10 days ago.
Simplify & modernize _register_widget_update_callback()
47678-_register_widget_form_callback.patch (1.3 KB) - added by jrf 10 days ago.
Simplify & modernize _register_widget_form_callback()
47678-wpdb-prepare.patch (3.8 KB) - added by jrf 10 days ago.
Simplify & modernize wpdb::prepare(). Includes reformatting the documentation for line length/readibility.
47678-walk_category_tree.patch (1.5 KB) - added by jrf 3 days ago.
Simplify & modernize walk_category_tree() Includes minor documentation fixes.
47678-walk_category_dropdown_tree.patch (1.5 KB) - added by jrf 3 days ago.
Simplify & modernize walk_category_dropdown_tree() Includes minor documentation fixes.
47678-_WP_Dependency-__construct.patch (1.0 KB) - added by jrf 3 days ago.
Use spread operator in _WP_Dependency::construct()
47678-_Upgrader_Skin-feedback.patch (4.2 KB) - added by jrf 3 days ago.
Simplify & modernize *_Upgrader_Skin::feedback()
47678-WP_Ajax_Upgrader_Skin-error.patch (1.6 KB) - added by jrf 3 days ago.
Simplify & modernize WP_Ajax_Upgrader_Skin::error()
47678-do_action.patch (1.5 KB) - added by jrf 3 days ago.
Simplify & modernize do_action()
47678-add_query_args.patch (942 bytes) - added by jrf 3 days ago.
Use spread operator in add_query_args()
47678- code-simplification-apply_filters.patch (1.2 KB) - added by jrf 3 days ago.
Code simplification apply_filters()
47678-unit-test-util-xml_find.patch (1.7 KB) - added by jrf 3 days ago.
Simplify & modernize unit test util xml_find()
47678-some-unit-test-functions.patch (4.1 KB) - added by jrf 3 days ago.
Simplify & modernize some unit test functions
47678-wp_sprintf.patch (1.5 KB) - added by jrf 3 days ago.
Simplify & modernize wp_sprintf()
47678-select-functions-in-wp-includes-deprecated.patch (1.3 KB) - added by jrf 3 days ago.
Simplify & modernize functions in wp-includes/deprecated.php While these functions are deprecated, they can still get a minor performance boost in case they are being called.
47678-functions-in-tools-i18n-not-gettext.patch (1.3 KB) - added by jrf 3 days ago.
Simplify & modernize functions in tools/i18n/not-gettexted.php
47678-wp_dashboard_cached_rss_widget.patch (1.5 KB) - added by jrf 24 hours ago.
Simplify & modernize wp_dashboard_cached_rss_widget()
47678-wp_iframe.patch (1.3 KB) - added by jrf 24 hours ago.
Simplify & modernize wp_iframe()
47678-wp_iframe.2.patch (10 bytes) - added by jrf 24 hours ago.
Ignore/remove
47678-map_meta_cap-part-2.patch (1022 bytes) - added by jrf 9 hours ago.
Use spread operator in map_meta_cap() [2]
47678-WP_Customize_-check_capabilities.patch (3.1 KB) - added by jrf 9 hours ago.
Simplify & modernize WP_Customize_*::check_capabilities()
47678-WP_Customize_Manager-import_theme_starter_.patch (1.0 KB) - added by jrf 9 hours ago.
Modernize WP_Customize_Manager::import_theme_starter_content()
47678-walk_nav_menu_tree.patch (914 bytes) - added by jrf 9 hours ago.
Simplify walk_nav_menu_tree()
47678-walk_page_tree.patch (802 bytes) - added by jrf 9 hours ago.
Simplify walk_page_tree()
47678-wp_ajax_menu_get_metabox.patch (1.4 KB) - added by jrf 9 hours ago.
Simplify wp_ajax_menu_get_metabox()
47678-wp_terms_checklist.patch (1.0 KB) - added by jrf 9 hours ago.
Simplify wp_terms_checklist()
47678-wp_list_widgets-and-associated-unit-tests.patch (2.9 KB) - added by jrf 9 hours ago.
Modernize wp_list_widgets() and associated unit tests ... and improve the readability of the code.
47678-WP_Hook-apply_filters.patch (1.3 KB) - added by jrf 9 hours ago.
Minor simplification of WP_Hook::apply_filters()
47678-WP_Admin_Bar-add_node.patch (1.0 KB) - added by jrf 9 hours ago.
WP_Admin_Bar::add_node(): Remove redundant calls to func_get_arg()
47678-WP_Rewrite-add_endpoint.patch (939 bytes) - added by jrf 9 hours ago.
WP_Rewrite::add_endpoint(): Remove redundant call to func_get_arg()
47678-wp_cron.php.patch (813 bytes) - added by jrf 9 hours ago.
Simplify wp_cron.php
47678-WP_Customize_Widgets-get_widget_control.patch (971 bytes) - added by jrf 9 hours ago.
Simplify WP_Customize_Widgets::get_widget_control()
47678-wp_specialchars.patch (1.0 KB) - added by jrf 9 hours ago.
Simplify wp_specialchars()
47678-Walker-display_element.patch (2.0 KB) - added by jrf 9 hours ago.
Simplify Walker::display_element()
47678-__call.patch (4.8 KB) - added by jrf 9 hours ago.
Simplify & modernize *::call() The callback in these functions is always checked against a limited list of valid callbacks and those are not problematic, so these can be safely changed to dynamic function calls.
47678-more-select-functions-in-wp-includes-deprecated.patch (3.9 KB) - added by jrf 9 hours ago.
Simplify & modernize functions in wp-includes/deprecated.php [2] While these functions are deprecated, they can still get a minor performance boost in case they are being called. For these functions I've verified that they are still being called by > 1000 plugins, so IMO boosting their performance a little is warranted.
47678-select-unit-tests.patch (1.7 KB) - added by jrf 9 hours ago.
Simplify & modernize select unit tests

Download all attachments as: .zip

Change History (83)

@jrf
10 days ago

#2 @pento
10 days ago

  • Keywords commit added; 2nd-opinion removed
  • Milestone changed from Awaiting Review to 5.3
  • Owner set to jrf
  • Status changed from new to assigned

Interestingly, this has existed since nearly the beginning of time.

I suspect that it originated as a copy/pasta bug when the feature was being written: the similar behaviour in WP_User::has_cap() was copied and modified to suit the desired behaviour of current_user_can(). Unfortunately, I haven't been able to locate any relevant discussion around it, so it seems it just slipped under the radar at the time.

Due to the giant "here be dragons" signs all over the roles/capabilities code, it's not really surprising that redundant code like this has quietly existed the entire time, given that it works.

I'm cool with both removing the redundant code, and introducing the spread operator here.

#3 @jrf
10 days ago

👍

This ticket is basically a proof of concept and the same principle can be applied to a lot more functions in WP and in the capabilities.php file in particular.

Should I open separate tickets for each of those patches ? or can I add additional patches, at least for the Capabilities related functions, to this ticket ?

@jrf
10 days ago

Use spread operator in map_meta_cap()

@jrf
10 days ago

Simplify & modernize current_user_can_for_blog()

@jrf
10 days ago

Simplify & modernize author_can()

@jrf
10 days ago

Simplify & modernize user_can()

#4 @jrf
10 days ago

I've added (and will continue to add) a number of additional patches to this ticket which each apply the same principle as applied in the original ticket to other functions.

Each patch is named after the WP function it addresses.

@jrf
10 days ago

Simplify & modernize WP_User::has_cap()

@jrf
10 days ago

Use spread operator in Walker::walk()

@jrf
10 days ago

Use spread operator in Walker::paged_walk()

@jrf
10 days ago

Simplify & modernize add_post_type_support()

@jrf
10 days ago

Simplify & modernize walk_page_dropdown_tree()

@jrf
10 days ago

Simplify & modernize add_theme_support()

@jrf
10 days ago

Simplify & modernize get_theme_support()

@jrf
10 days ago

Simplify & modernize current_theme_supports()

@jrf
10 days ago

Simplify & modernize wp_register_sidebar_widget()

@jrf
10 days ago

Simplify & modernize wp_register_widget_control()

@jrf
10 days ago

Simplify & modernize _register_widget_update_callback()

@jrf
10 days ago

Simplify & modernize _register_widget_form_callback()

@jrf
10 days ago

Simplify & modernize wpdb::prepare(). Includes reformatting the documentation for line length/readibility.

#5 @knutsp
10 days ago

Nice!

array() to [] and it will be perfect.

#6 @swissspidy
10 days ago

Short array syntax is probably best introduced in one swoop in a separate ticket. It doesn't give any advantage over the long syntax anyway. Let's focus on using the spread operator here, which I think makes for a great improvement already!

#7 @jrf
10 days ago

FYI: Including all patches attached to this ticket, the build is still passing: https://travis-ci.com/WordPress/wordpress-develop/builds/118756852

array() to [] and it will be perfect.

@knutsp Short array syntax is "sugar". It makes no functional difference, has no performance benefits and depending on whom you speak to, decreases code readability.

I'm with @swissspidy here. Let's keep this ticket focused on the spread operator which with the implementations in the patches I've submitted so far does actually have performance benefits and gets rid of a lot of unnecessary function call overhead.

Whether short array syntax should be introduced or even allowed, is a whole other discussion.
Also see the existing discussion about this here: https://make.wordpress.org/core/2019/03/26/coding-standards-updates-for-php-5-6/

#8 @Hareesh Pillai
9 days ago

  • Focuses performance added
  • Type changed from defect (bug) to enhancement

#9 @pento
9 days ago

  • Owner changed from jrf to pento

I'm going to work through these patches and get them committed. 🙂

#10 @pento
9 days ago

In 45622:

Code Modernisation: Introduce the spread operator in capabilities.php.

Rather than relying func_get_args() to retrieve arbitrary function arguments, we can now use the spread operator to assign them directly to a variable.

Props jrf.
See #47678.

#11 @pento
9 days ago

In 45623:

Code Modernisation: Introduce the spread operator in WP_User.

Rather than relying func_get_args() to retrieve arbitrary function arguments, we can now use the spread operator to assign them directly to a variable.

Props jrf.
See #47678.

#12 @pento
9 days ago

In 45624:

Code Modernisation: Introduce the spread operator in Walker.

Rather than relying func_get_args() to retrieve arbitrary function arguments, we can now use the spread operator to assign them directly to a variable.

Props jrf.
See #47678.

#13 @pento
9 days ago

In 45625:

Code Modernisation: Introduce the spread operator in add_post_type_support().

Rather than relying func_get_args() to retrieve arbitrary function arguments, we can now use the spread operator to assign them directly to a variable.

Props jrf, pento.
See #47678.

#14 follow-up: @pento
9 days ago

Note: [45625] is slightly different from 47678-add_post_type_support.patch: we don't need to use func_num_args(), as $args() will be set to an empty array if no bonus arguments are passed.

#15 @pento
9 days ago

In 45627:

Code Modernisation: Introduce the spread operator in walk_page_dropdown_tree().

Rather than relying func_get_args() to retrieve arbitrary function arguments, we can now use the spread operator to assign them directly to a variable.

Props jrf.
See #47678.

#16 @pento
9 days ago

In 45628:

Code Modernisation: Introduce the spread operator in theme.php.

Rather than relying func_get_args() to retrieve arbitrary function arguments, we can now use the spread operator to assign them directly to a variable.

Props jrf, pento.
See #47678.

#17 @pento
9 days ago

In 45629:

Code Modernisation: Introduce the spread operator in widgets.php.

Rather than relying func_get_args() to retrieve arbitrary function arguments, we can now use the spread operator to assign them directly to a variable.

Props jrf.
See #47678.

#18 @pento
9 days ago

In 45630:

Code Modernisation: Introduce the spread operator in wpdb::prepare().

Rather than relying func_get_args() to retrieve arbitrary function arguments, we can now use the spread operator to assign them directly to a variable.

Props jrf.
See #47678.

#19 in reply to: ↑ 14 @jrf
9 days ago

@pento Thanks for all the commits so far! I'll try and add the next set of patches tomorrow (my timezone).

Replying to pento:

Note: [45625] is slightly different from 47678-add_post_type_support.patch: we don't need to use func_num_args(), as $args() will be set to an empty array if no bonus arguments are passed.

True enough, though using if ( $args ) isn't actually showing what you are testing for, if ( ! empty( $args ) ) would have been my preference in that case, even if just to prevent the next person scratching their head trying to figure out what's happening ;-)

#20 follow-up: @pento
9 days ago

Technically it's only testing for if ( 0 !== count( $args ) ) ? 😉

I'm okay with the assumed knowledge that the spread operator will ensure that $args will always be an array (which is a little new to the WordPress codebase), so a falsey test of $args is checking if that array is empty or not (which is a very common pattern in the WordPress codebase).

#21 in reply to: ↑ 20 @jrf
9 days ago

Replying to pento:

Technically it's only testing for if ( 0 !== count( $args ) ) ? 😉

I'm okay with the assumed knowledge that the spread operator will ensure that $args will always be an array (which is a little new to the WordPress codebase), so a falsey test of $args is checking if that array is empty or not (which is a very common pattern in the WordPress codebase).

Well, you know me, I'm just not a fan of implicit non-descript loose comparisons ;-)
I've had to debug way too many issues with that: https://phpcheatsheets.com/test/general/

#22 follow-up: @pento
9 days ago

So, we have a bit of a problem, because PHP doesn't do method overloading.

Changing the method signature of Walker::walk() caused an explosion of PHP warnings on WordPress.org when I deployed this change. The code still works as expected, it's just a sub-optimal experience.

Example: https://3v4l.org/2iS9g

#23 in reply to: ↑ 22 ; follow-up: @jrf
9 days ago

Replying to pento:

So, we have a bit of a problem, because PHP doesn't do method overloading.

Changing the method signature of Walker::walk() caused an explosion of PHP warnings on WordPress.org when I deployed this change. The code still works as expected, it's just a sub-optimal experience.

Example: https://3v4l.org/2iS9g

Well, PHP does do method overloading, just expects as a minimum the same (number of) arguments.

There are two choices here:

  1. Revert that particular change.
  2. Update the method signature for the walk() and paged_walk() methods in child classes.

I'm very much in favour of 2. Yes, WP is notorious for its backward-compatibility, but it's not as if other methods have never received new arguments if a functional change warranted this or am I mistaken ?

And while this may not be considered a "functional change", all this does is formalize existing behaviour of the methods which child-methods should already comply with.

In reality the optional variadic additional arguments to these methods are not new, they just hadn't been made explicit in the method signature. In most cases, they already were explicit in the documentation, showing that there was full awareness that this was only not (yet) formalized because of the limitations in PHP before 5.6.

I'm very happy & willing to prepare the patch for [2], which should effectively solve this for Core, though to be honest a quick search did not yield me any classes in Core which overloaded those particular methods (so far).

As for userland code which may extend the Walker class:

  1. I have no clue how often this happens. Clearly something on wp.org does. @Ipstenu would you be able & willing to see how often extends [A-Za-z_]*Walker occurs in plugin-land code ?
  2. A dev-note on Make could alert devs to this early so they can update their code accordingly, though this would "force" userland plugins/themes to drop support for PHP < 5.6 as well.

Note: function calls to these methods should not be affected, it is only classes which extend the Walker class (or any of the other touched classes) and overload the particular method which has changed.

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

#24 in reply to: ↑ 23 ; follow-up: @pento
6 days ago

Replying to jrf:

There are two choices here:

  1. Revert that particular change.
  2. Update the method signature for the walk() and paged_walk() methods in child classes.

I'm very much in favour of 2. Yes, WP is notorious for its backward-compatibility, but it's not as if other methods have never received new arguments if a functional change warranted this or am I mistaken ?

New arguments are added to methods extremely rarely, and offset against the effort required for plugins to release a compatibility fix. The last instance I recall was WP_Customize_Manager::__construct() gaining a parameter in WP 4.7.

There are ~260 instances of extend Walker here, but presumably more in themes, custom plugins, etc.

I think this change could still work, in combination with a dev-note, and an easy pattern for plugin authors to maintain back compat with old versions of WordPress.

For now, I'm going to revert [45624] until we have a plan laid out.

#25 @pento
6 days ago

In 45640:

Code Modernisation: Revert [45624].

Changing the method signatures on Walker causes back compat issues.

See #47678.

#26 in reply to: ↑ 24 @jrf
5 days ago

Replying to pento:

There are ~260 instances of extend Walker here, but presumably more in themes, custom plugins, etc.

I've reviewed the first 100 results and only two of these overload the walk() method.

All the others just overload the start_el(), start_lvl(), end_el() and/or end_lvl() methods, just like the WP Core native child classes do.

A little more specific search yields 26 results out of which 16 are still false positives or tagged version duplicates and only 10 actually 1) extend one of the Walker classes and 2) overload the walk() method.
Those 10 files are in the following 8 plugins:

  • Polylang (2x)
  • Thoughful Comments (1x)
  • Another WordPress Classifieds Plugin (1x)
  • BuddyPress (1x)
  • Extended Categories Widget (2x)
  • Page Menu Widget (1x)
  • WP Sitemanager (1x)
  • Custom Menu Wizard (1x)

So, while it does happen, it is extremely rare (8 plugins out of 80.000+).

I think this change could still work, in combination with a dev-note, and an easy pattern for plugin authors to maintain back compat with old versions of WordPress.

Maintaining back-compat with older versions of WP is not a problem. Overloaded methods are perfectly okay to have more parameters than the parent method. See: https://3v4l.org/P1PaI

The only issue would be that they wouldn't be able to still support older PHP versions. They would have to update their minimum supported PHP version to 5.6+ to use the spread operator.

And while we've now just looked at Walker::walk(), the same principle applies to the other public methods where the signature changed. Though I expect that, for any of those to be overloaded, would be even more rare than the overloading of Walker::walk().

Changed WP Core classes:

  • WP:User::has_cap()
  • Walker::walk() (reverted in [45640])
  • Walker::paged_walk()
  • wpdb::prepare()

And in the WP unit test classes:

  • WP_UnitTestCase_Base::assertQueryTrue()
  • MockAction::filterall()
  • Tests_WP_Customize_Manager::capture_customize_post_value_set_actions()
  • Tests_WP_Hook_Do_Action::_action_callback()

And the tools class:

  • NotGettexted::logmsg()

#27 follow-up: @peterwilsoncc
5 days ago

I've found WPdirectory does a slightly better job of finding code, searching extends [A-Za-z_]*Walker returns the following:

As custom walkers are more common in themes, I'm concerned about breaking BC as themes are less likely to be open-source than plugins. Most sites run a custom theme.

Custom walkers are required for relatively common features, for example adding data attributes and IDs to Walker_Nav_Menu > ul for use with generic JavaScript libraries.

#28 in reply to: ↑ 27 ; follow-up: @jrf
5 days ago

Replying to peterwilsoncc:

I've found WPdirectory does a slightly better job of finding code, searching extends [A-Za-z_]*Walker returns the following:

As custom walkers are more common in themes, I'm concerned about breaking BC as themes are less likely to be open-source than plugins. Most sites run a custom theme.

Custom walkers are required for relatively common features, for example adding data attributes and IDs to Walker_Nav_Menu > ul for use with generic JavaScript libraries.

Hi Peter, nice! I hadn't thought of searching that way.

I appreciate your concerns and take them seriously, so I've done some more research.

I've reviewed the code of the themes with > 5000 installs based on the links you posted, but all of those are false positives, in that - similar to my previous findings - they just overload the start_el(), start_lvl(), end_el() and/or end_lvl() methods, not the walk() or paged_walk() methods.

So, let's look at getting a little more accurate search results and use this regex function (?:paged_)?walk\s*\(\s*\$.

The results are very different then:

  • Themes: It says 11 matches, but is showing only 4 themes, 1 of which is a false positive (not in a class extending a *Walker class)
  • Plugins: It says 277 matches, but is showing only 102 plugins, with ~50% having 0 installs and most of the ones which do have installs, including ACF, being false positives, i.e. in a class not extending a Walker class, but, typically a stand-alone RWMB_Walker_Select_Tree class or a class extending acf_field. In reality only 20 of the plugins with more than 0 installs extends a *Walker class.

So 3 out 5.000+ themes and some 20 plugins out of 80.000+ in the repo.

Of course, this excludes the commercial plugins and themes, as well as private plugins and themes, but all the same, I do think it's safe to assume that the numbers will be roughly the same, which means that less than 0.05% of plugins and themes will run into this issue.

So, while I appreciate that we should be very careful about this, I think that we may be overestimating the impact a bit.

Last edited 5 days ago by jrf (previous) (diff)

#29 in reply to: ↑ 28 @peterwilsoncc
5 days ago

  • Keywords needs-dev-note added

Replying to jrf:

Of course, this excludes the commercial plugins and themes, as well as private plugins and themes, but all the same, I do think it's safe to assume that the numbers will be roughly the same, which means that less than 0.05% of plugins and themes will run into this issue.

So, while I appreciate that we should be very careful about this, I think that we may be overestimating the impact a bit.

Agreed, the common use case is for *_lvl() and *_el() modifications. @pento's suggestion of a dev-note ought to be adequate.

@jrf
3 days ago

Simplify & modernize walk_category_tree() Includes minor documentation fixes.

@jrf
3 days ago

Simplify & modernize walk_category_dropdown_tree() Includes minor documentation fixes.

@jrf
3 days ago

Use spread operator in _WP_Dependency::construct()

@jrf
3 days ago

Simplify & modernize *_Upgrader_Skin::feedback()

@jrf
3 days ago

Simplify & modernize WP_Ajax_Upgrader_Skin::error()

@jrf
3 days ago

Simplify & modernize do_action()

@jrf
3 days ago

Use spread operator in add_query_args()

@jrf
3 days ago

Code simplification apply_filters()

@jrf
3 days ago

Simplify & modernize unit test util xml_find()

@jrf
3 days ago

Simplify & modernize some unit test functions

@jrf
3 days ago

Simplify & modernize wp_sprintf()

@jrf
3 days ago

Simplify & modernize functions in wp-includes/deprecated.php While these functions are deprecated, they can still get a minor performance boost in case they are being called.

@jrf
3 days ago

Simplify & modernize functions in tools/i18n/not-gettexted.php

#30 @jrf
3 days ago

I've just added another set of patches to this ticket.
Passing build which includes all the patches in this ticket which haven't been committed yet: https://travis-ci.com/WordPress/wordpress-develop/builds/119576361

Notes for select patches in the current set

walk_category_tree() and walk_category_dropdown_tree()

Both these functions referred in the documentation to a walk() method in one of the Walker child classes, but as the child classes in WP Core don't overload the walk() method, that link in the documentation resulted in a 404.

I've changed those links now to Walker:walk() so they will resolve properly in the documentation.

Refs:

do_action()

I'm removing the complete code block quoted below.

This is PHP 4 code. In PHP 4, objects were not automatically passed by reference which is the only explanation I can come up for the current way of setting up the $args array.
As of PHP 5, objects are always passed by reference, so what was being done here has not been needed for quite some time.

<?php
        $args = array();
        if ( is_array( $arg ) && 1 == count( $arg ) && isset( $arg[0] ) && is_object( $arg[0] ) ) { // array(&$this)
                $args[] =& $arg[0];
        } else {
                $args[] = $arg;
        }
        for ( $a = 2, $num = func_num_args(); $a < $num; $a++ ) {
                $args[] = func_get_arg( $a );
        }

apply_filters()

Removing the call to func_get_args() would not have any benefits here, but we can remove the duplicate function call.

Review Status

These are the last patches for the review of all func_get_args() calls in the codebase.
For the remaining calls to func_get_args() I didn't see any real benefit in making this change for various reasons.

Based on a review of the function calls to call_user_func_array(), I expect to add yet some more patches to this ticket.

Commit Status

The following patches have not been committed yet:

  • Walker::walk() and Walker::paged_walk() - see discussion above.
  • wp_register_sidebar_widget()
  • The new patches just added.

@jrf
24 hours ago

Simplify & modernize wp_dashboard_cached_rss_widget()

@jrf
24 hours ago

Simplify & modernize wp_iframe()

@jrf
24 hours ago

Ignore/remove

@jrf
9 hours ago

Use spread operator in map_meta_cap() [2]

@jrf
9 hours ago

Simplify & modernize WP_Customize_*::check_capabilities()

@jrf
9 hours ago

Modernize WP_Customize_Manager::import_theme_starter_content()

@jrf
9 hours ago

Simplify walk_nav_menu_tree()

@jrf
9 hours ago

Simplify walk_page_tree()

@jrf
9 hours ago

Simplify wp_ajax_menu_get_metabox()

@jrf
9 hours ago

Simplify wp_terms_checklist()

@jrf
9 hours ago

Modernize wp_list_widgets() and associated unit tests ... and improve the readability of the code.

@jrf
9 hours ago

Minor simplification of WP_Hook::apply_filters()

@jrf
9 hours ago

WP_Admin_Bar::add_node(): Remove redundant calls to func_get_arg()

@jrf
9 hours ago

WP_Rewrite::add_endpoint(): Remove redundant call to func_get_arg()

@jrf
9 hours ago

Simplify wp_cron.php

@jrf
9 hours ago

Simplify WP_Customize_Widgets::get_widget_control()

@jrf
9 hours ago

Simplify wp_specialchars()

@jrf
9 hours ago

Simplify Walker::display_element()

@jrf
9 hours ago

Simplify & modernize *::call() The callback in these functions is always checked against a limited list of valid callbacks and those are not problematic, so these can be safely changed to dynamic function calls.

@jrf
9 hours ago

Simplify & modernize functions in wp-includes/deprecated.php [2] While these functions are deprecated, they can still get a minor performance boost in case they are being called. For these functions I've verified that they are still being called by > 1000 plugins, so IMO boosting their performance a little is warranted.

@jrf
9 hours ago

Simplify & modernize select unit tests

#31 @jrf
8 hours ago

Ok, so I've now also reviewed all uses of call_user_func_array() within WP Core and have uploaded the relevant patches.

These are - as far as I'm concerned - the last patches for this ticket.

Passing build which includes all non-committed patches: https://travis-ci.org/jrfnl/wordpress-develop/builds/561468904

To explain what I've done and the choices I've made - and more importantly, why a lot of function calls to call_user_func_array() have not been changed, there are a couple of things one needs to know.

1. Spread operator versus associative arrays

The spread operator when used for unpacking an array to individual arguments in a function call only works with numerically indexed arrays.

To explain this in code:

<?php
$return = call_user_func_array( $callable, $args );

If $args is always a numerically indexed array, this can be replaced by:

<?php
$return = $callable( ...$args );

If $args is could potentially be an associative array, this would need to be replaced by:

<?php
$return = $callable( ...array_values( $args ) );

Now while $callable( ...array_values( $args ) ) would still potentially be faster than call_user_func_array( $callable, $args ), the benefits are so small that it is rarely worth replacing these.

Conclusion: If there is any doubt over whether or not $args could be an associative array, the code should be left as is.

There is one exception to this: if the $args array was being created using an array_merge(), then using array_values() in combination with the spread operator is most definitely faster, so in a few instances those have been replaced.

2. PHP cross-version compatibility for dynamic calls to callables.

A callable can be defined in a number of different ways:

<?php
// Type 1: Simple callback
$callback = 'my_callback_function';

// Type 2: Static class method call
$callback = array('MyClass', 'myCallbackMethod');

// Type 3: Object method call
$callback = array($obj, 'myCallbackMethod');

// Type 4: Static class method call (As of PHP 5.2.3)
$callback = 'MyClass::myCallbackMethod';

// Type 5: Relative static class method call (As of PHP 5.3.0)
$callback = array('B', 'parent::who');

// Type 6: Objects implementing __invoke can be used as callables (since PHP 5.3)
$callback = $c, $param);

// Type 7: Closures
$callback = function($a) { return $a * 2; };

Source: https://www.php.net/manual/en/language.types.callable.php

is_callable() will return true for all of the above.

So, in that case, you'd think it'd be safe to call each of these dynamically, wouldn't you ?

<?php
// Replace call to `call_user_func( $callback, $param );`
$callback( $param );

// Replace call to `call_user_func_array( $callback, $params );`
$callback( ...$params );

Unfortunately, that is not the case. It will work in 5 out of the 7 cases, but:

  • callbacks to static methods declared as per Type 4 will fail with a fatal error Call to undefined function MyClass::myCallbackMethod() in PHP 5.6, though they will work fine in PHP 7 and above.
  • callbacks to static methods declared as per Type 5 will fail with a (catchable) fatal error Call to undefined method MyClass::parent::myCallbackMethod.

See: https://3v4l.org/C28tc

Conclusion: While the callback declarations of type 4 and type 5 are quite rare, for any call to call_user_func_array() where the $callback is user/plugin/theme defined, the function call to call_user_func_array() should not be changed to a dynamic function call.

3. call_user_func_array() vs call_user_func() vs dynamic function calls

As a rule of thumb for function call performance:

  • A direct function call - function_call() - would be 1.
  • A dynamic function call - $function() - takes ~1.5x as long.
  • A call via call_user_func() takes ~2x as long.
  • And a call via call_user_func_array() takes ~3x as long.

While point 2 explains why not all calls to call_user_func_array() can be replaced by dynamic functions, here I will try to explain why they also can't all be replaced by calls to call_user_func() in combination with the spread operator.

The short of it, is that call_user_func() will always pass by value, while a dynamic function call and a call via call_user_func_array() passes the variables and keeps references intact.

In code:

<?php
// This function expects to always be passed a variable by reference, not a plain value.
function my_callback( &$param ) {}

$callback = 'my_callback';
$a = true;
$b = array( $a );
$c = array( &$a );

//my_callback( true ); // Fatal error: passing a value when reference is expected.
my_callback( $a ); // OK
my_callback( ...$b ); // OK
my_callback( ...$c ); // OK

//$callback( true ); // Fatal error: passing a value when reference is expected.
$callback( $a ); // OK
$callback( ...$b ); // OK
$callback( ...$c ); // OK

call_user_func( $callback, true ); // PHP 7.1+: Warning: Parameter 1 to my_callback() expected to be a reference, value given
call_user_func( $callback, $a ); // PHP 7.1+: Warning: Parameter 1 to my_callback() expected to be a reference, value given
call_user_func( $callback, ...$b ); // PHP 5.6+: Warning: Parameter 1 to my_callback() expected to be a reference, value given
call_user_func( $callback, ...$c ); // PHP 7.0+: Warning: Parameter 1 to my_callback() expected to be a reference, value given

call_user_func_array( $callback, array( true ) ); // PHP 7.0+: Warning: Parameter 1 to my_callback() expected to be a reference, value given
call_user_func_array( $callback, array( $a ) ); // PHP 7.0+: Warning: Parameter 1 to my_callback() expected to be a reference, value given
call_user_func_array( $callback, $b ); // PHP 5.6: Warning: Parameter 1 to my_callback() expected to be a reference, value given
call_user_func_array( $callback, array( &$a ) ); // OK
call_user_func_array( $callback, $c ); // OK

Here's the code on 3v4l to have a play with: https://3v4l.org/AfX8n

This means that, even though call_user_func() in combination with the spread operator performs better than call_user_func_array(), it cannot always be used as a replacement if the function being called expects a variable passed by reference instead of a plain value.

While to be fair, using reference is a not a good idea™ and should be avoided in most cases, we cannot ignore existing uses and while it would be better - over time - for those to be changed, until that time, we need to account for them.

This, in effect, means that, for instance WP_Hooks::apply_filters() can not be simplified further as - even within WP Core -, there are filter functions which expect to receive the $value to be filtered by reference.

Conclusion: When the $callback is user/plugin/theme defined and it is not known whether or not it expects parameters by reference, calls to call_user_func_array() can only safely be changed to dynamic function calls, not to a call to call_user_func() in combination with the spread operator.


Now, in case anyone is thinking "hang on, but I seem to remember that call-time pass by reference was deprecated/removed" ... ?
Yes, it is. Call-time pass by reference was deprecated in PHP 5.3 and removed in PHP 5.4, but is something different than what I'm talking about here.

In code:

<?php
function my_callback( &$param ) {} // <-- This is still fine and is what I'm talking about above.

$a = 1;
my_callback( &$a ); // <- This has been deprecated/removed in PHP 5.3/5.4.

So, the change to do_action() as mentioned in https://core.trac.wordpress.org/ticket/47678#comment:30 is still perfectly valid.


The patches

Having said the above, I've erred on the side of caution and only made those changes which in my estimation were safe to make.
This includes not having made significant changes to functions using an arbitrary $callback even when those functions are never called with a type 4 or 5 callback from within WP or (confirmed by searches) from any plugin or theme.

All of the patches for this ticket which have already been committed, are "safe" from the above described issues.

Of the existing, non-committed patches, there were two which could have run into point 2 of the above explanation. I have replaced these with new versions of those patches.

All the new patches, of course, comply with the above conclusions.

Additional notes

Some patches don't add a spread operator

A number of patches don't add the spread operator, but do remove the call to call_user_func_array(). In none of these cases, the call to call_user_func_array() was necessary in the first place, so I've just gotten rid of them.


P.S.: sometimes I hate PHP.

Note: See TracTickets for help on using tickets.