WordPress.org

Make WordPress Core

Opened 8 months ago

Last modified 5 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: Future Release Priority: normal
Severity: normal Version:
Component: General Keywords: php8 has-patch 2nd-opinion dev-feedback needs-docs needs-codex
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 (18)

#1 @SergeyBiryukov
8 months ago

  • Milestone changed from Awaiting Review to 5.6

#2 @johnbillion
8 months 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
8 months 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
8 months 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.


8 months 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
8 months 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
8 months ago

jrfnl commented on PR #612:

Rebased for merge conflicts due to 5b6a20af07f7a34b1e20611a58741e20454cbd1a

This ticket was mentioned in Slack in #core by sergey. View the logs.


7 months ago

This ticket was mentioned in Slack in #core by jrf. View the logs.


7 months ago

#10 @johnbillion
7 months ago

Opinion from me: I think it's unrealistic to never again change the name of a parameter to avoid breaking usage of named parameters. Maybe in 9 or 12 months' time when we've had a chance to audit the whole of core then we can commit to that.

I would support a statement on make/core that the project cannot guarantee there will be no changes to parameter names after 5.6, and anybody who chooses to immediately start using named parameters with PHP 8 needs to expect such changes in the short term.

That said, after some cursory searching I've not found a similar statement from any other PHP project, possibly because PHP 8 stable isn't released yet. It would be interesting to know if other projects are considering this too.

#11 @desrosj
7 months ago

Sorry it took a while to weigh in on this one. Looking at the 3 tasks in this ticket, the discussion over on #50531 which is related, and the amount of time left to tackle this quickly dissipating, I don't think that tasks 1 and 2 are realistic for 5.6.

I think that the audit required to ensure item 3 will not cause compatibility issues should move forward prior to 5.6.

I am drafting the dev note about PHP 8 and WordPress and will be explicitly recommending against using named parameters when utilizing WordPress Core functions and classes until tasks 1 and 2 can be completed. Since named parameters affect call_user_func_array(), a callout about that can be included.

#12 follow-up: @jrf
7 months ago

Thanks @johnbillion and @desrosj for chiming in.

I would support a statement on make/core that the project cannot guarantee there will be no changes to parameter names after 5.6, and anybody who chooses to immediately start using named parameters with PHP 8 needs to expect such changes in the short term.

I'd suggest taking that one step further and would like to recommend very explicitly stating that "WP will NOT support named parameters for the time being and that it will be announced when this will be supported at a later point in time".

That said, after some cursory searching I've not found a similar statement from any other PHP project,

A lot of projects have already been reviewing and renaming parameters in anticipation of PHP 8, including projects like PHP itself, PHPUnit, PHPOffice etc.

Other projects may not have started their PHP 8 efforts yet.

I don't think that tasks 1 and 2 are realistic for 5.6.

Task 1 is already finished for all WP native classes. See the attached patch I created four weeks ago.

explicitly recommending against using named parameters when utilizing WordPress Core functions and classes until tasks 1 and 2 can be completed

:+1:

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

#13 @prbot
7 months ago

jrfnl commented on PR #612:

@desrosj I would have happily rebased ...

#14 @prbot
7 months ago

desrosj commented on PR #612:

Sorry! I was actively reviewing so figured I'd just update to use the same PR. Updating the ticket soon!

#15 @desrosj
7 months ago

  • Keywords 2nd-opinion removed
  • Milestone changed from 5.6 to 5.7

I was reviewing the task 1 PR with the intention of landing it prior to RC1, but I think it's best to wait in order perform a more thorough review and have a longer testing period for any issues.

Since we are going to explicitly advise against using named parameters in 5.6, task 1 should not cause compatibility issues. Unless someone goes against the advisory at their own risk.

I was discussing with @SergeyBiryukov, and here are some notes from what we discussed:

  • There are a few name changes that we didn't love. For example, renaming $item to $data_object everywhere in the affected code. It seems simpler to just assign $item = $data_object; at the beginning. This is something done in other instances already.
  • In some places the PR just assignes $post = $item at the beginning, in others it replaces everything with $item, which could make the code less clear. One location this is noticeable is within class-wp-ms-themes-list-table.php. The first two methods rename the variable across the whole method, but the third one does not. Would be great to have some consistency there.

I think task 3 is the only area here for potential issues in PHP 8 when not utilizing the named parameters feature, but to my knowledge no related problems have been reported. It's likely some will be uncovered going forward though.

All this being said, let's prioritize this for 5.7 with #50531.

Last edited 5 weeks ago by SergeyBiryukov (previous) (diff)

#16 @jrf
7 months ago

I was discussing with @SergeyBiryukov, and here are some notes from what we discussed:

@desrosj The choices I made for that and why are explained in the commit message. Could it be you missed that ?

Other than that, it will be easier to discuss the details by leaving comments on the specific lines in the PR, so I know what you're talking about and can respond with that context in mind.

#17 in reply to: ↑ 12 @azaozz
6 months ago

  • Keywords 2nd-opinion dev-feedback needs-docs needs-codex added; needs-dev-note removed
  • Milestone changed from 5.7 to Future Release

Replying to jrf:

I'd suggest taking that one step further and would like to recommend very explicitly stating that "WP will NOT support named parameters for the time being and that it will be announced when this will be supported at a later point in time".

This!! By far the most prudent approach for the time being.

Of course this will have to be reviewed again in a year or two, when PHP 8.0+ becomes more "mainstream".

Task 1 is already finished for all WP native classes. See the attached patch I created four weeks ago.

Looking at https://github.com/WordPress/wordpress-develop/pull/612/files, the changes there make the code less self-documenting, less intuitive/readable.

True, picking good, intuitive, helpful function, var, param, arg, etc. names when writing core is probably one of the hardest tasks. Been feeling "stuck" at that pretty often :)

Not claiming that WP has the best, most helpful, self-documenting names, but they are pretty good in most cases. Renaming instances of WP_Post, WP_Term, WP_Comment, WP_Theme, WP_User, etc. from $post, $page, $category, $tag, $comment, $user, etc. to $item or $data_object is a big step backwards, and frankly the trade-off doesn't look acceptable imho.

A new language feature that demands or requires reduced/diminished code readability should simply be rejected, especially if the benefits are somewhat limited, and it brings a lot of back-compat problems/considerations.

Changing this to future-release in the hope that more acceptable patterns/ways to handle this may emerge once PHP 8.0 is widely used.

#18 @jrf
6 months ago

#51900 was marked as a duplicate.

Note: See TracTickets for help on using tickets.