Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions src/wp-includes/class-wp-hook.php
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ private function resort_active_iterations( $new_priority = false, $priority_exis
* a callback that may or may not exist.
* @param int $priority The exact priority used when adding the original filter callback.
* @return bool Whether the callback existed before it was removed.
* @phpstan-param callable|string|array{ 0: string|object, 1: string, ... } $callback

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose array{ 0: string|object, 1: string, ... } as the array shape for the $callback because this is the array shape required by _wp_filter_build_unique_id() or else it will return null.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about encapsulating callable|string|array{ 0: string|object, 1: string, ... } into a PHPStan type on the class with a name that makes it clear what it represents? eg:

@phpstan-type Maybe_Callable callable|string|array{ 0: string|object, 1: string }

I don't believe the trailing ... is necessary on the array either. A callable array will only every have two elements, the class name or object and the method name.

*/
public function remove_filter( $hook_name, $callback, $priority ) {
if ( null === $priority ) {
Expand Down Expand Up @@ -248,13 +249,14 @@ public function remove_filter( $hook_name, $callback, $priority ) {
* of that hook is returned, or false if the function is not attached.
* If `$callback` and `$priority` are both provided, a boolean is returned
* for whether the specific function is registered at that priority.
* @phpstan-param callable|string|array{ 0: string|object, 1: string, ... }|false $callback
*/
public function has_filter( $hook_name = '', $callback = false, $priority = false ) {
if ( false === $callback ) {
return $this->has_filters();
}

$function_key = _wp_filter_build_unique_id( $hook_name, $callback, false );
$function_key = _wp_filter_build_unique_id( $hook_name, $callback, is_int( $priority ) ? $priority : 10 );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The $priority argument is no longer used in _wp_filter_build_unique_id(). Perhaps it should be removed instead.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was debating whether to do this when working through this. The $hook_name parameter isn't used either, but we can't remove it so easily. My hesitation about removing it is that static analysis with older versions of WP would then flag the lack of the arg as being an error when supplied, or otherwise on newer versions it would flag when it is supplied. I think it's just as well to leave it as-is for now.


if ( ! $function_key ) {
return false;
Expand Down Expand Up @@ -320,9 +322,9 @@ public function remove_all_filters( $priority = false ) {
*
* @since 4.7.0
*
* @param mixed $value The value to filter.
* @param array $args Additional parameters to pass to the callback functions.
* This array is expected to include $value at index 0.
* @param mixed $value The value to filter.
* @param list<mixed> $args Additional parameters to pass to the callback functions.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frustratingly, $args isn't necessarily a list here (although I can see that WP_Hook:: do_action() already documents it as such). The top-level apply_filters() function can be called with the associative array unpacking syntax:

$args = [
    'one' => 1,
    'two' => 2,
];
$value = apply_filters( 'foo', 'bar', ...$args );

This causes named parameters to be passed to apply_filters() which results in an associative array of those named parameters being passed to WP_Hook::apply_filters().

I doubt anyone is doing this in real life, but for type safety we need to account for it. Not sure the best approach though. Perhaps $args = array_values( $args )?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, in PHP 8 passing an associative array to call_user_func_array() maps the parameters to the corresponding named param in the callback function: https://3v4l.org/GXnqP#veol

When passing the array through apply_filters(), then the mapping is lost, resulting in (apparent) incorrect param: https://3v4l.org/lTFHM#veol

This doesn't work in PHP 8.0 at all. It only starts to work in PHP 8.1. In PHP 7.x, the associative nature of the array is ignored.

Since this didn't work in PHP 7.x and you couldn't pass them in PHP 8.0 either, it seems like now is the time to make sure that $args gets turned into a list to make sure to preserve the expected behavior from PHP 7.

As for how to type apply_filters() so that static analysis flags attempting to spread named parameters into an invocation, I just learned about the @no-named-arguments annotation which does the trick:

apply_filters( 'foo', 'bar', ...array( 'asdasd' => 1 ) );

Function apply_filters invoked with named argument $asdasd, but it's not allowed because of @no-named-arguments.

I've pushed this as bd5a548.

We could add something like this in a function like apply_filters(), do_action(), et al:

/** @var mixed[] $args */
if ( ! array_is_list( $args ) ) {
	_doing_it_wrong( __FUNCTION__, __( 'Expected args as list.' ), '7.1.0' );
	$args = array_values( $args );
}

This might be overkill and it adds another function call in the critical path for WP.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the latest changes, given the following PHP file:

<?php
apply_filters( 'foo', 'bar', ...array( 'baz' => 1 ) );
apply_filters_ref_array(
	'foo',
	array(
		'bar',
		'baz' => 1,
	)
);
apply_filters_deprecated(
	'foo',
	array(
		'bar',
		'baz' => 1,
	),
	'1.0'
);

do_action( 'foo', ...array( 'bar' => 1 ) );
do_action_ref_array(
	'foo',
	array(
		'bar' => 1,
	)
);
do_action_deprecated(
	'foo',
	array(
		'bar' => 1,
	),
	'1.0'
);

PHPStan (rule level 10) is flagging each invocation as having an error:

 ------ ----------------------------------------------------------------------- 
  Line   try-bad-hook-params.php                                                
 ------ ----------------------------------------------------------------------- 
  2      Function apply_filters invoked with named argument $baz, but it's not  
         allowed because of @no-named-arguments.                                
         🪪  argument.named                                                     
         at src/wp-content/mu-plugins/try-bad-hook-params.php:2                 
  5      Parameter #2 $args of function apply_filters_ref_array expects list<m  
         ixed>, array{0: 'bar', baz: 1} given.                                  
         🪪  argument.type                                                      
         💡  array{0: 'bar', baz: 1} is not a list.                             
         at src/wp-content/mu-plugins/try-bad-hook-params.php:5                 
  12     Parameter #2 $args of function apply_filters_deprecated expects list<  
         mixed>, array{0: 'bar', baz: 1} given.                                 
         🪪  argument.type                                                      
         💡  array{0: 'bar', baz: 1} is not a list.                             
         at src/wp-content/mu-plugins/try-bad-hook-params.php:12                
  19     Function do_action invoked with named argument $bar, but it's not      
         allowed because of @no-named-arguments.                                
         🪪  argument.named                                                     
         at src/wp-content/mu-plugins/try-bad-hook-params.php:19                
  22     Parameter #2 $args of function do_action_ref_array expects list<mixed  
         >, array{bar: 1} given.                                                
         🪪  argument.type                                                      
         💡  array{bar: 1} is not a list.                                       
         at src/wp-content/mu-plugins/try-bad-hook-params.php:22                
  28     Parameter #2 $args of function do_action_deprecated expects list<mixe  
         d>, array{bar: 1} given.                                               
         🪪  argument.type                                                      
         💡  array{bar: 1} is not a list.                                       
         at src/wp-content/mu-plugins/try-bad-hook-params.php:28                
 ------ ----------------------------------------------------------------------- 


 [ERROR] Found 6 errors                                                         

* This array is expected to include $value at index 0.
* @return mixed The filtered value after all hooked functions are applied to it.
*/
public function apply_filters( $value, $args ) {
Expand All @@ -337,9 +339,14 @@ public function apply_filters( $value, $args ) {
$num_args = count( $args );

do {
$this->current_priority[ $nesting_level ] = current( $this->iterations[ $nesting_level ] );
$priority = current( $this->iterations[ $nesting_level ] );

if ( false === $priority ) {
// This is not expected to occur since the hook is known to have callbacks at one or more priorities.
break;
}

$priority = $this->current_priority[ $nesting_level ];
$this->current_priority[ $nesting_level ] = $priority;

foreach ( $this->callbacks[ $priority ] as $the_ ) {
if ( ! $this->doing_action ) {
Expand Down Expand Up @@ -370,7 +377,7 @@ public function apply_filters( $value, $args ) {
*
* @since 4.7.0
*
* @param array $args Parameters to pass to the callback functions.
* @param list<mixed> $args Parameters to pass to the callback functions.
*/
public function do_action( $args ) {
$this->doing_action = true;
Expand All @@ -387,7 +394,7 @@ public function do_action( $args ) {
*
* @since 4.7.0
*
* @param array $args Arguments to pass to the hook callbacks. Passed by reference.
* @param list<mixed> $args Arguments to pass to the hook callbacks. Passed by reference.
*/
public function do_all_hook( &$args ) {
$nesting_level = $this->nesting_level++;
Expand Down
43 changes: 23 additions & 20 deletions src/wp-includes/plugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ function add_filter( $hook_name, $callback, $priority = 10, $accepted_args = 1 )
* @param string $hook_name The name of the filter hook.
* @param mixed $value The value to filter.
* @param mixed ...$args Optional. Additional parameters to pass to the callback functions.
* @no-named-arguments
* @return mixed The filtered value after all hooked functions are applied to it.
*/
function apply_filters( $hook_name, $value, ...$args ) {
Expand Down Expand Up @@ -222,8 +223,8 @@ function apply_filters( $hook_name, $value, ...$args ) {
* @global int[] $wp_filters Stores the number of times each filter was triggered.
* @global string[] $wp_current_filter Stores the list of current filters with the current one last.
*
* @param string $hook_name The name of the filter hook.
* @param array $args The arguments supplied to the functions hooked to `$hook_name`.
* @param string $hook_name The name of the filter hook.
* @param list<mixed> $args The arguments supplied to the functions hooked to `$hook_name`.
* @return mixed The filtered value after all hooked functions are applied to it.
*/
function apply_filters_ref_array( $hook_name, $args ) {
Expand Down Expand Up @@ -484,6 +485,7 @@ function add_action( $hook_name, $callback, $priority = 10, $accepted_args = 1 )
* @param string $hook_name The name of the action to be executed.
* @param mixed ...$arg Optional. Additional arguments which are passed on to the
* functions hooked to the action. Default empty.
* @no-named-arguments
*/
function do_action( $hook_name, ...$arg ) {
global $wp_filter, $wp_actions, $wp_current_filter;
Expand Down Expand Up @@ -537,8 +539,8 @@ function do_action( $hook_name, ...$arg ) {
* @global int[] $wp_actions Stores the number of times each action was triggered.
* @global string[] $wp_current_filter Stores the list of current filters with the current one last.
*
* @param string $hook_name The name of the action to be executed.
* @param array $args The arguments supplied to the functions hooked to `$hook_name`.
* @param string $hook_name The name of the action to be executed.
* @param list<mixed> $args The arguments supplied to the functions hooked to `$hook_name`.
*/
function do_action_ref_array( $hook_name, $args ) {
global $wp_filter, $wp_actions, $wp_current_filter;
Expand Down Expand Up @@ -713,11 +715,11 @@ function did_action( $hook_name ) {
*
* @see _deprecated_hook()
*
* @param string $hook_name The name of the filter hook.
* @param array $args Array of additional function arguments to be passed to apply_filters().
* @param string $version The version of WordPress that deprecated the hook.
* @param string $replacement Optional. The hook that should have been used. Default empty.
* @param string $message Optional. A message regarding the change. Default empty.
* @param string $hook_name The name of the filter hook.
* @param list<mixed> $args Array of additional function arguments to be passed to apply_filters().
* @param string $version The version of WordPress that deprecated the hook.
* @param string $replacement Optional. The hook that should have been used. Default empty.
* @param string $message Optional. A message regarding the change. Default empty.
* @return mixed The filtered value after all hooked functions are applied to it.
*/
function apply_filters_deprecated( $hook_name, $args, $version, $replacement = '', $message = '' ) {
Expand All @@ -741,11 +743,11 @@ function apply_filters_deprecated( $hook_name, $args, $version, $replacement = '
*
* @see _deprecated_hook()
*
* @param string $hook_name The name of the action hook.
* @param array $args Array of additional function arguments to be passed to do_action().
* @param string $version The version of WordPress that deprecated the hook.
* @param string $replacement Optional. The hook that should have been used. Default empty.
* @param string $message Optional. A message regarding the change. Default empty.
* @param string $hook_name The name of the action hook.
* @param list<mixed> $args Array of additional function arguments to be passed to do_action().
* @param string $version The version of WordPress that deprecated the hook.
* @param string $replacement Optional. The hook that should have been used. Default empty.
* @param string $message Optional. A message regarding the change. Default empty.
*/
function do_action_deprecated( $hook_name, $args, $version, $replacement = '', $message = '' ) {
if ( ! has_action( $hook_name ) ) {
Expand Down Expand Up @@ -967,7 +969,7 @@ function register_uninstall_hook( $file, $callback ) {
*
* @global WP_Hook[] $wp_filter Stores all of the filters and actions.
*
* @param array $args The collected parameters from the hook that was called.
* @param list<mixed> $args The collected parameters from the hook that was called.
*/
function _wp_call_all_hook( $args ) {
global $wp_filter;
Expand All @@ -992,12 +994,13 @@ function _wp_call_all_hook( $args ) {
*
* @access private
*
* @param string $hook_name Unused. The name of the filter to build ID for.
* @param callable $callback The callback to generate ID for. The callback may
* or may not exist.
* @param int $priority Unused. The order in which the functions
* associated with a particular action are executed.
* @param string $hook_name Unused. The name of the filter to build ID for.
* @param callable|string|array $callback The callback to generate ID for. The callback may
* or may not exist.
* @param int $priority Unused. The order in which the functions
* associated with a particular action are executed.
* @return string|null Unique function ID for usage as array key, or null if it couldn't be determined.
* @phpstan-param callable|string|array{ 0: string|object, 1: string, ... } $callback
*/
function _wp_filter_build_unique_id( $hook_name, $callback, $priority ): ?string {
if ( is_string( $callback ) ) {
Expand Down
Loading