#64896 Fix remaining PHPStan level 10 errors in WP_Hook - #12443
#64896 Fix remaining PHPStan level 10 errors in WP_Hook#12443westonruter wants to merge 18 commits into
WP_Hook#12443Conversation
… types. As an `ArrayAccess` object, the offsets of `WP_Hook` are hook priorities, which are always integers (or null when appending via `offsetSet()`), and the values are the `Hook_Callback` groups keyed by unique function ID. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add `@return void` to the methods lacking return values, document the callback `$args` arrays as `list<mixed>`, and narrow the `$callback` params of `remove_filter()`/`has_filter()` to `callable`, matching `add_filter()`. In `has_filter()`, pass `0` instead of `false` to `_wp_filter_build_unique_id()`, matching its `int $priority` param. In `apply_filters()`, assign the current priority to a local variable before storing it, and bail from the loop in the (impossible) case that `current()` returns `false`, since `$current_priority` only ever holds integers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
| * a callback that may or may not exist. | ||
| * @param int $priority The exact priority used when adding the original filter callback. | ||
| * @param string $hook_name The filter hook to which the function to be removed is hooked. | ||
| * @param callable $callback The callback to be removed from running when the filter is applied. |
There was a problem hiding this comment.
I had to widen the $callback type in _wp_filter_build_unique_id() as well.
As per the documentation standards, this should generally be avoided:
That said, it is quite possible that this point is now outdated and should perhaps be reconsidered. |
I think it is obsolete, yes. PHPStan identifies the lack of a return type as an error. New Related: There has been cases of |
|
I agree that refraining from using |
| } | ||
|
|
||
| $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 ); |
There was a problem hiding this comment.
The $priority argument is no longer used in _wp_filter_build_unique_id(). Perhaps it should be removed instead.
There was a problem hiding this comment.
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.
| * @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. |
There was a problem hiding this comment.
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 )?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Reverted in 199b9ce |
…ay not be defined in the current scope.
…which no longer applies.
…filter_build_unique_id()`.
| - ../../src/wp-includes/pomo | ||
| - ../../src/wp-includes/rss.php | ||
| - ../../src/wp-includes/sodium_compat | ||
| typeAliases: |
There was a problem hiding this comment.
TIL! I always defined them locally in a class.
There was a problem hiding this comment.
The only problem is it isn't supported by PhpStorm yet 😦
There was a problem hiding this comment.
Pull request overview
This PR continues the PHPStan type-tightening work around WordPress hooks by removing remaining level-10 issues in WP_Hook and aligning hook-related docblocks/config with the intended callback and args shapes.
Changes:
- Introduces a
Maybe_CallablePHPStan type alias and uses it to narrow hook callback parameters in docs. - Updates hook API docblocks to describe callback argument arrays as
list<mixed>and adds@no-named-argumentsto variadic hook functions. - Adjusts
WP_Hook::has_filter()/apply_filters()internals to satisfy stricter PHPStan typing, and removes the corresponding PHPStan baseline entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/phpstan/baselines/argument.type.neon | Removes the baseline for passing false where an int is expected in WP_Hook. |
| tests/phpstan/base.neon | Adds a Maybe_Callable type alias used by hook-related PHPStan annotations. |
| src/wp-includes/plugin.php | Tightens hook function docblocks (args lists, callback types) and adds @no-named-arguments. |
| src/wp-includes/class-wp-hook.php | Updates hook method annotations and refactors has_filter()/apply_filters() to keep types PHPStan-clean. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| * @param mixed $value The value to filter. | ||
| * @param list<mixed> $args Additional parameters to pass to the callback functions. | ||
| * This array is expected to include $value at index 0. |
There was a problem hiding this comment.
Then it should be typed as non-empty-list<mixed> here as well.
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5
The observation is right, but the earlier reply on this thread was not: non-empty-list<mixed> is not the correct fix here.
Narrowing WP_Hook::apply_filters() makes PHPStan flag WP_Hook::do_action() for passing list<mixed>, which cascades to do_action_ref_array() and then do_action_deprecated(). That cascade happens to introduce no new errors in core, but it would be inaccurate: wp-cron.php:191 calls do_action_ref_array( $hook, $v['args'] ), and both wp_schedule_event() and wp_schedule_single_event() default $args to array(). Every cron event scheduled without arguments therefore fires do_action_ref_array( $hook, array() ), which covers most core events, wp_version_check and wp_scheduled_delete among them.
The implementation does not require a non-empty array either. On the filter path, index 0 is only ever written — $args[0] = $value at class-wp-hook.php:353, guarded by ! $this->doing_action — and never read; on the action path it is not touched at all. The incoming value at index 0 matters only for count( $args ) at line 339, which is what drives the arity slicing.
So the type stays list<mixed>, and the docblock sentence has been reworded instead in cc601b9:
* @param mixed $value The value to filter.
* @param list<mixed> $args Additional parameters to pass to the callback functions.
* When filtering, this array is expected to include $value
* at index 0. When called for an action by WP_Hook::do_action(),
* it contains only the action arguments.
For the record, that sentence predates this PR; only its column alignment was touched here.
…mpty list. `apply_filters_ref_array()` reads `$args[0]` unconditionally, both for the early return when the hook has no callbacks and when passing the value to `WP_Hook`. `apply_filters_deprecated()` returns `$args[0]` when the hook is not registered, and otherwise forwards the array to `apply_filters_ref_array()`. In both cases an empty list is invalid, so `non-empty-list<mixed>` is the accurate type. PHPStan does not report possibly-missing offsets on general array types, so this does not resolve an error inside either function. It takes effect at the call sites, where passing a list that may be empty is now an `argument.type` error. The `do_action` variants are deliberately left as `list<mixed>`: nothing on that path reads index 0, and an empty array is a legitimate argument. `wp-cron.php` calls `do_action_ref_array( $hook, $v['args'] )`, and `wp_schedule_event()` and `wp_schedule_single_event()` both default `$args` to an empty array. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ex 0 when filtering. The docblock stated unconditionally that `$args` is expected to include `$value` at index 0, which does not hold for the action path: `WP_Hook::do_action()` calls the method with an empty string for `$value` and an array holding only the action arguments. The parameter stays `list<mixed>` rather than narrowing to `non-empty-list<mixed>`, because index 0 is only ever written on the filter path and never read, and is not touched at all on the action path. Its incoming value matters solely for the `count( $args )` that drives the arity slicing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This is stacked on #12441 and includes all of its commits. The only new commit here is b0d8b59, which resolves the remaining PHPStan level 10 errors in
WP_Hookon top of the type tightening done in that PR. @johnbillion: feel free to merge this into your branch if you want to fold it in; otherwise I'll rebase ontotrunkonce your PR lands.The new commit:
@return voidto the methods lacking return values.$argsarrays aslist<mixed>and narrows the$callbackparams ofremove_filter()/has_filter()tocallable, matchingadd_filter().0instead offalseto_wp_filter_build_unique_id()inhas_filter(), matching itsint $priorityparam. (The value is unused for the key, so this is behavior-neutral.)apply_filters()loop to assign the current priority to a local variable before storing it in$current_priority, bailing from the loop in the (impossible) case thatcurrent()returnsfalse. This keeps$current_priorityhonestly typed asarray<int, int>; previously, PHPStan sawint|falsebeing stored.With this,
composer phpstan src/wp-includes/class-wp-hook.phpreports no errors at level 10.Local
phpstan.neonoverridesTrac ticket: https://core.trac.wordpress.org/ticket/64896
Trac ticket: https://core.trac.wordpress.org/ticket/65817
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Fable 5
Used for: Diagnosing some of the PHPStan level 10 errors and drafting some of the fixes in the final commit; I reviewed, adjusted, and take responsibility for the changes.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Y1qB3ZWwGV2nBtdb9NgcJa