Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 20 months ago

#55650 closed task (blessed) (fixed)

PHP 8.0: improvements to allow for named parameters in WP 6.1

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: General Keywords: php8 has-patch has-unit-tests
Focuses: Cc:

Description

Follow-up to #51553 and #55327, quoted in full below:

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 (14)

This ticket was mentioned in PR #2484 on WordPress/wordpress-develop by JustinyAhin.


2 years ago
#1

Related Trac ticket: https://core.trac.wordpress.org/ticket/55650.

This PR fixes some warning with reserved PHP names used as parameter.

See https://gist.github.com/jrfnl/a42f06ec032b0d7911cba32a72ea99ff.

This ticket was mentioned in PR #2530 on WordPress/wordpress-develop by JustinyAhin.


2 years ago
#2

Related Trac tickets:
https://core.trac.wordpress.org/ticket/55650
https://core.trac.wordpress.org/ticket/55327

This PR fixes some warning with reserved PHP names used as parameter.

This ticket was mentioned in PR #2642 on WordPress/wordpress-develop by JustinyAhin.


2 years ago
#4

  • Keywords has-unit-tests added

Related Trac tickets:
https://core.trac.wordpress.org/ticket/55650
https://core.trac.wordpress.org/ticket/55327

This PR fixes some warning with reserved PHP names used as parameter.

#5 @SergeyBiryukov
2 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#6 @SergeyBiryukov
2 years ago

In 53364:

Code Modernization: Rename parameters that use reserved keywords in wp-admin/includes/template.php.

While using reserved PHP keywords as parameter name labels is allowed, in the context of function calls using named parameters in PHP 8.0+, this will easily lead to confusion. To avoid that, it is recommended not to use reserved keywords as function parameter names.

This commit renames the $parent parameter to $parent_page in parent_dropdown().

Follow-up to [52946], [52996], [52997], [52998], [53003], [53014], [53029], [53039], [53116], [53117], [53137], [53174], [53184], [53185], [53192], [53193], [53198], [53203], [53207], [53215], [53216], [53220], [53230], [53232], [53236], [53239], [53240], [53242], [53243], [53245], [53246], [53257], [53269], [53270], [53271], [53272], [53273], [53274], [53275], [53276], [53277], [53281], [53283], [53284], [53285], [53287].

Props jrf, aristath, poena, justinahinon, SergeyBiryukov.
See #55650.

#7 @SergeyBiryukov
2 years ago

In 53365:

Code Modernization: Rename parameters to match native PHP functions in wp-includes/compat.php.

This ensures that parameter names for PHP polyfills in WordPress core 100% match the native PHP parameter names. Otherwise using named parameters with those functions could cause fatal errors for installs where the polyfills kick in.

This commit:

  • Renames the $string parameter to $message in _() polyfill.
  • Renames the $str parameter to $string in mb_substr() and mb_strlen() polyfills.
  • Renames the $raw_output parameter to $binary in hash_hmac() polyfill.
  • Renames the $a and $b parameters to $known_string and $user_string in hash_equals() polyfill.
  • Renames the $var parameter to $value in is_countable() and is_iterable() polyfills.
  • Renames the $arr parameter to $array in array_key_first() and array_key_last() polyfills.

Follow-up to [52946], [52996], [52997], [52998], [53003], [53014], [53029], [53039], [53116], [53117], [53137], [53174], [53184], [53185], [53192], [53193], [53198], [53203], [53207], [53215], [53216], [53220], [53230], [53232], [53236], [53239], [53240], [53242], [53243], [53245], [53246], [53257], [53269], [53270], [53271], [53272], [53273], [53274], [53275], [53276], [53277], [53281], [53283], [53284], [53285], [53287], [53364].

Props jrf, aristath, poena, justinahinon, SergeyBiryukov.
See #55650.

SergeyBiryukov commented on PR #2484:


2 years ago
#8

Thanks for the PR! Most of the commits have been merged in various changesets on tickets #55327 and #55650.

The remaining files are:

  • wp-includes/class-json.php
  • wp-includes/class-wp-text-diff-renderer-inline.php
  • wp-includes/class-wp-text-diff-renderer-table.php

The first one is a copy of the Services_JSON package, which used to be a PECL extension and has since been bundled and compiled into PHP by default. The parameters in this file should be the same as in the native PHP class.

The last two extend the Text_Diff_Renderer_inline and Text_Diff_Renderer classes from the Text_Diff package and should have the same parameters as the parent class methods, per the Task 1 section of ticket #51553.

So I think these three files don't need any changes at this time.

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


2 years ago

#10 @SergeyBiryukov
2 years ago

  • Keywords php8 added; php80 removed
  • Resolution set to fixed
  • Status changed from accepted to closed

Follow-up: #56788

This ticket was mentioned in Slack in #core-php by jspellman. View the logs.


23 months ago

@SergeyBiryukov commented on PR #2530:


22 months ago
#12

Thanks for the PR! All commits from this batch of changes have been merged.

@SergeyBiryukov commented on PR #2590:


21 months ago
#13

Thanks for the PR! All commits from this batch of changes have been merged.

@SergeyBiryukov commented on PR #2642:


20 months ago
#14

Thanks for the PR! All commits from this batch of changes have been merged.

Note: See TracTickets for help on using tickets.