Sniff for correct usage of add_action vs add_filter

[johnbillion]

Today we discovered that some code under development was breaking functionality related to the attachment_fields_to_save hook.

It turns out the bug was caused by a callback being incorrectly registered as an action (with add_action()) instead of a filter (with add_filter()). This meant that all filters after this particular callback were receiving a null value instead of the WP_Post object.

It's a bit of a long shot, but it would be great if WPCS could be aware of which hooks in WordPress are actions, and which are filters, and raise an error if add_action() is used instead of add_filter().

[grappler]

I think the sniff could be relatively simple if we have list of WordPress filters and actions. It would need to be decided what happens if the hook is not part of the list as it is not part of WordPress core. Maybe https://github.com/WordPress/phpdoc-parser could help with the list.

[jrfnl]

Just checking - was the actual issue caused by the use of add_action() or was it caused by the hooked in function not returning the expected value ... ?

[JDGrimes]

Just checking - was the actual issue caused by the use of add_action() or was it caused by the hooked in function not returning the expected value ... ?

Yeah, I've been bitten by that before. But that would really be beyond WPCS's ability to detect in some cases.

[johnbillion]

Yes the actual bug was caused because the action callback was not returning a value. The same problem would occur if a filter callback didn't return a value, but it would be more likely to be picked up during code review in that case, I think.

A sniff which could detect when a filter callback doesn't contain a return statement on the final line would be great, but that's probably a bit too much to ask of a static analyser.

[johnbillion]

Gonna close this off as something that realistically isn't practical to implement.

[johnbillion]

I'm going to re-open this as the original issue can be quite difficult for a developer to spot. I don't think WPCS should go so far as detecting return values, but detecting incorrect usage of add_action() versus add_filter() for core hooks ought to be doable.

I recently created a library that lists core's actions and filters that could be used by WPCS: https://github.com/johnbillion/wp-hooks

[dingo-d]

That would be awesome! I've run into this problem yesterday, where I ended up one hour debugging the issue where I typed add_action( 'wp_handle_upload', ... instead of add_filter( 'wp_handle_upload', ..., and my image uploads were failing and I had no idea why.

:+1: from me 🙂

[jrfnl]

Loosely related to #1803.

Any sniff for this would need a list of hooks and that list could be generated by running PHPCS over Core.

A sister-sniff checking for use of deprecated hooks would also be good.

[johnbillion]

Any sniff for this would need a list of hooks and that list could be generated by running PHPCS over Core.

This is what the wp-hooks library provides.

A sister-sniff checking for use of deprecated hooks would also be good.

Yep, agreed. Let's keep this focused for now though.

[jrfnl]

Just saw a comment somewhere in a Slack about someone having to fix some bugs related to exactly this. Good reminder that a sniff which checks that the correct hook-in function is used would be a good addition, if for no other reason than to remind the developer that actions !== filters.

[johnbillion]

If someone wants to pick this up, my wp-hooks library is actively maintained and would be a good source for the list of core actions and filters.

Aside: I'm considering how best to provide common aliases for hooks that include dynamic portions in their names, eg. {$old_status}_to_{$new_status}.