Make WordPress Core

Opened 19 months ago

Last modified 5 weeks ago

#59649 new task (blessed)

PHP 8.0: improvements to allow for named parameters in 6.9

Reported by: hellofromtonya's profile hellofromTonya Owned by:
Milestone: 6.9 Priority: normal
Severity: normal Version:
Component: General Keywords: php80
Focuses: php-compatibility Cc:

Description

Previously:

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

#1 @swissspidy
15 months ago

  • Type changed from enhancement to task (blessed)

#2 @swissspidy
14 months ago

@hellofromTonya It doesn't look like this will be worked on this cycle. Any objections to renaming this ticket & repurposing for 6.6?

#3 @swissspidy
14 months ago

  • Milestone changed from 6.5 to 6.6
  • Summary changed from PHP 8.0: improvements to allow for named parameters in 6.5 to PHP 8.0: improvements to allow for named parameters in 6.6

#4 @hellofromTonya
11 months ago

  • Milestone changed from 6.6 to 6.7
  • Summary changed from PHP 8.0: improvements to allow for named parameters in 6.6 to PHP 8.0: improvements to allow for named parameters in 6.7

Repurposing this ticket for 6.7 as no commits happened during 6.6.

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


6 months ago

#6 @peterwilsoncc
6 months ago

  • Milestone changed from 6.7 to 6.8
  • Summary changed from PHP 8.0: improvements to allow for named parameters in 6.7 to PHP 8.0: improvements to allow for named parameters in 6.8

#7 @albatross10
4 months ago

Task 1

child classes/classes implementing an interface

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/class-wp-block-list.php

Methods

offsetExists() - Arguments: $offset
offsetGet() - Arguments: $offset
offsetSet() - Arguments: $offset, $value
offsetUnset() - Arguments: $offset
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/class-wp-feed-cache-transient.php

Methods

__construct( $location, $name, $type )
save( $data )
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/class-wp-hook.php

Methods
offsetExists( $offset )
offsetGet( $offset )
offsetSet( $offset, $value )
offsetUnset( $offset )
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/class-wp-block-list.php

Methods
offsetExists( $offset )
offsetGet( $offset )
offsetSet( $offset, $value )
offsetUnset( $offset )
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/Requests/src/Hooks.php

Methods
register($hook, $callback, $priority = 0)
dispatch($hook, $parameters = [])
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/Requests/src/Auth/Basic.php

Methods
register(Hooks $hooks)
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/Requests/src/Cookie/Jar.php

Methods
offsetExists( $offset )
offsetGet( $offset )
offsetSet( $offset, $value )
offsetUnset( $offset )
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/Requests/src/Proxy/Http.php

Methods
register(Hooks $hooks)
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/Requests/src/Transport/Curl.php

Methods
request($url, $headers = [], $data = [], $options = [])
request_multiple($requests, $options)
test($capabilities = [])
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/Requests/src/Transport/Fsockopen.php

Methods
request($url, $headers = [], $data = [], $options = [])
request_multiple($requests, $options)
test($capabilities = [])
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/Requests/src/Utility/CaseInsensitiveDictionary.php

Methods
offsetExists( $offset )
offsetGet( $offset )
offsetSet( $offset, $value )
offsetUnset( $offset )
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/tree/trunk/src/wp-includes/rest-api

Methods
offsetExists( $offset )
offsetGet( $offset )
offsetSet( $offset, $value )
offsetUnset( $offset )
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/SimplePie/src/Item.php

Methods
set_registry(\SimplePie\Registry $registry)
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/SimplePie/src/Locator.php

Methods
set_registry(\SimplePie\Registry $registry)
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/SimplePie/src/Parser.php

Methods
set_registry(\SimplePie\Registry $registry)
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/SimplePie/src/Sanitize.php

Methods
set_registry(\SimplePie\Registry $registry)
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/SimplePie/src/Source.php

Methods
set_registry(\SimplePie\Registry $registry)
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/SimplePie/src/Cache/BaseDataCache.php

Methods
get_data(string $key, $default = null)
set_data(string $key, array $value, ?int $ttl = null)
delete_data(string $key)
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/SimplePie/src/Cache/CallableNameFilter.php

Methods
filter(string $name)
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/SimplePie/src/Cache/DB.php

Methods
No Methods
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/SimplePie/src/Cache/File.php

Methods
__construct($location, $name, $type)
save($data)
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/SimplePie/src/Cache/Memcache.php

Methods
__construct($location, $name, $type)
save($data)
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/SimplePie/src/Cache/Memcached.php

Methods
__construct($location, $name, $type)
save($data)
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/SimplePie/src/Cache/Psr16.php

Methods
get_data(string $key, $default = null)
set_data(string $key, array $value, ?int $ttl = null)
delete_data(string $key)
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/SimplePie/src/Cache/Redis.php

Methods
__construct($location, $name, $options = null) - There is an issue with this one. The [Base](https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/SimplePie/src/Cache/Base.php) Interface has __construct($location, $name, $type) . Additionally, this might be a different task to tackle as the constructor doc block is also incorrect.
save($data)
Verified and found one method with an issue.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/sodium_compat/src/Core/ChaCha20/Ctx.php

Methods
offsetExists( $offset )
offsetGet( $offset )
offsetSet( $offset, $value )
offsetUnset( $offset )
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/sodium_compat/src/Core/Curve25519/Fe.php

Methods
offsetExists( $offset )
offsetGet( $offset )
offsetSet( $offset, $value )
offsetUnset( $offset )
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/sodium_compat/src/Core32/ChaCha20/Ctx.php

Methods
offsetExists( $offset )
offsetGet( $offset )
offsetSet( $offset, $value )
offsetUnset( $offset )
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/sodium_compat/src/Core32/Curve25519/Fe.php

Methods
offsetExists( $offset )
offsetGet( $offset )
offsetSet( $offset, $value )
offsetUnset( $offset )
Verified and no issues found.

  1. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/sodium_compat/src/PHP52/SplFixedArray.php

Methods
offsetExists($index) - Needs Attention
offsetGet($index) - Needs Attention
offsetSet($index, $newval) - Needs Attention
offsetUnset($index) - Needs Attention
Verified and 4 issues found.

In addition to this, feel free to let me know if we can add these long details elsewhere in case these are too long for comments.

Thank You

CC @hellofromTonya

Last edited 4 months ago by albatross10 (previous) (diff)

#8 follow-up: @albatross10
4 months ago

Last edited 4 months ago by albatross10 (previous) (diff)

#9 @albatross10
4 months ago

In addition to this

I did mention this above.
https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/sodium_compat/src/PHP52/SplFixedArray.php
Methods
offsetExists($index) - Needs Attention
offsetGet($index) - Needs Attention
offsetSet($index, $newval) - Needs Attention
offsetUnset($index) - Needs Attention
Verified and 4 issues found.

This file is specific to php version 5.2 so this should not be an issue too

Last edited 4 months ago by albatross10 (previous) (diff)

#10 in reply to: ↑ 8 @albatross10
4 months ago

Replying to albatross10:

#62768

Created this Ticket to track the issue in https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/SimplePie/src/Cache/Redis.php

This needs a separate task. It is mentioned here https://core.trac.wordpress.org/ticket/62768#comment:1

Thanks

This will no longer be an issue as the Redis Class has been deprecated

#11 @albatross10
4 months ago

Hello @hellofromTonya

I need a little clarification for

Task 3: Review all uses of call_user_func_array()

Do we need to implement the array_values function before each call_user_func_array() in the case where it is not clear about the argument being a numeric indexed array?

Example: call_user_func_array( 'test_function', $options );

In this case, we can assume that the $options is a numeric indexed array but it can also be a string one. So if we do use array_values, passing the key names as parameter names will also not matter. So in this case, it seems we would have to add a condition check for param names if the function does support it. Otherwise use array_values before it

#12 @desrosj
5 weeks ago

  • Milestone changed from 6.8 to 6.9
  • Summary changed from PHP 8.0: improvements to allow for named parameters in 6.8 to PHP 8.0: improvements to allow for named parameters in 6.9

Hi @albatross10,

Thank you for taking a look at this and providing this detail. Apologies that someone did not have a chance to look into this for the 6.8 cycle. I am going to punt this to 6.9 as 6.8 RC1 is due out at some point today.

Note: See TracTickets for help on using tickets.