AlwaysReturnInFilter: bow out on exit/die/throw ?

[jrfnl]

trafficstars

Bug Description

Generally speaking, functions hooked into filters should never throw exceptions or call exit() or die() and should always return a value.

However, there are (valid) edge cases in which a function hooked into a filter can legitimately use this functionality.

In my opinion, if such code is encountered, the sniff should bow out and count the throw/exit statement as a return statement.

Forbidding the use of exceptions/exit()/die() in functions used by filters is not the job of this sniff. If so desired, this should be addressed in a separate sniff (or by a separate error code).

@rebeccahum @GaryJones What do you think ?

Minimal Code Snippet

class Foo {
	public function __construct() {
		add_filter( 'redirect_canonical', [ $this, 'redirect' ] );
	}

	public function redirect( $redirect_url, $requested_url ) {
		$redirect  = true; // In real code this would be determined by logic.
		if ( $redirect === false ) {
			return $redirect_url;
		}

		wp_redirect( $redirect_url );
		exit;
	}
}

Error Code

The above code sample will currently result in:

6 | ERROR   | Please, make sure that a callback to `'redirect_canonical'` filter is always returning some value.
  |         | (WordPressVIPMinimum.Hooks.AlwaysReturnInFilter.MissingReturnStatement)

.. while in my opinion this code should not be flagged like this.

Environment

Question	Answer
PHP version	PHP 8.2-beta1
PHP_CodeSniffer version	bleeding edge
VIPCS version	bleeding edge

Tested Against master branch?

• [x] I have verified the issue still exists in the master branch of VIPCS.
• [x] I have verified the issue still exists in the develop branch of VIPCS.

[jrfnl]

Just ran into this one again. @GaryJones Got an opinion on this report ?

[GaryJones]

I think a new sniff makes sense.

I'm not sure I agree about the edge cases though - they are intentionally mis-using a filter callback as an action callback, so they should expect correct violations about not including a return.

[jrfnl]

I'm not sure I agree about the edge cases though - they are intentionally mis-using a filter callback as an action callback, so they should expect correct violations about not including a return.

You got a point. I will re-evaluate the instances I came across to double-check what they do and report back.

Forbidding the use of exceptions/exit()/die() in functions used by filters is not the job of this sniff. If so desired, this should be addressed in a separate sniff (or by a separate error code).

I think a new sniff makes sense.

In that case, I think we should open a separate issue for that ? For now (until performance friendly helper utilities are available in PHPCSUtils), I think adding a separate error code for this to this sniff might be the best way forward.