-
Notifications
You must be signed in to change notification settings - Fork 3.5k
#64896 Fix remaining PHPStan level 10 errors in WP_Hook
#12443
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Changes from all commits
49e7f6f
6e10867
67f3a9b
b0d8b59
bf773c3
3b5fd8a
66ff15c
97f4432
199b9ce
bd5a548
824c89b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
| */ | ||
| public function remove_filter( $hook_name, $callback, $priority ) { | ||
| if ( null === $priority ) { | ||
|
|
@@ -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 ); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was debating whether to do this when working through this. The |
||
|
|
||
| if ( ! $function_key ) { | ||
| return false; | ||
|
|
@@ -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. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Frustratingly, $args = [
'one' => 1,
'two' => 2,
];
$value = apply_filters( 'foo', 'bar', ...$args );This causes named parameters to be passed to 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interestingly, in PHP 8 passing an associative array to When passing the array through 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 As for how to type apply_filters( 'foo', 'bar', ...array( 'asdasd' => 1 ) );
I've pushed this as bd5a548. We could add something like this in a function like /** @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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: |
||
| * 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 ) { | ||
|
|
@@ -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 ) { | ||
|
|
@@ -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; | ||
|
|
@@ -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++; | ||
|
|
||
There was a problem hiding this comment.
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$callbackbecause this is the array shape required by_wp_filter_build_unique_id()or else it will returnnull.There was a problem hiding this comment.
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: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.