Add type checking for hook callbacks in add_action and add_filter

[johnbillion]

I'm planning on working on some rules that check the use of add_action() and add_filter() when the action or filter is from WordPress core. The wp-hooks library facilitates this, and the wp-hooks-generator library facilitates this for hooks in third party plugins and themes (although I'm only concentrating on the WordPress core hooks for now).

I'll open a PR once I have something working.

---

When I call add_action( 'foo', $callback ) or add_filter( 'foo', $callback ):

• The signature of the callback function should be checked against the parameters that get passed to the hook
• The number used in the $accepted_args parameter should match the number of parameters that the callback accepts
• A filter should not be used as an action, nor vice versa
• The callback for add_filter must return a value with a type which is compatible with the type passed to it
• The callback for add_action must return void

Notes:

• It's fine for the callback function to accept fewer parameters than are passed to the hook (but see point above about $accepted_args)
• There is no requirement to have a docblock on the callback because it's the job of PHPStan to figure out parameter types and raise errors due to implicit mixed, etc
• We could also look up the literal hook name, eg plugin_action_links_{$plugin_file} and see if we get a match for dynamic hook names (stretch goal)

[szepeviktor]

High expectations!
Thank you.

[herndlm]

This was completed, wasn't it? 🎉

[szepeviktor]

I do not follow the development :)

[johnbillion]

This was only partially completed, I'll open a follow-up issue with the details.

[menno-ll]

Hi there,

Awesome that this feature is added! We've just updated to the latest version for some of our projects.
However, we've encountered an issue, which is mixed return types.
We see that when using a filter which has a return type of mixed declared in the PHPDoc, phpstan does not detect any return type, throwing this error: Filter callback return statement is missing.

This happens on a function like this:

/**
 *
 * @param mixed $some_mixed_var
 * @return mixed
 */
function some_function_with_mixed_return_type ( $some_mixed_var ) {
    $some_mixed_var = is_string( $some_mixed_var ) ? 'I am a string' : $some_mixed_var;
    return $some_mixed_var;
}
add_filter( 'some_filter_name', 'some_function_with_mixed_return_type' );

I hope you guys can take a look if we can resolve this :).
That would save us from ignoring these errors which are actually important.

CC @sheila-levellevel

[menno-ll]

Also one extra thing, which is more an opinion i would like to start a little discussion about.

When creating a callback function for an action, with this new set of rules, a return type of void is forced for that function.
However, because of it beeing an action, nothing would happen if you would let the callback function return a value.
So why do I bring this up?
It's because in some occasions, you would like to use the callback function in another place in your project. And there, you actually want to use the return variable.
And this is now not possible.

An example below:

add_action( 'save_post', 'set_custom_meta' );
function set_custom_meta( int $post_id ): bool {
    return (bool) update_post_meta( $post_id, 'some_meta', 'some_value' );
}

function some_function_somewhere_else_in_the_website(): void {
    $meta_updated = set_custom_meta( 1 );
    if ( $meta_updated ) {
        // Do something
    } else {
        // Do something
    }
}

As you can see, this example is not possible, because we are now forced to return a void for the set_custom_meta function.
A way around this issue is to create a separate function for both occasions, one returning a void, and the other returning the boolean. But that sounds as a workaround that's not needed.
I would love to hear you guy's opinions.
Thanks in advance!

CC @sheila-levellevel

[johnbillion]

@menno-ll Are you using version 1.1.5? Issues with mixed should have been fixed in that version. If so, can you open a new issue please?

Regarding returning a value from an action, I can see how it's beneficial to reuse functions but at the same time a developer looking at your set_custom_meta() function would, rightfully, expect its return value to be used, which is not true, and can cause confusion. This is the sort of strictness that PHPStan rules are intended to address. I recommend introducing a simple wrapper function that calls set_custom_meta() and use that in your action.

[menno-ll]

Hi @johnbillion

Thank you for the quick response!

You were right, we made the discovery with the mixed return type the day before v1.1.5 came out, and did not notice there was a new version where it was fixed. And yesterday I did not notice that v1.1.5 contained the fix, as it was not mentioned in the description of that release.
I've upgraded to v1.1.5 and can confirm this resolved the issue. Thanks for pointing it out! No new issue is needed.

Regarding the reuse of a function used as an action callback, the way you describe with creating a wrapper function was also what i had in mind to bypass the phpstan error. I just wanted to make you aware of this scenario we found in our code, so it could be taken into consideration. If writing a wrapper function is the way to go, we will modify it to comply with the new rule, or ignore that rule for our projects, depending on the opinion of my collegues.

Thanks again for helping out!

[westonruter]

Just chiming in here to note how I believe this would have caught a fatal error from occurring in a Performance Lab plugin: WordPress/performance#1755. The issue is that the Image Placeholders plugin added a wp_prepare_attachment_for_js filter which is defined as:

/**
 * Filters the attachment data prepared for JavaScript.
 *
 * @since 3.5.0
 *
 * @param array       $response   Array of prepared attachment data. See {@see wp_prepare_attachment_for_js()}.
 * @param WP_Post     $attachment Attachment object.
 * @param array|false $meta       Array of attachment meta data, or false if there is none.
 */
return apply_filters( 'wp_prepare_attachment_for_js', $response, $attachment, $meta );

However, the filter callback in Image Placeholders incorrectly presumed the third $meta filter argument is always an array when it can also be false, so this was fixed to apply this change to remove the array type hint:

- function dominant_color_prepare_attachment_for_js( $response, WP_Post $attachment, array $meta ): array {
+ function dominant_color_prepare_attachment_for_js( $response, WP_Post $attachment, $meta ): array {

Do I understand correctly that this issue here of type checking all of the callback args is in scope?

[johnbillion]

It should be in scope yes. I haven't had a chance to work on this recently, but I did work on the prerequisite PHP API to the wp-hooks info.