Discussion on the value of type checks for hooks

[ssnepenthe]

I wonder how much value these type checks provide... Or rather if they risk introducing new problems by giving developers a false sense of type safety.

WordPress has no built-in type enforcement mechanism for hooks so a misbehaving theme or plugin is free to return any arbitrary value via a filter regardless of the documented type.

Further - there is no protection against third party code triggering core hooks, so initial values aren't really safe either.

For this reason I still generally prefer to be very defensive in action/filter callbacks.

Unfortunately it seems that this plugin discourages type checks by raising various errors for redundant conditions, type contradictions, etc.

Consider the following example:

add_filter( 'the_content', function( $content ) {
    if ( ! is_string( $content ) ) {
        throw new RuntimeException();
    }

    return $content;
} );

Running psalm gives us the following:

ERROR: TypeDoesNotContainType - psalm-wp-test.php:14:12 - Type string for $content is always string (see https://psalm.dev/056)

If we modify the example so that types are documented via docblock or native types, psalm will report type contradiction errors.

This is of course a very contrived example - If a plugin is destroying your post content you have much bigger problems.

For a more real-world example - I recently ran into an issue using the 'pre_post_link' filter where a 3rd party plugin managed to trigger the filter with a stdClass instance despite the documented type being WP_Post.

Maybe it is just me and I am overthinking things?

I don't really have a solution to propose, just curious what anybody else thinks about this and if you've run into similar issues?

[mcaskill]

This issue has been an increasing pain, as well, in my recent projects, mostly originating from third-party WP plugins but even WP's own erroneous/outdated/invalid docblock types.

Disabling the default hooks kind of helps but isn't the most practical solution:

<useDefaultHooks value="false" />

I suppose the ideal would be to have every hook's parameters and returns to always be "mixed" and have their docblocks just be non-strict suggestions/nice-to-haves. Maybe filter out TypeDoesNotContainType as non-errors / convert them to a WordPress-centric type notice.

I'm reminded of TypeScript's any and unknown types:

• any turns off the type check errors. When a value is of type any, you can access any properties of it (which will in turn be of type any), call it like a function, assign it to (or from) a value of any type, or pretty much anything else that’s syntactically legal.
• unknown represents any value. It's similar to the any type, but is safer because it’s not legal to do anything with an unknown value.

[kkmuffme]

This is something that was an issue/discussion in WP core itself - to what extend should we protect/validate to ensure the types passed in what they are, and to what extent should we rely on people passing in the types you expect.

In the end, especially with PHPs native type system that is available now as of PHP 7/8, you have to rely on the types being what you expect (= what is declared), as otherwise PHP itself (with native types) would error out in the callback already. Additionally, not trusting the types, requires lots of additional type checking which makes the code extremely verbose and slower. Especially with WP core, we realized that over time the more checking and adjusting (= have people passed the correct type,...) we added, the MORE bugs were created, bc people just relied on WP reporting an error or automatically converting the type if something is wrong, leading to worse code in plugins.

As long as you ensure you always pass the correct types (which is what psalm does - it helps you in your codebase to ensure types are correct), you're fine. If you have a 3rd party plugin that does it wrong - that's an issue that needs to be addressed in that 3rd party plugin and not in your add_filter callback function.

You don't bring your own laboratory to the gas station to ensure that the gas nozzle actually pumps gas and not Diesel, do you? :-)

---

For any/unknown/mixed: this will creates tons of errors and unnecessary code. Run psalm on level 1/with mixed errors enabled and you'll see you're in for a world of pain if you do it mixed.

However, if you think that types should be lenient, changing the types to mixed is a simple PR and functionality that can be enabled with a config option. So feel free to create a PR for that (next week once we released a v5 compatible version :-)