Sniff WordPressVIPMinimum.Hooks.AlwaysReturnInFilter.MissingReturnStatement fails on abstract methods

[kevinfodness]

Bug Description

If you define an abstract class that sets up a filter in its constructor and sets the callback to an abstract method in the class which will be defined by implementing classes, the WordPressVIPMinimum.Hooks.AlwaysReturnInFilter.MissingReturnStatement sniff causes phpcs to crash, because it's trying to evaluate the return statement in an abstract method.

The specific error message being returned is this:

An error occurred during processing; checking has been aborted. The error message was: Undefined index: scope_opener in /path/to/.composer/vendor/automattic/vipwpcs/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php on line 194 (Internal.Exception)

Minimal Code Snippet

abstract class good_example_abstract_class { // Ok.
	public function __construct() {
		add_filter( 'good_example_class_filter', [ $this, 'class_filter' ] );
	}

	abstract public function class_filter( $param );
}

class good_example_abstract_class_implementation { // Ok.
	public function class_filter( $param ) {
		if ( 1 === 1 ) {
			if ( 1 === 0 ) {
				return 'whoops';
			} else {
				return 'here!';
			}
		}
		return 'This is Okay';
	}
}

Error Code

No error code should be triggered.

Environment

Use php -v and composer show to get versions.

Question	Answer
PHP version	7.3.0
PHP_CodeSniffer version	3.5.6
VIPCS version	2.2.0

Additional Context (optional)

Fixed in PR #581

Tested Against master branch?

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

[jrfnl]

@kevinfodness Thanks for reporting this.

I agree that that Undefined index: scope_opener error should be avoided. I'm not so sure whether the sniff should bow out/stay silent though.
It might be better for it to throw a warning as a reviewer should in that case manually check that all child classes which implement the method contain a return statement.

If that direction would be chosen though, some additional code is needed to make the distinction between "scope opener missing because of a parse error/tokenizer error/live coding" and "scope opener missing because it is an abstract method".

@rebeccahum @GaryJones What do you think ?

[rebeccahum]

That makes sense to me, as we do already have some sniffs that flag for "manual inspection" due to static analysis limitations.

[kevinfodness]

That sounds like a fine solution to me also.

[jrfnl]

I've updated @kevinfodness's PR with the proposed additional change.