From aa77ed2bfa3ab2c32f05450d98e57a3f07756c19 Mon Sep 17 00:00:00 2001 From: Gary Jones Date: Fri, 9 Oct 2026 22:41:16 +0100 Subject: [PATCH] fix: only check the filter callback's own returns AlwaysReturnInFilter looked at every return in the callback body, including those in nested closures and anonymous class methods. A callback that never returned passed when a nested closure did, and an intentional `return;` in a nested closure, such as an add_action() callback, was reported as the filter returning void. It also treated a return as conditional when an if wrapped the whole callback. A function inside a function_exists() check that returned only from try and catch was reported as missing a return, while the same function without the wrapper passed. --- .../Hooks/AlwaysReturnInFilterSniff.php | 42 ++++++++++--------- .../Hooks/AlwaysReturnInFilterUnitTest.inc | 34 +++++++++++++++ .../Hooks/AlwaysReturnInFilterUnitTest.php | 2 + 3 files changed, 59 insertions(+), 19 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php b/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php index 2793c7fc..bc354b56 100644 --- a/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php +++ b/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php @@ -11,6 +11,7 @@ use PHP_CodeSniffer\Util\Tokens; use PHPCSUtils\Utils\Arrays; +use PHPCSUtils\Utils\Conditions; use PHPCSUtils\Utils\FunctionDeclarations; use PHPCSUtils\Utils\TextStrings; use WordPressVIPMinimum\Sniffs\Sniff; @@ -230,13 +231,16 @@ private function processFunctionBody( $stackPtr ): void { $outsideConditionalReturn = 0; while ( $returnTokenPtr ) { - if ( $this->isInsideIfConditonal( $returnTokenPtr ) === false ) { - ++$outsideConditionalReturn; - } - if ( $this->isReturningVoid( $returnTokenPtr ) ) { - $message = 'Please, make sure that a callback to `%s` filter is returning void intentionally.'; - $data = [ $filterName ]; - $this->phpcsFile->addError( $message, $functionBodyScopeStart, 'VoidReturn', $data ); + // A return in a nested closure or function does not return from the callback. + if ( Conditions::getLastCondition( $this->phpcsFile, $returnTokenPtr, [ T_FUNCTION, T_CLOSURE ] ) === $stackPtr ) { + if ( $this->isInsideIfConditonal( $returnTokenPtr, $stackPtr ) === false ) { + ++$outsideConditionalReturn; + } + if ( $this->isReturningVoid( $returnTokenPtr ) ) { + $message = 'Please, make sure that a callback to `%s` filter is returning void intentionally.'; + $data = [ $filterName ]; + $this->phpcsFile->addError( $message, $functionBodyScopeStart, 'VoidReturn', $data ); + } } $returnTokenPtr = $this->phpcsFile->findNext( [ T_RETURN ], @@ -259,24 +263,24 @@ private function processFunctionBody( $stackPtr ): void { } /** - * Is the current token inside a conditional? + * Is the current token inside a conditional within the function? * - * @param int $stackPtr The position in the stack where the token was found. + * @param int $stackPtr The position in the stack where the token was found. + * @param int $functionPtr The position of the function the token belongs to. * * @return bool */ - private function isInsideIfConditonal( $stackPtr ): bool { - - // This check helps us in situations a class or a function is wrapped - // inside a conditional as a whole. Eg.: inside `class_exists`. - // Similar case may be a conditional closure. - $conditions = $this->tokens[ $stackPtr ]['conditions']; - $innermost = end( $conditions ); - if ( $innermost === T_FUNCTION || $innermost === T_CLOSURE ) { - return false; + private function isInsideIfConditonal( $stackPtr, $functionPtr ): bool { + + // Only look inside the function, so a class or a function wrapped inside + // a conditional as a whole does not count. Eg.: inside `function_exists`. + foreach ( $this->tokens[ $stackPtr ]['conditions'] as $conditionPtr => $conditionCode ) { + if ( $conditionPtr > $functionPtr && $conditionCode === T_IF ) { + return true; + } } - return in_array( T_IF, $conditions, true ); + return false; } /** diff --git a/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc b/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc index 086b0eab..b3eb4e5e 100644 --- a/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc @@ -242,6 +242,40 @@ add_filter( 'still_missing_return', function( $input ) { // Error. do_something( $input ); } ); +// A conditional wrapping the callback as a whole does not make its returns conditional. +if ( ! function_exists( 'wrapped_try_catch' ) ) { + function wrapped_try_catch( $input ) { // Ok. + try { + return do_something( $input ); + } catch ( Exception $e ) { + return $input; + } + } +} +add_filter( 'wrapped_try_catch', 'wrapped_try_catch' ); + +// A return in a nested closure does not return from the callback. +add_filter( 'nested_closure_return', function( $input ) { // Error. + $input = array_map( function( $item ) { + return $item; + }, $input ); +} ); + +add_filter( 'nested_closure_void_return', function( $input ) { // Ok. + add_action( 'init', function() { + return; + } ); + return $input; +} ); + +add_filter( 'nested_class_return', function( $input ) { // Error. + $object = new class() { + public function get() { + return 'value'; + } + }; +} ); + // Intentional parse error. This has to be the last test in the file! class parse_error_test { public function __construct() { diff --git a/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.php b/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.php index 9201985f..c954f095 100644 --- a/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.php +++ b/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.php @@ -33,6 +33,8 @@ public function getErrorList() { 188 => 1, 196 => 1, 241 => 1, + 258 => 1, + 271 => 1, ]; }