[wp-trac] [WordPress Trac] #47678: Modernize/simplify current_user_can()

[WordPress Trac]

#47678: Modernize/simplify current_user_can()
-------------------------------------+-------------------------------------
 Reporter:  jrf                      |       Owner:  pento
     Type:  enhancement              |      Status:  assigned
 Priority:  normal                   |   Milestone:  5.3
Component:  Role/Capability          |     Version:  trunk
 Severity:  normal                   |  Resolution:
 Keywords:  has-patch has-unit-      |     Focuses:  performance, coding-
  tests commit needs-dev-note        |  standards
-------------------------------------+-------------------------------------

Comment (by 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
 <?php
 $return = call_user_func_array( $callable, $args );
 }}}

 If `$args` is ''always'' a numerically indexed array, this can be replaced
 by:
 {{{#!php
 <?php
 $return = $callable( ...$args );
 }}}

 If `$args` is ''could potentially be an associative array'', this would
 need to be replaced by:
 {{{#!php
 <?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
 <?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
 <?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
 <?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
 <?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.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/47678#comment:31>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform