Modernize/simplify current_user_can()

[jrf]

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

[jrf]

Travis is building:

• ​https://github.com/WordPress/wordpress-develop/pull/73
• ​https://travis-ci.com/WordPress/wordpress-develop/builds/118721067

[pento]

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.

[jrf]

👍

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]

Use spread operator in map_meta_cap()

[jrf]

Simplify & modernize current_user_can_for_blog()

[jrf]

Simplify & modernize author_can()

[jrf]

Simplify & modernize user_can()

[jrf]

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]

Simplify & modernize WP_User::has_cap()

[jrf]

Use spread operator in Walker::walk()

[jrf]

Use spread operator in Walker::paged_walk()

[jrf]

Simplify & modernize add_post_type_support()

[jrf]

Simplify & modernize walk_page_dropdown_tree()

[jrf]

Simplify & modernize add_theme_support()

[jrf]

Simplify & modernize get_theme_support()

[jrf]

Simplify & modernize current_theme_supports()

[jrf]

Simplify & modernize wp_register_sidebar_widget()

[jrf]

Simplify & modernize wp_register_widget_control()

[jrf]

Simplify & modernize _register_widget_update_callback()

[jrf]

Simplify & modernize _register_widget_form_callback()

[jrf]

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

[knutsp]

Nice!

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

[swissspidy]

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!

[jrf]

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/

[pento]

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

[pento]

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.

[pento]

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.

[pento]

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.

[pento]

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.

[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.

[pento]

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.

[pento]

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.

[pento]

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.

[pento]

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.

[jrf]

@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 ;-)

[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).

[jrf]

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/

[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

[jrf]

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.

[pento]

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.

[pento]

In 45640:

Code Modernisation: Revert [45624].

Changing the method signatures on Walker causes back compat issues.

See #47678.

[jrf]

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.

• ​Polylang
• ​Thoughtful Comments

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()

[peterwilsoncc]

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

• Themes: ​1496 matches
• Plugins: ​1127 matches

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.

[jrf]

Replying to peterwilsoncc:

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

• Themes: ​1496 matches
• Plugins: ​1127 matches

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.

[peterwilsoncc]

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]

Simplify & modernize walk_category_tree() Includes minor documentation fixes.

[jrf]

Simplify & modernize walk_category_dropdown_tree() Includes minor documentation fixes.

[jrf]

Use spread operator in _WP_Dependency::construct()

[jrf]

Simplify & modernize *_Upgrader_Skin::feedback()

[jrf]

Simplify & modernize WP_Ajax_Upgrader_Skin::error()

[jrf]

Simplify & modernize do_action()

[jrf]

Use spread operator in add_query_args()

[jrf]

Code simplification apply_filters()

[jrf]

Simplify & modernize unit test util xml_find()

[jrf]

Simplify & modernize some unit test functions

[jrf]

Simplify & modernize wp_sprintf()

[jrf]

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]

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

[jrf]

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:

• ​https://developer.wordpress.org/reference/functions/walk_category_tree/
• ​https://developer.wordpress.org/reference/functions/walk_category_dropdown_tree/

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]

Simplify & modernize wp_dashboard_cached_rss_widget()

[jrf]

Simplify & modernize wp_iframe()

[jrf]

Ignore/remove

[jrf]

Use spread operator in map_meta_cap() [2]

[jrf]

Simplify & modernize WP_Customize_*::check_capabilities()

[jrf]

Modernize WP_Customize_Manager::import_theme_starter_content()

[jrf]

Simplify walk_nav_menu_tree()

[jrf]

Simplify walk_page_tree()

[jrf]

Simplify wp_ajax_menu_get_metabox()

[jrf]

Simplify wp_terms_checklist()

[jrf]

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

[jrf]

Minor simplification of WP_Hook::apply_filters()

[jrf]

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

[jrf]

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

[jrf]

Simplify wp_cron.php

[jrf]

Simplify WP_Customize_Widgets::get_widget_control()

[jrf]

Simplify wp_specialchars()

[jrf]

Simplify Walker::display_element()

[jrf]

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]

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]

Simplify & modernize select unit tests

[jrf]

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.

[jrf]

Just checking in: what can I do to move this ticket forward ?

The first 15 patches were committed (with one being reverted - see discussion above, this revert should probably be reconsidered). The other 37(!) patches have had no action so far.

[SergeyBiryukov]

In 46122:

Code Modernisation: Use the spread operator in wp_register_sidebar_widget().

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.

Missed in [45629].

Props jrf.
See #47678.

[SergeyBiryukov]

In 46123:

Code Modernisation: Introduce the spread operator in wp-includes/category-template.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.

[SergeyBiryukov]

In 46124:

Code Modernisation: Introduce the spread operator in wp-includes/class-wp-dependency.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.

[SergeyBiryukov]

In 46125:

Code Modernisation: Introduce the spread operator in wp-admin/includes/class-*-upgrader-skin.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.

[SergeyBiryukov]

In 46126:

Code Modernisation: Introduce the spread operator in wp-includes/functions.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.

[SergeyBiryukov]

In 46127:

Code Modernisation: Introduce the spread operator in tests/phpunit/*.

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.

[SergeyBiryukov]

In 46128:

Code Modernisation: Introduce the spread operator in wp-includes/formatting.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.

[SergeyBiryukov]

In 46129:

Code Modernisation: Introduce the spread operator in wp-includes/deprecated.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.

While these functions are deprecated, they can still get a minor performance boost in case they are being called.

Props jrf.
See #47678.

[SergeyBiryukov]

In 46130:

Code Modernisation: Introduce the spread operator in wp-admin/includes/dashboard.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.

[SergeyBiryukov]

In 46131:

Code Modernisation: Introduce the spread operator in wp-admin/includes/media.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.

[SergeyBiryukov]

In 46132:

Code Modernisation: Replace call_user_func_array() in wp-includes/capabilities.php with a direct function call.

Props jrf.
See #47678.

[SergeyBiryukov]

In 46133:

Code Modernisation: Replace call_user_func_array() in wp-includes/class-wp-customize-*.php with direct function calls in combination with the spread operator.

Props jrf.
See #47678.

[SergeyBiryukov]

In 46134:

Code Modernisation: Replace call_user_func_array() in wp-includes/nav-menu-template.php with a dynamic function call.

Props jrf.
See #47678.

[SergeyBiryukov]

In 46135:

Code Modernisation: Replace call_user_func_array() in wp-includes/post-template.php with a dynamic function call.

Props jrf.
See #47678.

[SergeyBiryukov]

In 46136:

Code Modernisation: Replace call_user_func_array() in wp-admin/includes/ajax-actions.php with a dynamic function call.

Props jrf.
See #47678.

[SergeyBiryukov]

In 46137:

Code Modernisation: Replace call_user_func_array() in wp-admin/includes/template.php with a dynamic function call.

Props jrf.
See #47678.

[SergeyBiryukov]

In 46138:

Code Modernisation: Replace call_user_func_array() in wp-admin/includes/widgets.php and associated unit tests with a direct function call.

Props jrf.
See #47678.

[SergeyBiryukov]

In 46139:

Code Modernisation: Replace call_user_func_array() in combination with an empty array in wp-includes/class-wp-hook.php with call_user_func().

Props jrf.
See #47678.

[SergeyBiryukov]

In 46140:

Code Modernisation: Remove redundant calls to func_get_arg() in wp-includes/class-wp-admin-bar.php.

Props jrf.
See #47678.

[SergeyBiryukov]

In 46141:

Code Modernisation: Remove redundant call to func_get_arg() in wp-includes/class-wp-rewrite.php.

Props jrf.
See #47678.

[SergeyBiryukov]

In 46142:

Code Modernisation: Replace call_user_func_array() in wp-cron.php with a direct function call.

Props jrf.
See #47678.

[SergeyBiryukov]

In 46143:

Code Modernisation: Replace call_user_func_array() in wp-includes/class-wp-walker.php with dynamic function calls.

Props jrf.
See #47678.

[SergeyBiryukov]

In 46144:

Code Modernisation: Replace call_user_func_array() in various __call() methods with dynamic function calls.

The callback in these functions is always checked against a limited list of valid callbacks that can be safely changed to dynamic function calls.

Props jrf.
See #47678.

[SergeyBiryukov]

In 46145:

Code Modernisation: Replace call_user_func_array() in tests/phpunit/tests/db.php with dynamic function calls.

Props jrf.
See #47678.

[SergeyBiryukov]

In 46146:

Code Modernisation: Simplify some logic in apply_filters().

Props jrf.
See #47678.

[SergeyBiryukov]

Replying to jrf:

Just checking in: what can I do to move this ticket forward ?

The first 15 patches were committed (with one being reverted - see discussion above, this revert should probably be reconsidered). The other 37(!) patches have had no action so far.

Thank you for all the work on this!

The commits above should cover everything except for 47678-do_action.patch​, which causes a fatal error on older PHP versions:

Parse error: syntax error, unexpected '.', expecting '&' or T_VARIABLE in wp-includes/plugin.php on line 442

instead of displaying a proper error message:

Your server is running PHP version 5.2.17 but WordPress 5.3-alpha-45282-src requires at least 5.6.20.

I think it's still preferable to show a proper error message on PHP < 5.6, at least until a switch to PHP 7.x is made.

I'd like to remove this PHP 4 code though:

$args = array();
if ( is_array( $arg ) && 1 == count( $arg ) && isset( $arg[0] ) && is_object( $arg[0] ) ) { // array(&$this)
	$args[] =& $arg[0];
} else {
	$args[] = $arg;
}

1. Could you refresh the patch without changing the function signature for now?
2. Could you create a new ticket for reconsidering the revert of [45624]?

Thanks!

[jrf]

Simplify do_action() / remove PHP 4 code

[jrf]

@SergeyBiryukov Thanks for committing those patches <3

I think it's still preferable to show a proper error message on PHP < 5.6, at least until a switch to PHP 7.x is made.

I agree, though to be honest, this would indicate that the PHP version check is done too late in the process flow, rather than that this patch is invalid.

All the same, let's leave that for later.

1. Could you refresh the patch without changing the function signature for now?

Done. See the newly uploaded patch. Passing build for the patch: ​https://travis-ci.com/WordPress/wordpress-develop/builds/127739735

2. Could you create a new ticket for reconsidering the revert of [45624]?

Would you mind explaining why this should be moved to a new ticket ?

I'd personally rather we keep it in this ticket as comments 23 up to 29 contain a detailed analysis of the impact, which would otherwise need to be re-iterated completely in a new ticket.

It has already been established that the changes made via this ticket need a dev-note, as the function signature of a number of methods has changed, Walker:walk() is just one of those methods and should IMO not have special status just because the VIP platform overloads the method.

[SergeyBiryukov]

Replying to jrf:

I agree, though to be honest, this would indicate that the PHP version check is done too late in the process flow, rather than that this patch is invalid.

All the same, let's leave that for later.

Right. We could potentially run wp_check_php_mysql_versions() earlier, but wp-includes/plugin.php is also included in wp_load_translations_early() before displaying the error message, apparently for do_action() and apply_filters().

I would be up for exploring ways to remove that dependency in a new ticket. Since plugins cannot be loaded at that point, we could stub those functions in a way similar to wp-admin/includes/noop.php, or just display the message in English.

Would you mind explaining why this should be moved to a new ticket ?

I'd personally rather we keep it in this ticket as comments 23 up to 29 contain a detailed analysis of the impact, which would otherwise need to be re-iterated completely in a new ticket.

It has already been established that the changes made via this ticket need a dev-note, as the function signature of a number of methods has changed, Walker:walk() is just one of those methods and should IMO not have special status just because the VIP platform overloads the method.

Fair enough :) Let's handle it here, comment:23 to comment:29 do indeed provide valuable context.

[SergeyBiryukov]

In 46149:

Code Modernisation: Remove redundant PHP 4 code from do_action().

As of PHP 5, objects are always passed by reference, so this has not been needed for quite some time.

Props jrf.
See #47678.

[SergeyBiryukov]

Replying to SergeyBiryukov:

Replying to jrf:

I agree, though to be honest, this would indicate that the PHP version check is done too late in the process flow, rather than that this patch is invalid.

All the same, let's leave that for later.

Right. We could potentially run wp_check_php_mysql_versions() earlier, but wp-includes/plugin.php is also included in wp_load_translations_early() before displaying the error message, apparently for do_action() and apply_filters().

I would be up for exploring ways to remove that dependency in a new ticket. Since plugins cannot be loaded at that point, we could stub those functions in a way similar to wp-admin/includes/noop.php, or just display the message in English.

Created #48059 to change the process flow and check the required PHP version as early as possible, so that the original version of 47678-do_action.patch​ could be committed. Could you refresh that patch after [46149]?

[ayeshrajans]

I apologize for the noise in the thread, but I wanted to say that you all do an amazing work modernizing WordPress!

[jrf]

Simplify & modernize do_action() - refreshed after the PHP 4 code was removed from the function

[jrf]

Created #48059 to change the process flow and check the required PHP version as early as possible, so that the original version of 47678-do_action.patch​ could be committed.

@SergeyBiryukov You're a star! Awesome work, including having this committed for WP 5.3.

Could you refresh that patch after [46149]?

Done.

I apologize for the noise in the thread, but I wanted to say that you all do an amazing work modernizing WordPress!

@ayeshrajans Thank you 😊

[jrf]

I've started drafting a dev-note for WP 5.3 covering this ticket and I'm wondering how much detail the dev note should contain ?

Should it contain the details about the method signature changes (old vs new) for each adjusted method or should it, for instance, just contain a link to the relevant commit ? or even just a link to this ticket ?

I will list all the methods changed in the dev-note either way.

/cc @desrosj

[jrf]

Draft dev-note for this ticket: ​https://gist.github.com/jrfnl/23fa057e5a951ab8b6bf8709e5e5b281

[jrf]

Status summary:

Ok, so there are still three patches to be (re-)committed before this ticket can be closed:

• 47678-do_action-spread-operator-v2.patch
• 47678-Walker-walk.patch
• 47678-Walker-paged_walk.patch

A ​dev-note to accompany the changes has been prepared.

Before publishing the dev-note, I'd like to get clarification on whether the Walker class patches will or will not go into WP 5.3 as they should be included in the dev-note if they go in.

And if they don't go in, WP 5.4 (or whatever future version) would again need a separate dev-note.

What will all the research done for this and spelled out above in comments 23 up to 29, the change can be estimated to impact ~0.05% of plugins and themes.

Aside from that, the way the change has been made, those can be fixed in a BC-compatible way, as in: the affected plugins/themes don't need to drop support for older WP versions to be compatible with WP 5.3. They only need to drop support for PHP < 5.6.
See the dev-note for more details.

@pento What say you ?

[davidbaumwald]

@pento @jrf With the deadline for enhancements nearing for 5.3 Beta 1, the remaining bits of this are being punted to 5.4.

[desrosj]

I'm going to leave this one in 5.3 just because there have been commits already in the milestone.

Let's convert to a task to decide whether the remaining changes are worth making during beta, or should be split off into separate tickets for 5.4.

[SergeyBiryukov]

In 46322:

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

Rather than relying on 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.

[jrf]

What to do with the last two remaining patches - 47678-Walker-walk.patch and
47678-Walker-paged_walk.patch - has been discussed in Slack (see above mentioned logs) and it's been agreed that they are OK to go into WP 5.3.

Prerequisite for merging this is that the following function gets adjusted in Meta:

​https://meta.trac.wordpress.org/browser/sites/trunk/wordpress.org/public_html/wp-content/plugins/handbook/inc/walker.php#L31

Basically, the same changes as outlined in patch 47678-Walker-walk.patch should be applied to the code in Meta.

Anyone with commit rights to Meta want to make the change ?

The change to Meta can go in any time, the change to Core can follow as soon as the Meta change has been made.

[jrf]

Related #meta ticket: ​https://meta.trac.wordpress.org/ticket/4758

[SergeyBiryukov]

In 46442:

Code Modernisation: Introduce the spread operator in Walker::walk() and ::paged_walk().

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

This re-applies [45624] and reverts [45640], with a dev note upcoming for plugin authors to maintain backward compatibility with old versions of WordPress.

Props jrf.
See #47678.

[pento]

Thank you for committing that, @SergeyBiryukov!

To close the loop (given that I originally reverted this change), I'm 💯 in favour of it landing now. @jrf did an excellent job of showing that the back compat risks are minimal: it will only cause a PHP warning, there's no functional breakage. It also likely affects a very small number of sites.

[jrf]

Documentation only patch: add @since changelog entries about the function signature changes

[jrf]

Just uploaded one more - documentation only - patch to ensure that the functions which have had an update to their function signature have this recorded in their @since changelog.

[SergeyBiryukov]

In 46451:

Docs: Add a @since note about new parameters with the spread operator added to function signatures.

Props jrf.
See #47678.

[SergeyBiryukov]

In 46452:

Docs: Tweak the @since note in Walker::walk() and ::paged_walk() for better readability and consistency with other notes.

See #47678.

[jrf]

Thank you all for working with me to get this slew of patches in. Special kudos to @SergeyBiryukov and @pento for their amazing support and doing all the commits!

Dev-note has been published: ​https://make.wordpress.org/core/2019/10/09/wp-5-3-introducing-the-spread-operator/ and a tweet for good measure in case anyone wants to retweet: ​https://twitter.com/jrf_nl/status/1181850598460837888

So, with that I'm closing this ticket as completed. 🎉

[desrosj]

For future reference, here is the dev note: ​https://make.wordpress.org/core/2019/10/09/wp-5-3-introducing-the-spread-operator/