report invalid arguments for do_action/apply_filters invocation too

[kkmuffme]

Report invalid arguments (= arguments that do not match the declared PHPDoc type) passed to do_action and apply_filters

$foo = rand( 0, 10 );

/**
 * @param string $foo
 * @param int
 */
$foo = apply_filters( 'bar', $foo, 'hello' );

Will report like:

Argument 2 of apply_filters expects string, int<1, 10> provided Argument 3 of apply_filters expects int, "hello" provided

[kkmuffme]

Will fix the failing test in a separate PR in the coming days, as this is a separate bug I had on my to do list already. Don't merge this yet. Will rebase later.

[joehoyle]

Will this also catch a incompability between a registered core hook? For example, if I try:

do_action( 'save_post', 1 );

This can cause bugs, as anything hooking save_post can expect a second param of type WP_Post

[kkmuffme]

Yes, it can report this. I added a option for this:

<plugins>
    <pluginClass class="PsalmWordPress\Plugin">
        <requireAllParams value="true" />
    </pluginClass>
</plugins>

It will make all parameters of this filter/action that have ever been set in a PHPDoc required.

I could however also change this to:

• require at least x parameters (if phpdoc has 5, we will always require at least x, as long as phpdoc params are > x)
• OR: require at least PHPDoc - y parameters (if phpdoc has 5, we will require at least 5-y parameters to be set, but at least 1 for filter/0 for action) (or of course also add both possibilities, if you think that makes sense)

Unfortunately, it has tons of issues atm, due to bad hook type overwrites, which I fixed in https://github.com/humanmade/psalm-plugin-wordpress/pull/32 Once that's merged, I will rebase it to complete this.

[joehoyle]

@kkmuffme ok cool-- I think requireAllParams needs renaming, it's not clear what it means. Also, why make this an option? I can't think of a valid reason to call do_action / apply_filters with params that don't match a registered hook.

[kkmuffme]

Yeah renaming is definitely a good idea, bc you misunderstood it too :-) It's actually for what you said here https://github.com/humanmade/psalm-plugin-wordpress/pull/31#issuecomment-1108640445

It makes all params that were ever declared for this filter mandatory. e.g. if we have:

$foo = apply_filters( 'foo', $hello, $world );

It would give an error when you do this:

$foo = apply_filters( 'foo', $hello, $world );

// this will give an error, bc you didn't pass all params
$foo = apply_filters( 'foo', $hello );

[joehoyle]

@kkmuffme haha ok, my bad! In that case though, it seems like this should be atleast default-on; why would we want to disable a check like that?

Also, does it need to be an option, can people use the Supress docblock in cases where they are doing something spooky and not passing all values to a filter / action?

[kkmuffme]

Why it's not default on: Bc for most filters that allow a variable number of args, only 1-2 are mandatory. So always requiring the dev to pass all args (with a default value + documenting that) was a bit much to do. I can still set it default on, but this will mean for most people who start with this psalm plugin that their error list is at least twice as long (= twice as likely to just not use it)

How about allParamsMandatory? Honestly, I'm happy with any suggestion from your end on this.

Also, does it need to be an option, can people use the Supress docblock in cases where they are doing something spooky and not passing all values to a filter / action?

No. Either it's mandatory or it's not. There's no in between. As I cannot think of any case where you'd want that to have it suppressed. (but I could add that later on anyway, if someone really needs that)

[joehoyle]

Bc for most filters that allow a variable number of args, only 1-2 are mandatory. So always requiring the dev to pass all args (with a default value + documenting that) was a bit much to do.

Ahh I guess I had not realized this! Do you have any examples of a filter with variable number of args?

[kkmuffme]

e.g. https://wpml.org/documentation/support/wpml-coding-api/wpml-hooks-reference/#hook-1215380 (there are a couple in WooCommerce and WP core too I guess, I just had this one at the ready as I had to fix an issue with it)

[joehoyle]

Ahh I see, is there anything in the source code where we can mark it as optional and do checks based on that? Really to make things typesafe we want to do both:

• Alert when you're missing an non-optional param from your callback
• Alert when a param is not marked as optional in a callback when the filter makes it optional

It seems that would be the only correct way to have it type complete.

[kkmuffme]

We can currently report if you pass the wrong number of args to an add_action/add_filter callback (the 4th argument) AND (separately) also if that function does not have the correct number of args (too few/too many without defaults)

To be honest: I have developed it much further, tested it on some huge WP codebases, filled in a few todo gaps and added some more enhancements (e.g. for hooks called by cron functions like wp_schedule_event or also partial support for do_action/apply_filter hooks with dynamic/variable hook names)

There are additional issues (= false positives reported by psalm without and with this plugin), which need to be addressed to get fully correct results (except for https://github.com/humanmade/psalm-plugin-wordpress/issues/12 which is impossible atm) I fixed those already. I'm happy to PR them, just not until the current 2 PRs are merged, bc I don't have time to separate the existing PR stuff out again. If you want, I can also submit a massive PR with all changes.

[mcaskill]

Is this pull request still relevant and/or should be included for Psalm 4?

[kkmuffme]

Still relevant, will be for v5 though, since my code has diverted quite a bit from this PR already, to handle additional cases,... and partially uses v5 API in my fork.