Review the WordPressVIPMinimum.Hooks.AlwaysReturnInFilter sniff

[jrfnl]

Review the WordPressVIPMinimum.Hooks.AlwaysReturnInFilter sniff for the following in as far as relevant to that sniff:

• [ ] Code style independent sniffing / Correct handling of quirky code Typical things to add tests for and verify correct handling of:
  • [ ] Nested function/closure declarations
  • [ ] Nested class declarations
  • [ ] Comments in unexpected places
  • [ ] Variables being assigned to via list statements
  • [ ] Multiline text strings
  • [ ] Text strings provided via heredoc/nowdoc
  • [ ] Use of short open tags
  • [ ] Using PHP close tag as end of statement
  • [ ] Inline control structures (without braces)
• [ ] Code simplifications which can be made using PHPCSUtils
• [ ] Sniff stability improvements which can be made using PHPCSUtils
• [ ] Correct handling of modern PHP code Typical things to add tests for and verify correct handling of (where applicable):
  • [ ] PHP 5.0 Try/catch/finally (PHP 5.5) and exceptions
  • [ ] PHP 5.3 Namespaced code vs code in the global namespace
  • [ ] PHP 5.3 Use import statements, incl aliasing
  • [ ] PHP 5.3 Short ternaries
  • [ ] PHP 5.3 Closures, incl closure use
  • [ ] PHP 5.4 Short arrays
  • [ ] PHP 5.5 Class name resolution using ::class
  • [ ] PHP 5.5 List in foreach
  • [ ] PHP 5.5/7.0 Generators using yield and yield from
  • [ ] PHP 5.6 Constant scalar expressions
  • [ ] PHP 5.6 Importing via use function/const
  • [ ] PHP 7.0 Null coalesce
  • [ ] PHP 7.0 Anonymous classes
  • [ ] PHP 7.0 Scalar and return type declarations
  • [ ] PHP 7.0 Group use statements
  • [ ] PHP 7.1 Short lists
  • [ ] PHP 7.1 Keyed lists
  • [ ] PHP 7.1 Multi-catch
  • [ ] PHP 7.1 Nullable types
  • [ ] PHP 7.3 List reference assignments
  • [ ] PHP 7.4 arrow functions
  • [ ] PHP 7.4 numeric literals with underscores
  • [ ] PHP 7.4 null coalesce equals
  • [ ] PHP 7.4 Typed properties
  • [ ] Various versions: trailing comma's in function calls, group use, function declarations, closure use etc

Other:

• [ ] Review violation error vs warning
• [ ] Review violation severity
• [ ] Review violation message, consider adding a link
• [ ] Check open issues related to the sniff
• [ ] Review PHPDoc comments

Sniff basics, but changes need to be lined up for next major release:

• [ ] Inappropriate use of public properties (#234)
• [ ] Modular error codes (unique error code for each distinct message)

Once PHPCS/PHPCSUtils supports this:

• [ ] PHP 8.0 Constructor property promotion
• [ ] PHP 8.0 Union types
• [ ] PHP 8.0 match expressions
• [ ] PHP 8.0 Nullsafe operator
• [ ] PHP 8.0 Named arguments
• [ ] PHP 8.0 Single token namespaced names

[jrfnl]

I've done an initial review of this sniff and the PreGetPosts sniff while fixing #358.

These sniffs can use quite some love. The age of the sniff is clearly showing through as code past the PHP 4 age is barely taken into account. Time to bring it into the modern age :smile:

Findings:

• The logic of the two sniffs is supposedly partially the same, but has diverged where bug fixes applied to one haven't been applied to the other.
• The sniffs rely far too heavily on token walking based on flawed logic and presumptions of how code is written, instead of using utility functions from PHPCS/WPCS (or future: PHPCSUtils). This makes the sniffs unstable and buggy.
  • Some examples of bugs/inefficiencies for this sniff:
  • A function declared to return a reference - function &foo() - would not be examined as the sniff thinks the reference is for the first parameter being passed by reference. (false negatives)
  • A function where the first parameter is declared to be passed by reference, but which also has a type declaration - function foo( array &$param ) -, would be examined, even though it doesn't need to be. (false positives)
  • Compound filter names or function names are not taken into account. Think add_filter( 'filter', 'prefix' . $type . 'function_name' ) would lead the sniff to search for a function called function_name. If that function exists, this could lead to false positives, but independently of that, the sniff should bow out from searching any further when a compound name is encountered as it will not be able to reliably match it. (efficiency)
  • The sniff searches the current class, even when it can be known in advance that the function won't be found. (efficiency) For callbacks passed as an array, a check for $this, self/static::class, get_class() and some more variants should be done before walking the whole class.
  • The sniff misses support for filters in traits and anonymous classes. This could lead to false positives as the function search is not limited to the OO scope, but will search the whole file.
  • A function search of the whole file, will include examining methods in a class within that same file, even though the search is for a global function, which could lead to false positives/misidentifying the target function.
  • Namespaced functions are not handled at all. This goes for a potential MyNamespace\add_filter() being seen as the global add_filter()function, but also for the callbacks.
  • Similarly, a call to $myObject->add_filter() will also trigger the sniff. while again, this is not the global WP function.
  • Nested function/class declarations are not handled correctly. The outer scope is taken as the search scope, while it should be the inner scope.
  • Support for PHP 7.4 arrow functions as callback should be added.
  • The sniff has memory leaks due to using end() and reset() on large nested arrays.
  • The sniff listens to 12 tokens, while only one can ever be a match. etc etc

Recommendations

• Create an abstract sniff containing the duplicate logic and only have the sniff specific logic in the individual sniffs.
• Base the abstract sniff on the namespace and use statement aware version of the AbstractFunctionParametersSniff which is to be added to PHPCSUtils.
• Add tons more unit tests.
• Rewrite the sniff specific logic based on PHPCSUtils.

All in all, this sniff (and the PreGetPosts sniff) basically need a complete rewrite with modern code in mind. This is not an indictment of the principle of the sniff, it is just an old sniff which hasn't kept up with the changes in the PHP landscape around it.

Timeline

I'd recommend moving both this issue as well as the PreGetPosts issue out of the 2.2.0 milestone and scheduling the rewrite for a later release when the above mentioned abstract will be available.

Note: this abstract is currently not available in PHPCSUtils and still needs work before it can be pulled to PHPCSUtils.

Pertinent bugs can be patched up with quick fixes in the mean time if and when reported.

[jrfnl]

Been thinking about this sniff some more.

I think the wider WP community can benefit from this sniff, so once it is improved, this sniff would be a very strong candidate for moving to WPCS.

I also think that a generic "function call with callback parameter" abstract sniff in PHPCSUtils may not be a bad idea. This abstract could then be the base for this sniff.

Future scope for this sniff/the abstract (after the initial round of improvements):

• Awareness of native PHP functions used in callbacks. If a native PHP function is used, walking the whole file/class searching for that function declaration is quite unnecessary.

  // This will automatically removed falsey values from an array.
  add_filter( 'filter_an_array', 'array_filter' );

• Awareness of WP native functions used in callbacks. Again, if a WP native function is used, walking the whole file/class searching for the declaration is unnecessary. This is loosely related to some open issues in WPCS about adding lists with, for instance, WP native functions as traits to WPCS. Once such lists are available, this sniff could use them.

  add_filter( 'filter_boolean', '__return_false' );

[jrfnl]

From #783 - another thing which should be improved:

Looking at the sniff, I believe the intention was to only flag the "return outside condition missing" when not all control structure paths had a return statement, but this was never really properly checked as the only control structures taken into account are if control structures.

I believe it would be good to improve the sniff to handle more control structures (switch, while etc) and to not throw the "return outside condition missing" error if all possible paths have a return statement ... .