#51553 closed task (blessed) (fixed)
PHP 8.0: code improvements to allow for named parameters in function calls
Reported by: | jrf | Owned by: | hellofromTonya |
---|---|---|---|
Milestone: | 5.9 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | php8 needs-codex early needs-dev-note needs-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
- 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.
- 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:
Attachments (1)
Change History (59)
#3
@
4 years 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
@
4 years 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.
4 years ago
#5
- 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
@
4 years 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.
This ticket was mentioned in Slack in #core by sergey. View the logs.
4 years ago
This ticket was mentioned in Slack in #core by jrf. View the logs.
4 years ago
#10
@
4 years 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
@
4 years 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:
↓ 17
@
4 years 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:
4 years ago
#14
Sorry! I was actively reviewing so figured I'd just update to use the same PR. Updating the ticket soon!
#15
@
4 years 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 withinclass-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.
#16
@
4 years 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
@
4 years 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.
#19
@
3 years ago
FYI: the gist mentioned in comment https://core.trac.wordpress.org/ticket/51553#comment:3 with actionable parameter renames still to be done (as a minimum) has been updated.
#20
@
3 years ago
During today's pair programming session, @jrf and I did a deep review of PR 612 with Juliette updating items.
Next step for this PR for is to audit the current code against this PR (as it was originally created last fall) for the following:
- Any newly introduced classes that are extending from the parent, i.e. auditing for parameter name and/or type mismatch(es)
- Any code changes within the PR's methods where the original parameter is being used, i.e. and the renamed parameter is not be assigned to the original
We plan to tackle this in our next pair programming session.
#21
@
3 years ago
I still quite dislike some of the changes in PR 612. The PHP RFC for Named Arguments says it makes the arguments self-documenting, however the changes in the current patch do exactly the opposite in some cases: some old names were (more or less) self-documenting but the new names are not.
Looking more closely, wondering if this "pattern" of renaming the args/params should be applied to these cases:
- * @param WP_Block_Type $block_type Block type data. - * @param WP_REST_Request $request Full details about the request. + * @param WP_Block_Type $item Block type data. + * @param WP_REST_Request $request Full details about the request. * @return WP_REST_Response Block type data. */ - public function prepare_item_for_response( $block_type, $request ) { - - $fields = $this->get_fields_for_response( $request ); - $data = array(); + public function prepare_item_for_response( $item, $request ) { + $block_type = $item; + $fields = $this->get_fields_for_response( $request ); + $data = array();
($block_type
was renamed to $item
to comply with the new "enhancement" in PHP 8.0, but then at the top of the function the more descriptive name was restored.)
Of course, all of these will need an inline comment to explain what's going on.
It looks a bit silly, but... Any better ideas on how to preserve the self-documenting names? :)
#22
follow-up:
↓ 23
@
3 years ago
@azaozz I'm totally with you on that this is my least favourite "feature" in PHP 8.0, but that's not something we can change, so we just have to deal with it.
I'm open to better names or better patterns, but the discrepancy between the parameter names in parent vs child classes *must* be solved for PHP 8 compatibility as explained in the original issue description.
The choice for the pattern of "generic" parameter name in declaration, renamed to the original, more specific and more descriptive name near the top of the function, was made, as explained in the PR description to:
- preserve the more descriptive, self-documenting names, which make the rest of the code easier to understand
- minimize the code churn caused by the renames
Not sure what more I can say. As I said before, I'm open to suggestions on how to solve this differently, but we do need to solve it.
#23
in reply to:
↑ 22
@
3 years ago
- Keywords early added; 2nd-opinion removed
- Milestone changed from Future Release to 5.9
Replying to jrf:
the discrepancy between the parameter names in parent vs child classes *must* be solved for PHP 8 compatibility
Yes, of course.
The choice for the pattern of "generic" parameter name in declaration, renamed to the original, more specific and more descriptive name near the top of the function...
Yeah, thinking this is probably the best that can be done under the circumstances. Just thinking it would be good to add short comments there explaining what's going on.
Seems too late to do this in 5.8. Lets do in 5.9-early.
#24
@
3 years ago
@azaozz and I talked about this today. We agreed on ways to improve readability:
- For all instances, retain the original variable name by assigning the new generic parameter to it at the top of the method.
This extra step retains the context for better readability and understanding. Yes, it is overkill in smaller methods, but the benefit for contributors outweighs the extra line of code per method.
Note: It already exists for the majority of methods in the PR.
- Add an inline comment above each of these instances to explain why this assignment is here.
This code looks and reads odd. It does. When reading it, my first thought is: why not rename the method's parameter instead of doing the assignment? Adding the inline comment does the following:
- explains the why and hopefully avoids undoing the changes.
- provides a learning opportunity for contributors to better understand cross-version compatibility and more specifically PHP 8's named arguments.
#25
@
3 years ago
- Keywords dev-feedback removed
- Owner set to hellofromTonya
- Status changed from new to assigned
This ticket was mentioned in Slack in #core-php by hellofromtonya. View the logs.
3 years ago
#27
@
3 years ago
@hellofromTonya and me worked on this ticket today during our livestream session.
We basically actioned all open remarks for GitHub PR 612. If the build passes, this should now be ready to commit.
To summarize what we've done:
- Where relevant we've added an assignment at or near the top of the function back to the old variable name.
- In all cases where such re-assignments were done (previously and the newly added ones), we've added a comment explaining the re-assignment with a reference to this ticket.
- In a few select cases were the variable would only be used once in the function, mostly in function calls, we've made the choice to change such function calls to multi-line and to explain things in a trailing comment for the passed parameter.
#31
@
3 years ago
- Keywords commit added
Marking PR 612 for commit consideration. I've done a deep and thorough code review in multiple pair programming sessions with @jrf. Should be ready for commit.
#32
@
3 years ago
@azaozz and I reviewed the commenting and refined it a wee bit more:
- Add
@since 5.9.0
with the renamed param and reason. For example:* @since 5.9.0 Renamed `$comment` to `$item` to match parent class for PHP 8 named param.
- When reassigning to the original variable, change the inline comment to remove the ticket number and a little tweak to the wording. For example:
/** * Handles the default column output. * * @since 4.3.0 * @since 5.9.0 Renamed `$post` to `$item` to match parent class for PHP 8 named param. * * @param WP_Post $item The current WP_Post object. * @param string $column_name The current column name. */ public function column_default( $item, $column_name ) { // Restores the more descriptive, specific name for use within this method. $post = $item;
The inline message is there to provide context of why the reassignment.
This ticket was mentioned in Slack in #core by sergey. View the logs.
3 years ago
This ticket was mentioned in PR #1667 on WordPress/wordpress-develop by hellofromtonya.
3 years ago
#46
This PR is used to prep commits for PR #612.
Trac ticket: https://core.trac.wordpress.org/ticket/51553
This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.
3 years ago
hellofromtonya commented on PR #612:
3 years ago
#53
Each atomic commit as been committed with the following changesets https://core.trac.wordpress.org/log/?revs=51728,51734-51735,51737,51739,51779-51790
hellofromtonya commented on PR #612:
3 years ago
#54
Each atomic commit as been committed with the following changesets https://core.trac.wordpress.org/log/?revs=51728,51734-51735,51737,51739,51779-51790
#55
@
3 years ago
- Keywords needs-patch added; has-patch commit removed
Removing the commit
and has-patch
keywords. Why? PR 612 has been fully committed, which completes Task 1 for the classes.
This ticket was mentioned in Slack in #core by sergey. View the logs.
3 years ago
#57
@
3 years ago
- Milestone changed from 5.9 to 6.0
The remaining work in this ticket requires more investigation and discussion. With Beta 1 next week, moving this ticket to 6.0 to its continue progress.
#58
@
3 years ago
- Milestone changed from 6.0 to 5.9
- Resolution set to fixed
- Status changed from reviewing to closed
There were a bunch of commits made to this ticket in the 5.9 milestone.
So the commits don't get misplaced during git bisect/blame
excursions I'm going to move this ticket back to 5.9, close it and create a follow up ticket.
For the follow up work in WP 6.0, please refer to #55327.
Are there any tools to help us with Task 1 and point 2 in Task 2?