From 0c3afa145e7ed7ca939071cb52b9caeb9ff294a2 Mon Sep 17 00:00:00 2001 From: Gary Jones Date: Sat, 10 Oct 2026 17:23:45 +0100 Subject: [PATCH 1/2] fix: require every if/else path to return in filters AlwaysReturnInFilter treated only an if as conditional, so a return in an else or elseif counted as one that always runs. A callback that returned only from its else, or from an if/elseif chain with no else, passed even though one path returned nothing. An if chain now only counts as returning when it ends in an else and every branch returns. Other control structures, such as loops, switch and try, keep the previous leniency, where any return inside them counts. --- .../Hooks/AlwaysReturnInFilterSniff.php | 121 ++++++++++++++++-- .../Hooks/AlwaysReturnInFilterUnitTest.inc | 67 ++++++++++ .../Hooks/AlwaysReturnInFilterUnitTest.php | 3 + 3 files changed, 177 insertions(+), 14 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php b/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php index bc354b56..7a89f96c 100644 --- a/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php +++ b/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php @@ -10,6 +10,7 @@ namespace WordPressVIPMinimum\Sniffs\Hooks; use PHP_CodeSniffer\Util\Tokens; +use PHPCSUtils\Tokens\Collections; use PHPCSUtils\Utils\Arrays; use PHPCSUtils\Utils\Conditions; use PHPCSUtils\Utils\FunctionDeclarations; @@ -228,14 +229,9 @@ private function processFunctionBody( $stackPtr ): void { $functionBodyScopeEnd ); - $outsideConditionalReturn = 0; - while ( $returnTokenPtr ) { // 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 ]; @@ -249,7 +245,7 @@ private function processFunctionBody( $stackPtr ): void { ); } - if ( $outsideConditionalReturn === 0 ) { + if ( $this->alwaysReturns( $functionBodyScopeStart, $functionBodyScopeEnd ) === false ) { if ( $this->hasTerminatingStatement( $functionBodyScopeStart, $functionBodyScopeEnd ) ) { $message = 'The callback for the `%s` filter uses a terminating statement (`exit`, `die`, or `throw`) instead of returning a value. Filter callbacks should always return a value.'; $data = [ $filterName ]; @@ -263,26 +259,123 @@ private function processFunctionBody( $stackPtr ): void { } /** - * Is the current token inside a conditional within the function? + * Does every path through the code between the scope opener and closer reach a return? + * + * Only if, elseif and else are followed as separate paths. A return inside any other + * control structure, such as a loop, switch or try, counts as reaching a return. * - * @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. + * @param int $scopeOpener The scope opener. + * @param int $scopeCloser The scope closer. * * @return bool */ - private function isInsideIfConditonal( $stackPtr, $functionPtr ): bool { + private function alwaysReturns( $scopeOpener, $scopeCloser ): bool { + + for ( $i = $scopeOpener + 1; $i < $scopeCloser; $i++ ) { + if ( $this->tokens[ $i ]['code'] === T_RETURN ) { + return true; + } + + if ( isset( $this->tokens[ $i ]['scope_condition'], $this->tokens[ $i ]['scope_opener'], $this->tokens[ $i ]['scope_closer'] ) === false + || $this->tokens[ $i ]['scope_condition'] !== $i + ) { + continue; + } + + if ( $this->isNestedScope( $i ) ) { + // A return in a nested function or class does not return from the callback. + $i = $this->tokens[ $i ]['scope_closer']; + continue; + } + + if ( $this->tokens[ $i ]['code'] === T_IF ) { + // An if chain only returns on every path when it has an else and every branch returns. + $branches = $this->getIfChainBranches( $i ); + $lastBranch = end( $branches ); + $returns = $this->tokens[ $lastBranch ]['code'] === T_ELSE; + foreach ( $branches as $branch ) { + $returns = $returns && $this->alwaysReturns( $this->tokens[ $branch ]['scope_opener'], $this->tokens[ $branch ]['scope_closer'] ); + } + + if ( $returns ) { + return true; + } - // 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 ) { + $i = $this->tokens[ $lastBranch ]['scope_closer']; + continue; + } + + if ( $this->alwaysReturns( $this->tokens[ $i ]['scope_opener'], $this->tokens[ $i ]['scope_closer'] ) ) { return true; } + + $i = $this->tokens[ $i ]['scope_closer']; } return false; } + /** + * Get the if, elseif and else branches of an if chain. + * + * A branch without braces ends the chain, as it has no scope to follow. + * + * @param int $ifPtr The position of the if. + * + * @return array The positions of the branches, in order. + */ + private function getIfChainBranches( $ifPtr ): array { + + $branches = []; + $branch = $ifPtr; + while ( isset( $this->tokens[ $branch ]['scope_opener'], $this->tokens[ $branch ]['scope_closer'] ) ) { + $branches[] = $branch; + if ( $this->tokens[ $branch ]['code'] === T_ELSE ) { + break; + } + + // With the alternative syntax, a branch closes on the next elseif or else. + $next = $this->tokens[ $branch ]['scope_closer']; + if ( $this->tokens[ $next ]['code'] !== T_ELSEIF && $this->tokens[ $next ]['code'] !== T_ELSE ) { + $next = $this->phpcsFile->findNext( Tokens::$emptyTokens, $next + 1, null, true ); + } + + if ( $next !== false + && $this->tokens[ $next ]['code'] === T_ELSE + && isset( $this->tokens[ $next ]['scope_opener'] ) === false + ) { + // An `else if` is an else without braces, followed by an if. + $next = $this->phpcsFile->findNext( Tokens::$emptyTokens, $next + 1, null, true ); + if ( $next === false || $this->tokens[ $next ]['code'] !== T_IF ) { + break; + } + } elseif ( $next === false + || ( $this->tokens[ $next ]['code'] !== T_ELSEIF && $this->tokens[ $next ]['code'] !== T_ELSE ) + ) { + break; + } + + $branch = $next; + } + + return $branches; + } + + /** + * Is the token a nested function or class, whose code does not belong to the callback? + * + * @param int $stackPtr The position in the stack where the token was found. + * + * @return bool + */ + private function isNestedScope( $stackPtr ): bool { + + $code = $this->tokens[ $stackPtr ]['code']; + + return isset( $this->tokens[ $stackPtr ]['scope_closer'] ) + && ( isset( Collections::functionDeclarationTokens()[ $code ] ) || isset( Tokens::$ooScopeTokens[ $code ] ) ); + } + /** * Check whether the function body contains an exit, die, or throw statement. * diff --git a/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc b/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc index b3eb4e5e..20a28444 100644 --- a/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc @@ -276,6 +276,73 @@ add_filter( 'nested_class_return', function( $input ) { // Error. }; } ); +// A return in an else or elseif is only reached on some paths. +add_filter( 'else_only_return', function( $input ) { // Error. + if ( is_admin() ) { + $input = 'admin'; + } else { + return $input; + } +} ); + +add_filter( 'elseif_without_else', function( $input ) { // Error. + if ( is_admin() ) { + return 'admin'; + } elseif ( is_feed() ) { + return 'feed'; + } +} ); + +add_filter( 'every_branch_returns', function( $input ) { // Ok. + if ( is_admin() ) { + return 'admin'; + } elseif ( is_feed() ) { + return 'feed'; + } else { + return $input; + } +} ); + +add_filter( 'every_branch_returns_else_if', function( $input ) { // Ok. + if ( is_admin() ) { + return 'admin'; + } else if ( is_feed() ) { + return 'feed'; + } else { + return $input; + } +} ); + +add_filter( 'every_branch_returns_alternative_syntax', function( $input ) { // Ok. + if ( is_admin() ) : + return 'admin'; + elseif ( is_feed() ) : + return 'feed'; + else : + return $input; + endif; +} ); + +add_filter( 'alternative_syntax_missing_else', function( $input ) { // Error. + if ( is_admin() ) : + return 'admin'; + elseif ( is_feed() ) : + return 'feed'; + endif; +} ); + +add_filter( 'nested_chain_in_else', function( $input ) { // Ok. + if ( is_admin() ) { + return 'admin'; + } else { + if ( is_feed() ) { + return 'feed'; + } else { + return $input; + } + } +} ); + // 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 c954f095..fc49c233 100644 --- a/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.php +++ b/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.php @@ -35,6 +35,9 @@ public function getErrorList() { 241 => 1, 258 => 1, 271 => 1, + 280 => 1, + 288 => 1, + 326 => 1, ]; } From b483022154476428a0c289c55b3be4e71cf82d82 Mon Sep 17 00:00:00 2001 From: Gary Jones Date: Sat, 10 Oct 2026 17:24:29 +0100 Subject: [PATCH 2/2] fix: ignore exit and throw in nested filter closures An exit, die or throw anywhere in a filter callback's body turned the missing return error into the softer terminating statement warning, even when it sat in a nested closure or arrow function, such as a shutdown action. The callback itself never terminates there, so it still fails to return a value on that path. --- .../Sniffs/Hooks/AlwaysReturnInFilterSniff.php | 18 ++++++++++++------ .../Hooks/AlwaysReturnInFilterUnitTest.inc | 17 +++++++++++++++++ .../Hooks/AlwaysReturnInFilterUnitTest.php | 2 ++ 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php b/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php index 7a89f96c..3e72f1f2 100644 --- a/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php +++ b/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php @@ -386,13 +386,19 @@ private function isNestedScope( $stackPtr ): bool { */ private function hasTerminatingStatement( $scopeStart, $scopeEnd ): bool { - $terminatingPtr = $this->phpcsFile->findNext( - [ T_EXIT, T_THROW ], - $scopeStart + 1, - $scopeEnd - ); + for ( $i = $scopeStart + 1; $i < $scopeEnd; $i++ ) { + if ( $this->isNestedScope( $i ) ) { + // An exit or throw in a nested function or class does not end the callback. + $i = $this->tokens[ $i ]['scope_closer']; + continue; + } - return $terminatingPtr !== false; + if ( $this->tokens[ $i ]['code'] === T_EXIT || $this->tokens[ $i ]['code'] === T_THROW ) { + return true; + } + } + + return false; } /** diff --git a/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc b/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc index 20a28444..44e0d764 100644 --- a/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc @@ -343,6 +343,23 @@ add_filter( 'nested_chain_in_else', function( $input ) { // Ok. } } ); +// An exit or throw in a nested closure does not end the callback. +add_filter( 'nested_closure_throw', function( $input ) { // Error. + if ( is_admin() ) { + return $input; + } + add_action( 'shutdown', function() { + throw new \RuntimeException( 'Failed' ); + } ); +} ); + +add_filter( 'nested_arrow_function_throw', function( $input ) { // Error. + if ( is_admin() ) { + return $input; + } + $fail = fn() => throw new \RuntimeException( 'Failed' ); +} ); + // 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 fc49c233..e2eaad9b 100644 --- a/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.php +++ b/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.php @@ -38,6 +38,8 @@ public function getErrorList() { 280 => 1, 288 => 1, 326 => 1, + 347 => 1, + 356 => 1, ]; }