WordPress.org

Make WordPress Core

Opened 2 weeks ago

Last modified 2 weeks ago

#51553 new task (blessed)

PHP 8.0: code improvements to allow for named parameters in function calls

Reported by: jrf Owned by:
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: General Keywords: 2nd-opinion php8 needs-dev-note has-patch
Focuses: coding-standards Cc:

Description

PHP 8.0 introduces the ability to pass named parameters to function calls.
Ref: https://wiki.php.net/rfc/named_params

Code example:

<?php
// Using positional arguments:
array_fill(0, 100, 50);
 
// Using named arguments:
array_fill(start_index: 0, num: 100, value: 50);

// Named arguments do not have to follow the same order as positional arguments:
array_fill(value: 50, num: 100, start_index: 0);

More than anything, this means that, as of PHP 8.0, renaming a parameter in a function declaration is a backward-compatibility break!

This should most definitely get prominent mention in the PHP8 dev-note as a lot of Core/plugin/theme developers will not be aware of this at this time.

I'm opening this ticket to address a number of issues this brings up for WordPress Core.

I've so far identified three tasks which should be executed ASAP to prepare for named parameters in function calls.

Task 1: Review Function Signatures of methods in child classes

Methods in child classes may use different parameter names than those used for the same method by its parent. Similarly, classes implementing an interface do not have to use the same parameter names.

While this does not change in PHP 8, it could create problems when the method is called using named parameters.

To quote from the RFC:

If an inheriting class changes a parameter name, calls using named arguments might fail, thus violating the Liskov substitution principle (LSP).

PHP will silently accept parameter name changes during inheritance, which may result in call-time exceptions when methods with renamed parameters are called

Code sample:

<?php
interface I {
    public function test($foo, $bar);
}
 
class C implements I {
    public function test($a, $b) {}
}
 
$obj = new C;
 
// Pass params according to I::test() contract
$obj->test(foo: "foo", bar: "bar"); // ERROR!

Note: For variadic functions this will not cause an error, but will move the unrecognized names to be part of the variadic argument, which can cause all sorts of problems.
Code sample courtesy of Chris Riley illustrating the problem: https://3v4l.org/MhJ79

With regards to WordPress, I'd like to propose making the parameter names in child classes/classes implementing an interface consistent to prevent such issues.

Task 2: Review Other Function Signatures

  1. The function signatures of existing functions and methods of all WordPress code should be examined and non-descriptive parameter names should be fixed (renamed) NOW as later will no longer be an option without creating a BC-break.
  2. While using reserved keywords as parameter name labels is allowed, in the context of function calls using named parameters, this will easily lead to confusion. I'd like to recommend to rename function parameters which use reserved keywords to remove this confusion.
    <?php
    function array_foobar($toggle = false, $array = []) {}
    
    array_foobar(array: $value);
    

Task 3: Review all uses of call_user_func_array()

Named parameters cause a BC-break for call_user_func_array() when passing an associative array.
In previous PHP versions, the array keys would have been ignored. In PHP 8, string keys will be interpreted as parameter name labels.

For more detail, see: https://wiki.php.net/rfc/named_params#call_user_func_and_friends

Basically, we need to make sure that any array passed to call_user_func_array() does NOT have string keys. If it does or if the format of the array is unknown (like when it is passed in via a function parameter), we need to add extra safeguards to make the code cross-version compatible.
A typical way to do this would be to use array_values() on the array before passing it to call_user_func_array(), though beware that will break code written for PHP 8 which actually expects the label to be used.

Other references:

Related Trac tickets: #50913, #50531

Change History (7)

#1 @SergeyBiryukov
2 weeks ago

  • Milestone changed from Awaiting Review to 5.6

#2 @johnbillion
2 weeks ago

  • Type changed from defect (bug) to task (blessed)
  • Version trunk deleted

Are there any tools to help us with Task 1 and point 2 in Task 2?

#3 @jrf
2 weeks ago

@johnbillion So glad you asked ;-)

I've just written & merged a sniff which will detect the issues for point 2 in Task 2: https://github.com/PHPCSStandards/PHPCSExtra/pull/80

Output of an initial run can be found here: https://gist.github.com/jrfnl/a42f06ec032b0d7911cba32a72ea99ff

For Task 1, I'm working through the list from Exakat: https://www.exakat.io/reports/wordpress/data/issues.html#analyzer=functions_mismatchparametername

Shall I push a branch online with what I've got so far ? Happy to collaborate on this for a joined PR.

#4 @ayeshrajans
2 weeks ago

Hi @jrf - I will be working on PHP 8 changes too, and will be happy to help. I suppose the changes would be huge, looking at the phpcs results, so a PR would definitely help compared to multiple patches.

Looking at random files in WP core, it probably would help to establish some guidelines for task 2.1 (meaningful parameter names). For example, there are parameters with name "deprecated" in it, or $args that are used more or less as $options rather when called.

This ticket was mentioned in PR #612 on WordPress/wordpress-develop by jrfnl.


2 weeks ago

  • Keywords has-patch added; needs-patch removed

This PR fixes all known instances of parameter name mismatches between methods in child classes vs the names used in the parent class.

Notes:

  • Whenever the parameter name which was originally used in the method signature of the child class was significantly more descriptive than the parameter name used in the method signature of the parent class, the parameter name in the method signature has been changed, but an assignment to the "old" parameter name is made near the top of the function to keep the readability of the rest of the function code intact. This also reduces code churn.
  • While doing these renames I noticed that a _lot_ of these functions violate the LISKOV principle by having covariant parameters, while parameters are supposed to be contravariant. This is outside the scope of this ticket, but is a concern.

Ref: https://www.php.net/manual/en/language.oop5.variance.php

Trac ticket: https://core.trac.wordpress.org/ticket/51553

#6 @jrf
2 weeks ago

@ayeshrajans Any and all help on this ticket is very very welcome as it will be huge undertaking to still get this done for the WP 5.6 release.

Might be a good idea for people to leave a note here about what task they are working on and which files/directories for that task, so we can prevent duplicate work.

The linked PR addresses Task 1 completely and where there was overlap, a few issues from Task 2-2.

I've also updated the sniff to include self and parent. While these are not reserved keywords, they can be equally confusing in the context of function calls using named arguments.

I've updated the gist with the latest scan results for Task 2-2 (based trunk + on the above patch): https://gist.github.com/jrfnl/a42f06ec032b0d7911cba32a72ea99ff

Looking at random files in WP core, it probably would help to establish some guidelines for task 2.1 (meaningful parameter names).

I agree that would be very useful. Input and suggestions welcome.

#7 @prbot
2 weeks ago

jrfnl commented on PR #612:

Rebased for merge conflicts due to 5b6a20af07f7a34b1e20611a58741e20454cbd1a

Note: See TracTickets for help on using tickets.