Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 14 months ago

#51553 closed task (blessed) (fixed)

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

Reported by: jrf's profile jrf Owned by: hellofromtonya's profile 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

  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

Attachments (1)

WP_ListTable-column_width.diff (9.4 KB) - added by hellofromTonya 3 years ago.
WP_List_Table::column_default(): fix parameters name mismatches (task 1)

Download all attachments as: .zip

Change History (59)

#1 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 5.6

#2 @johnbillion
3 years 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
3 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 @ayeshrajans
3 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.


3 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 @jrf
3 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.

jrfnl commented on PR #612:


3 years ago
#7

Rebased for merge conflicts due to 5b6a20af07f7a34b1e20611a58741e20454cbd1a

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


3 years ago

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


3 years ago

#10 @johnbillion
3 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 @desrosj
3 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: @jrf
3 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:

Last edited 3 years ago by jrf (previous) (diff)

jrfnl commented on PR #612:


3 years ago
#13

@desrosj I would have happily rebased ...

desrosj commented on PR #612:


3 years ago
#14

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

#15 @desrosj
3 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 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 3 years ago by SergeyBiryukov (previous) (diff)

#16 @jrf
3 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 @azaozz
3 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.

#18 @jrf
3 years ago

#51900 was marked as a duplicate.

#19 @jrf
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 @hellofromTonya
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 @azaozz
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? :)

Last edited 3 years ago by azaozz (previous) (diff)

#22 follow-up: @jrf
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:

  1. preserve the more descriptive, self-documenting names, which make the rest of the code easier to understand
  2. 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 @azaozz
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.

Last edited 3 years ago by azaozz (previous) (diff)

#24 @hellofromTonya
3 years ago

@azaozz and I talked about this today. We agreed on ways to improve readability:

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

  1. 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 @hellofromTonya
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 @jrf
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.

#28 @jrf
3 years ago

  • Keywords needs-dev-note added; needs-docs removed

#30 @hellofromTonya
3 years ago

  • Status changed from assigned to reviewing

#31 @hellofromTonya
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.

@hellofromTonya
3 years ago

WP_List_Table::column_default(): fix parameters name mismatches (task 1)

#32 @hellofromTonya
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.

#33 @hellofromTonya
3 years ago

In 51728:

Code Modernization: Fix parameter name mismatches for parent/child classes in WP_List_Table::column_default().

Matches the method signatures of the parent class and each child class.

Why? PHP 8 introduces the ability to pass named arguments to function/method calls. This means the child and parent method signatures (i.e. parameter names) need to match.

For readability:

  • @since clearly specifies the original parameter name and its new name as well as why the change happened
  • in methods longer than a single line, the generic parameter is reassigned to the original parameter restoring it for context for use within the method. An inline comment is added to explain why this reassignment is made.

Follow-up to [15632], [30679], [31210], [32740], [32753], [32754], [32755], [32756], [32757].

Props jrf, hellofromTonya, @sergeybiryukov, @azaozz, @desrosj, @johnbillion
See #51553.

#34 @hellofromTonya
3 years ago

In 51734:

Code Modernization: Improve @since message in WP_List_Table::column_default().

Improves the @since message to more clearly specify the reason for this change" for PHP 8 named parameter support.

Follow-up to [51728].

Props jrf.
See #51553.

#35 @hellofromTonya
3 years ago

In 51735:

Code Modernization: Fix parameter name mismatches for parent/child classes in WP_List_Table::column_cb().

Matches the method signatures of the parent class and each child class.

Why? PHP 8 introduces the ability to pass named arguments to function/method calls. This means the child and parent method signatures (i.e. parameter names) need to match.

For readability:

  • @since clearly specifies the original parameter name and its new name as well as why the change happened
  • in methods longer than a single line, the generic parameter is reassigned to the original parameter restoring it for context for use within the method. An inline comment is added to explain why this reassignment is made.

Follow-up to [15632], [30679], [31210], [32740], [32753], [32754], [32755], [32756], [32757].

Props jrf, hellofromTonya, sergeybiryukov, azaozz, desrosj, johnbillion.
See #51553.

#36 @hellofromTonya
3 years ago

In 51737:

Code Modernization: Fix parameter name mismatches for parent/child classes in WP_List_Table::handle_row_actions().

Matches the method signatures of the parent class and each child class.

Why? PHP 8 introduces the ability to pass named arguments to function/method calls. This means the child and parent method signatures (i.e. parameter names) need to match.

For readability:

  • @since clearly specifies the original parameter name and its new name as well as why the change happened
  • in methods longer than a single line, the generic parameter is reassigned to the original parameter restoring it for context for use within the method. An inline comment is added to explain why this reassignment is made.

Follow-up to [32644], [32664], [32798], [38489], [49183], [49197].

Props jrf, hellofromTonya, sergeybiryukov, azaozz, desrosj, johnbillion.
See #51553.

#37 @hellofromTonya
3 years ago

In 51739:

Code Modernization: Fix reserved keyword and parameter name mismatches for parent/child classes in Walker::start_el().

In the parent class, renames the parameter $object to $data_object.

Why? object is a PHP reserved keyword.

In each child class: renames the corresponding parameter to match the parent's method signature.

Why?

PHP 8 introduces the ability to pass named arguments to function/method calls. This means the child and parent method signatures (i.e. parameter names) need to match.

Changes for readability:

  • @since clearly specifies the original parameter name and its new name as well as why the change happened.
  • in methods longer than a single line, the generic parameter is reassigned to the original parameter restoring it for context for use within the method. An inline comment is added to explain why this reassignment is made.
  • in cases where the original parameter name was too generic, renamed (when reassigning) to a more descriptive name for use within the method.

Follow-up to [7737], [8900], [8970], [14248], [15077], [16100], [25642], [25644], [37051], [37054], [37056], [46271], [47189].

Props jrf, hellofromTonya, sergeybiryukov, azaozz, desrosj, johnbillion.
See #51553.

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


3 years ago

#39 @hellofromTonya
3 years ago

In 51779:

Code Modernization: Fix last parameter name mismatches for parent/child classes in Walker::start_el().

The parent class uses $current_object_id while most of the child classes use $id. As the parent class' is more descriptive, renaming the last parameter in each of child class.

Why? PHP 8 introduces the ability to pass named arguments to function/method calls. This means the child and parent method signatures (i.e. parameter names) need to match.

Changes for readability:

  • @since clearly specifies the original parameter name and its new name as well as why the change happened.
  • In methods longer than a single line, the generic parameter is reassigned to the original parameter restoring it for context for use within the method. An inline comment is added to explain why this reassignment is made.
  • In cases where the original parameter name was too generic or misleading, renamed (when reassigning) to a more descriptive name for use within the method.

Follow-up to [7737], [8900], [8970], [14248], [15077], [16100], [25642], [25644], [37051], [37054], [37056], [46271], [47189], [51739].

Props jrf, hellofromTonya, sergeybiryukov, azaozz, desrosj, johnbillion.
See #51553.

#40 @hellofromTonya
3 years ago

In 51780:

Code Modernization: Fix reserved keyword and parameter name mismatches for parent/child classes in Walker::end_el().

In the parent class, renames the parameter $object to $data_object.
Why? object is a PHP reserved keyword. The parameter name is selected for consistency with Walker::start_el().

In each child class: renames the parameter to match the parent's method signature.
Why? PHP 8 introduces the ability to pass named arguments to function/method calls. This means the child and parent method signatures (i.e. parameter names) need to match.

Changes for readability:

  • @since clearly specifies the original parameter name and its new name as well as why the change happened.

Follow-up to [7737], [8900], [8970], [14248], [16100], [25642], [25644], [37051], [37056].

Props jrf, hellofromTonya, sergeybiryukov, azaozz, desrosj, johnbillion.
See #51553.

#41 @hellofromTonya
3 years ago

In 51781:

Code Modernization: Fix reserved keyword and parameter name mismatches for parent/child classes in WP_Upgrader_Skin::feedback().

In the parent class, renames the parameter $string to $feedback.
Why? string is a PHP reserved keyword.

In each child class: renames the parameter to match the parent's method signature.

Why? PHP 8 introduces the ability to pass named arguments to function/method calls. This means the child and parent method signatures (i.e. parameter names) need to match.

Changes for readability:

  • @since clearly specifies the original parameter name and its new name as well as why the change happened.

Follow-up to [11005], [25228], [30680], [32655], [38199], [49596].

Props jrf, hellofromTonya, sergeybiryukov, azaozz, desrosj, johnbillion.
See #51553.

#42 @hellofromTonya
3 years ago

In 51782:

Code Modernization: Fix parameter name mismatches for parent/child classes in WP_Upgrader_Skin::error().

In each child class: renames the parameter to match the parent's method signature.

Why? PHP 8 introduces the ability to pass named arguments to function/method calls. This means the child and parent method signatures (i.e. parameter names) need to match.

Changes for readability:

  • @since clearly specifies the original parameter name and its new name as well as why the change happened.

Follow-up to [11005], [25806], [32655], [38199].

Props jrf, hellofromTonya, sergeybiryukov, azaozz, desrosj, johnbillion.
See #51553.

#43 @hellofromTonya
3 years ago

In 51783:

Code Modernization: Fix parameter name mismatches for parent/child classes in WP_Customize_Setting::sanitize().

In each child class: renames the parameter to match the parent's method signature.

Why? PHP 8 introduces the ability to pass named arguments to function/method calls. This means the child and parent method signatures (i.e. parameter names) need to match.

Changes for readability:

  • @since clearly specifies the original parameter name and its new name as well as why the change happened.
  • In methods longer than a single line, the generic parameter is reassigned to the original parameter restoring it for context for use within the method. An inline comment is added to explain why this reassignment is made.

Follow-up to [19995], [32806].

Props jrf, hellofromTonya, sergeybiryukov, azaozz, desrosj, johnbillion.
See #51553.

#44 @hellofromTonya
3 years ago

In 51784:

Code Modernization: Fix parameter name mismatches for parent/child classes in WP_Customize_Setting::update().

In each child class: renames the parameter to match the parent's method signature.

Why? PHP 8 introduces the ability to pass named arguments to function/method calls. This means the child and parent method signatures (i.e. parameter names) need to match.

Changes for readability:

  • @since clearly specifies the original parameter name and its new name as well as why the change happened.
  • In methods longer than a single line, the generic parameter is reassigned to the original parameter restoring it for context for use within the method. An inline comment is added to explain why this reassignment is made.

Follow-up to [19995], [21037], [21053], [21354], [38829], [51298].

Props jrf, hellofromTonya, sergeybiryukov, azaozz, desrosj, johnbillion.
See #51553.

#45 @hellofromTonya
3 years ago

In 51785:

Code Modernization: Fix parameter name mismatch with parent in WP_Customize_Custom_CSS_Setting::validate().

Renames the parameter to match the parent's method signature.
Why? PHP 8 introduces the ability to pass named arguments to function/method calls. This means the child and parent method signatures (i.e. parameter names) need to match.

Adds @since to clearly specify why the change happened.

Reassigns the generic parameter to the original parameter.
Why? Restoring the original name keeps the context intact within the method and makes the code more readable. An inline comment explains why this reassignment is made

Follow-up to [37476], [38829], [41376].

Props jrf, hellofromTonya, sergeybiryukov, azaozz, desrosj, johnbillion.
See #51553.

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

#47 @hellofromTonya
3 years ago

In 51786:

Code Modernization: Fix parameter name mismatches for parent/child classes in WP_REST_Controller::prepare_item_for_response().

In each child and grandchild class, renames the first parameter to match the parent's method signature.

Why? PHP 8 introduces the ability to pass named arguments to function/method calls. This means the child and parent method signatures (i.e. parameter names) need to match.

Changes for readability:

  • @since clearly specifies the original parameter name and its new name as well as why the change happened.
  • In methods longer than a single line, the generic parameter is reassigned to the original parameter restoring it for context for use within the method. An inline comment is added to explain why this reassignment is made.

Follow-up to [38832], [39011], [39015], [39021], [39024], [39025], [39031], [39036], [43519], [43735], [43739], [43768], [46821], [48173], [48242], [49088], [50995], [51003], [51021].

Props jrf, hellofromTonya, sergeybiryukov, azaozz, desrosj, johnbillion.
See #51553.

#48 @hellofromTonya
3 years ago

In 51787:

Code Modernization: Fix parameter name mismatches for parent/child classes in WP_Sitemaps_Provider::get_url_list().

In each child and grandchild class, renames the second parameter to match the parent's method signature.
Why? PHP 8 introduces the ability to pass named arguments to function/method calls. This means the child and parent method signatures (i.e. parameter names) need to match.

Adds @since to clearly specify why the change happened.

Reassigns the generic parameter to the original parameter.
Why? Restoring the original name keeps the context intact within the method and makes the code more readable. An inline comment explains why this reassignment is made.

Follow-up to [48072].

Props jrf, hellofromTonya, sergeybiryukov, azaozz, desrosj, johnbillion.
See #51553.

#49 @hellofromTonya
3 years ago

In 51788:

Code Modernization: Fix parameter name mismatches for parent/child classes in WP_Sitemaps_Provider::get_max_num_pages().

In each child class, renames the parameter to match the parent's method signature.
Why? PHP 8 introduces the ability to pass named arguments to function/method calls. This means the child and parent method signatures (i.e. parameter names) need to match.

Adds @since to clearly specify why the change happened.

Reassigns the generic parameter to the original parameter.
Why? Restoring the original name keeps the context intact within the method and makes the code more readable. An inline comment explains why this reassignment is made.

Note: Reassignment is done after the guard clause.
Why? To avoid unnecessary processing and memory should the method bail out.

Follow-up to [48072].

Props jrf, hellofromTonya, sergeybiryukov, azaozz, desrosj, johnbillion.
See #51553.

#50 @hellofromTonya
3 years ago

In 51789:

Code Modernization: Fix parameter name mismatches for parent/child classes in WP_Widget::update().

In each child class, renames the parameter to match the parent's method signature.
Why? PHP 8 introduces the ability to pass named arguments to function/method calls. This means the child and parent method signatures (i.e. parameter names) need to match.

Adds @since to clearly specify why the change happened.

Replaces the original with the variable name with within each method.
Why? The new name is more specific and descriptive, which improves readability.

Follow-up to [10782], [25090], [26556], [40640].

Props jrf, hellofromTonya, sergeybiryukov, azaozz, desrosj, johnbillion.
See #51553.

#51 @hellofromTonya
3 years ago

In 51790:

Code Modernization: Fix parameter name mismatches for parent/child classes in WP_Image_Editor::save().

Renames the first parameter in WP_Image_Editor_GD::save() to match the parent's method signature.
Why? PHP 8 introduces the ability to pass named arguments to function/method calls. This means the child and parent method signatures (i.e. parameter names) need to match.

Adds @since to clearly specify why the change happened.

Adds parameter descriptions to parent and both child classes.

Follow-up to [22094], [22619], [30681].

Props jrf, hellofromTonya, sergeybiryukov, azaozz, desrosj, johnbillion.
See #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 @hellofromTonya
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 @hellofromTonya
2 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 @peterwilsoncc
2 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.

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


14 months ago

Note: See TracTickets for help on using tickets.