From cee609afefbfb0bfe038af22c9acda364756f5a3 Mon Sep 17 00:00:00 2001 From: David Binovec Date: Mon, 23 Apr 2018 09:21:57 +0000 Subject: [PATCH 1/6] Adding Filters/AlwaysReturn sniff The sniff is supposed to flag cases when a callback to a filter hook is not returning any value. --- .../Sniffs/Filters/AlwaysReturnSniff.php | 266 ++++++++++++++++++ .../Tests/Filters/AlwaysReturnUnitTest.inc | 93 ++++++ .../Tests/Filters/AlwaysReturnUnitTest.php | 40 +++ 3 files changed, 399 insertions(+) create mode 100644 WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php create mode 100644 WordPressVIPMinimum/Tests/Filters/AlwaysReturnUnitTest.inc create mode 100644 WordPressVIPMinimum/Tests/Filters/AlwaysReturnUnitTest.php diff --git a/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php b/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php new file mode 100644 index 00000000..5ec40689 --- /dev/null +++ b/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php @@ -0,0 +1,266 @@ +tokens = $phpcsFile->getTokens(); + + $this->phpcsFile = $phpcsFile; + + $functionName = $this->tokens[ $stackPtr ]['content']; + + if ( 'add_filter' !== $functionName ) { + return; + } + + $this->filterNamePtr = $this->phpcsFile->findNext( + array_merge( Tokens::$emptyTokens, array( T_OPEN_PARENTHESIS ) ), // types. + $stackPtr + 1, // start. + null, // end. + true, // exclude. + null, // value. + true // local. + ); + + if ( ! $this->filterNamePtr ) { + // Something is wrong. + return; + } + + $callbackPtr = $this->phpcsFile->findNext( + array_merge( Tokens::$emptyTokens, array( T_COMMA ) ), // types. + $this->filterNamePtr + 1, // start. + null, // end. + true, // exclude. + null, // value. + true // local. + ); + + if ( ! $callbackPtr ) { + // Something is wrong. + return; + } + + if ( 'PHPCS_T_CLOSURE' === $this->tokens[ $callbackPtr ]['code'] ) { + $this->processFunctionBody( $callbackPtr ); + } elseif ( 'T_ARRAY' === $this->tokens[ $callbackPtr ]['type'] ) { + $this->processArray( $callbackPtr ); + } elseif ( true === in_array( $this->tokens[ $callbackPtr ]['code'], Tokens::$stringTokens, true ) ) { + $this->processString( $callbackPtr ); + } + + } + + /** + * Process array. + * + * @param int $stackPtr The position in the stack where the token was found. + */ + private function processArray( $stackPtr ) { + + $previous = $this->phpcsFile->findPrevious( + Tokens::$emptyTokens, // types. + $this->tokens[ $stackPtr ]['parenthesis_closer'] - 1, // start. + null, // end. + true, // exclude. + null, // value. + false // local. + ); + + if ( true === in_array( T_CLASS, $this->tokens[ $stackPtr ]['conditions'], true ) ) { + $classPtr = array_search( T_CLASS, $this->tokens[ $stackPtr ]['conditions'], true ); + if ( $classPtr ) { + $classToken = $this->tokens[ $classPtr ]; + $this->processString( $previous, $classToken['scope_opener'], $classToken['scope_closer'] ); + return; + } + } + + $this->processString( $previous ); + + } + + /** + * Process string. + * + * @param int $stackPtr The position in the stack where the token was found. + */ + private function processString( $stackPtr, $start = 0, $end = null ) { + + $callbackFunctionName = substr( $this->tokens[ $stackPtr ]['content'], 1, -1 ); + + $callbackFunctionPtr = $this->phpcsFile->findNext( + Tokens::$functionNameTokens, // types. + $start, // start. + $end, // end. + false, // exclude. + $callbackFunctionName, // value. + false // local. + ); + + if ( ! $callbackFunctionPtr ) { + // We were not able to find the function callback in the file. + return; + } + + $this->processFunction( $callbackFunctionPtr, $start, $end ); + + } + + /** + * Process function. + * + * @param int $stackPtr The position in the stack where the token was found. + */ + private function processFunction( $stackPtr, $start = 0, $end = null ) { + + $functionName = $this->tokens[ $stackPtr ]['content']; + + $offset = $start; + while( $functionStackPtr = $this->phpcsFile->findNext( array( T_FUNCTION ), $offset, $end, false, null, false ) ) { + $functionNamePtr = $this->phpcsFile->findNext( Tokens::$emptyTokens, $functionStackPtr + 1, null, true, null, true ); + if ( T_STRING === $this->tokens[ $functionNamePtr ]['code'] ) { + if ( $this->tokens[ $functionNamePtr ]['content'] === $functionName ) { + $this->processFunctionBody( $functionStackPtr ); + return; + } + } + $offset = $functionStackPtr + 1; + } + } + + /** + * Process function's body + * + * @param int $stackPtr The position in the stack where the token was found. + * @param string $variableName Variable name. + */ + private function processFunctionBody( $stackPtr ) { + + $functionBodyScopeStart = $this->tokens[ $stackPtr ]['scope_opener']; + $functionBodyScopeEnd = $this->tokens[ $stackPtr ]['scope_closer']; + + $returnToken = $this->phpcsFile->findNext( + array( T_RETURN ), // types. + ( $functionBodyScopeStart + 1 ), // start. + $functionBodyScopeEnd, // end. + false, // exclude. + null, // value. + false // local. + ); + + $insideIfConditionalReturn = 0; + $outsideConditionalReturn = 0; + + while ( $returnToken ) { + if ( $this->isInsideIfConditonal( $returnToken ) ) { + $insideIfConditionalReturn++; + } else { + $outsideConditionalReturn++; + } + $returnToken = $this->phpcsFile->findNext( + array( T_RETURN ), // types. + ( $returnToken + 1 ), // start. + $functionBodyScopeEnd, // end. + false, // exclude. + null, // value. + false // local. + ); + } + + if ( $insideIfConditionalReturn > 0 && $outsideConditionalReturn === 0 ) { + $filterName = $this->tokens[ $this->filterNamePtr ]['content']; + $this->phpcsFile->AddWarning( sprintf( 'Please, make sure that a callback to %s filter is always returning some value.', $filterName ), $functionBodyScopeStart, 'missingReturnStatement' ); + } + + } + + /** + * Is the current token inside a conditional? + * + * @param int $stackPtr The position in the stack where the token was found. + * + * @return bool + */ + private function isInsideIfConditonal( $stackPtr ) { + + // This check helps us in situations a class or a function is wrapped + // inside a conditional as a whole. Eg.: inside `class_exists`. + if ( T_FUNCTION === end( $this->tokens[ $stackPtr ]['conditions'] ) ) { + return false; + } + + // Loop over the array of conditions and look for an IF. + reset( $this->tokens[ $stackPtr ]['conditions'] ); + + if ( true === array_key_exists( 'conditions', $this->tokens[ $stackPtr ] ) + && true === is_array( $this->tokens[ $stackPtr ]['conditions'] ) + && false === empty( $this->tokens[ $stackPtr ]['conditions'] ) + ) { + foreach( $this->tokens[ $stackPtr ]['conditions'] as $tokenPtr => $tokenCode ) { + if ( T_IF === $this->tokens[ $stackPtr ]['conditions'][ $tokenPtr ] ) { + return true; + } + } + } + return false; + } +} diff --git a/WordPressVIPMinimum/Tests/Filters/AlwaysReturnUnitTest.inc b/WordPressVIPMinimum/Tests/Filters/AlwaysReturnUnitTest.inc new file mode 100644 index 00000000..97017af9 --- /dev/null +++ b/WordPressVIPMinimum/Tests/Filters/AlwaysReturnUnitTest.inc @@ -0,0 +1,93 @@ + => + */ + public function getErrorList() { + return array(); + } + + /** + * Returns the lines where warnings should occur. + * + * @return array => + */ + public function getWarningList() { + return array( + 15 => 1, + 49 => 1, + 88 => 1, + ); + } + +} // End class. From 82057d0df53bcf4e8cb4a19752e4692b96d90bf2 Mon Sep 17 00:00:00 2001 From: David Binovec Date: Mon, 23 Apr 2018 09:42:13 +0000 Subject: [PATCH 2/6] Adding check for void return from filter callback, improving variable naming, and taking conditional closures into account (similarly to classes inside conditionals) --- .../Sniffs/Filters/AlwaysReturnSniff.php | 41 +++++++++++++++---- .../Tests/Filters/AlwaysReturnUnitTest.inc | 12 +++++- .../Tests/Filters/AlwaysReturnUnitTest.php | 1 + 3 files changed, 46 insertions(+), 8 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php b/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php index 5ec40689..6852593f 100644 --- a/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php +++ b/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php @@ -195,10 +195,12 @@ private function processFunction( $stackPtr, $start = 0, $end = null ) { */ private function processFunctionBody( $stackPtr ) { + $filterName = $this->tokens[ $this->filterNamePtr ]['content']; + $functionBodyScopeStart = $this->tokens[ $stackPtr ]['scope_opener']; $functionBodyScopeEnd = $this->tokens[ $stackPtr ]['scope_closer']; - $returnToken = $this->phpcsFile->findNext( + $returnTokenPtr = $this->phpcsFile->findNext( array( T_RETURN ), // types. ( $functionBodyScopeStart + 1 ), // start. $functionBodyScopeEnd, // end. @@ -210,15 +212,18 @@ private function processFunctionBody( $stackPtr ) { $insideIfConditionalReturn = 0; $outsideConditionalReturn = 0; - while ( $returnToken ) { - if ( $this->isInsideIfConditonal( $returnToken ) ) { + while ( $returnTokenPtr ) { + if ( $this->isInsideIfConditonal( $returnTokenPtr ) ) { $insideIfConditionalReturn++; } else { $outsideConditionalReturn++; } - $returnToken = $this->phpcsFile->findNext( + if ( $this->isReturningVoid( $returnTokenPtr ) ) { + $this->phpcsFile->AddWarning( sprintf( 'Please, make sure that a callback to `%s` filter is returnin void intentionally.', $filterName ), $functionBodyScopeStart, 'voidReturn' ); + } + $returnTokenPtr = $this->phpcsFile->findNext( array( T_RETURN ), // types. - ( $returnToken + 1 ), // start. + ( $returnTokenPtr + 1 ), // start. $functionBodyScopeEnd, // end. false, // exclude. null, // value. @@ -227,8 +232,7 @@ private function processFunctionBody( $stackPtr ) { } if ( $insideIfConditionalReturn > 0 && $outsideConditionalReturn === 0 ) { - $filterName = $this->tokens[ $this->filterNamePtr ]['content']; - $this->phpcsFile->AddWarning( sprintf( 'Please, make sure that a callback to %s filter is always returning some value.', $filterName ), $functionBodyScopeStart, 'missingReturnStatement' ); + $this->phpcsFile->AddWarning( sprintf( 'Please, make sure that a callback to `%s` filter is always returning some value.', $filterName ), $functionBodyScopeStart, 'missingReturnStatement' ); } } @@ -248,6 +252,11 @@ private function isInsideIfConditonal( $stackPtr ) { return false; } + // Similar case may be a conditional closure + if ( 'PHPCS_T_CLOSURE' === end( $this->tokens[ $stackPtr ]['conditions'] ) ) { + return false; + } + // Loop over the array of conditions and look for an IF. reset( $this->tokens[ $stackPtr ]['conditions'] ); @@ -263,4 +272,22 @@ private function isInsideIfConditonal( $stackPtr ) { } return false; } + + private function isReturningVoid( $stackPtr ) { + + $nextToReturnTokenPtr = $this->phpcsFile->findNext( + array( Tokens::$emptyTokens ), // types. + ( $stackPtr + 1 ), // start. + null, // end. + true, // exclude. + null, // value. + false // local. + ); + + if ( T_SEMICOLON === $this->tokens[ $nextToReturnTokenPtr ]['code'] ) { + return true; + } + + return false; + } } diff --git a/WordPressVIPMinimum/Tests/Filters/AlwaysReturnUnitTest.inc b/WordPressVIPMinimum/Tests/Filters/AlwaysReturnUnitTest.inc index 97017af9..3fb5ce08 100644 --- a/WordPressVIPMinimum/Tests/Filters/AlwaysReturnUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Filters/AlwaysReturnUnitTest.inc @@ -90,4 +90,14 @@ add_filter( 'bad_example_closure', function() { return 'hey'; } // Missing else return; -} ); \ No newline at end of file +} ); + +add_filter( 'another_bad_example_closure', function() { + return; +} ); + +if ( 1 === 1 ) { + add_filter( 'another_good_example_closure', function () { + return 'hello'; + } ); +} \ No newline at end of file diff --git a/WordPressVIPMinimum/Tests/Filters/AlwaysReturnUnitTest.php b/WordPressVIPMinimum/Tests/Filters/AlwaysReturnUnitTest.php index db58cea3..92638d36 100644 --- a/WordPressVIPMinimum/Tests/Filters/AlwaysReturnUnitTest.php +++ b/WordPressVIPMinimum/Tests/Filters/AlwaysReturnUnitTest.php @@ -34,6 +34,7 @@ public function getWarningList() { 15 => 1, 49 => 1, 88 => 1, + 95 => 1, ); } From e467dd3b35fbb04ab250f35a0b88b0c33aff6852 Mon Sep 17 00:00:00 2001 From: Tom J Nowell Date: Thu, 9 Aug 2018 01:26:27 +0100 Subject: [PATCH 3/6] Update the PHPDoc to correctly indicate what the sniff does --- WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php b/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php index 6852593f..f901b1cd 100644 --- a/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php +++ b/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php @@ -11,9 +11,7 @@ use PHP_CodeSniffer_Tokens as Tokens; /** - * This sniff validates a propper usage of pre_get_posts action callback - * - * It looks for cases when the WP_Query object is being modified without checking for WP_Query::is_main_query(). + * This sniff validates that filters always return a value * * @package VIPCS\WordPressVIPMinimum */ From bfdd4df9809c3f7476a64b2972406055b616f054 Mon Sep 17 00:00:00 2001 From: Tom J Nowell Date: Thu, 9 Aug 2018 01:37:02 +0100 Subject: [PATCH 4/6] PHPCS fixes to the always return in filter sniff --- .../Sniffs/Filters/AlwaysReturnSniff.php | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php b/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php index f901b1cd..261511f8 100644 --- a/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php +++ b/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php @@ -140,6 +140,8 @@ private function processArray( $stackPtr ) { * Process string. * * @param int $stackPtr The position in the stack where the token was found. + * @param int $start The start of the token. + * @param int $end The end of the token. */ private function processString( $stackPtr, $start = 0, $end = null ) { @@ -167,13 +169,15 @@ private function processString( $stackPtr, $start = 0, $end = null ) { * Process function. * * @param int $stackPtr The position in the stack where the token was found. + * @param int $start The start of the token. + * @param int $end The end of the token. */ private function processFunction( $stackPtr, $start = 0, $end = null ) { $functionName = $this->tokens[ $stackPtr ]['content']; $offset = $start; - while( $functionStackPtr = $this->phpcsFile->findNext( array( T_FUNCTION ), $offset, $end, false, null, false ) ) { + while ( $functionStackPtr = $this->phpcsFile->findNext( array( T_FUNCTION ), $offset, $end, false, null, false ) ) { $functionNamePtr = $this->phpcsFile->findNext( Tokens::$emptyTokens, $functionStackPtr + 1, null, true, null, true ); if ( T_STRING === $this->tokens[ $functionNamePtr ]['code'] ) { if ( $this->tokens[ $functionNamePtr ]['content'] === $functionName ) { @@ -189,7 +193,6 @@ private function processFunction( $stackPtr, $start = 0, $end = null ) { * Process function's body * * @param int $stackPtr The position in the stack where the token was found. - * @param string $variableName Variable name. */ private function processFunctionBody( $stackPtr ) { @@ -208,7 +211,7 @@ private function processFunctionBody( $stackPtr ) { ); $insideIfConditionalReturn = 0; - $outsideConditionalReturn = 0; + $outsideConditionalReturn = 0; while ( $returnTokenPtr ) { if ( $this->isInsideIfConditonal( $returnTokenPtr ) ) { @@ -229,7 +232,7 @@ private function processFunctionBody( $stackPtr ) { ); } - if ( $insideIfConditionalReturn > 0 && $outsideConditionalReturn === 0 ) { + if ( 0 < $insideIfConditionalReturn && 0 === $outsideConditionalReturn ) { $this->phpcsFile->AddWarning( sprintf( 'Please, make sure that a callback to `%s` filter is always returning some value.', $filterName ), $functionBodyScopeStart, 'missingReturnStatement' ); } @@ -262,7 +265,7 @@ private function isInsideIfConditonal( $stackPtr ) { && true === is_array( $this->tokens[ $stackPtr ]['conditions'] ) && false === empty( $this->tokens[ $stackPtr ]['conditions'] ) ) { - foreach( $this->tokens[ $stackPtr ]['conditions'] as $tokenPtr => $tokenCode ) { + foreach ( $this->tokens[ $stackPtr ]['conditions'] as $tokenPtr => $tokenCode ) { if ( T_IF === $this->tokens[ $stackPtr ]['conditions'][ $tokenPtr ] ) { return true; } @@ -271,6 +274,13 @@ private function isInsideIfConditonal( $stackPtr ) { return false; } + /** + * Is the token returning void + * + * @param int $stackPtr The position in the stack where the token was found. + * + * @return bool + **/ private function isReturningVoid( $stackPtr ) { $nextToReturnTokenPtr = $this->phpcsFile->findNext( From b5dc486e182948f62989e039f9cd4a4a8a89b424 Mon Sep 17 00:00:00 2001 From: Tom J Nowell Date: Thu, 9 Aug 2018 01:40:24 +0100 Subject: [PATCH 5/6] More PHPCS fixes for the filters always return sniff --- WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php b/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php index 261511f8..e0487d0a 100644 --- a/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php +++ b/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php @@ -192,7 +192,7 @@ private function processFunction( $stackPtr, $start = 0, $end = null ) { /** * Process function's body * - * @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. */ private function processFunctionBody( $stackPtr ) { @@ -253,7 +253,7 @@ private function isInsideIfConditonal( $stackPtr ) { return false; } - // Similar case may be a conditional closure + // Similar case may be a conditional closure. if ( 'PHPCS_T_CLOSURE' === end( $this->tokens[ $stackPtr ]['conditions'] ) ) { return false; } @@ -262,8 +262,8 @@ private function isInsideIfConditonal( $stackPtr ) { reset( $this->tokens[ $stackPtr ]['conditions'] ); if ( true === array_key_exists( 'conditions', $this->tokens[ $stackPtr ] ) - && true === is_array( $this->tokens[ $stackPtr ]['conditions'] ) - && false === empty( $this->tokens[ $stackPtr ]['conditions'] ) + && true === is_array( $this->tokens[ $stackPtr ]['conditions'] ) + && false === empty( $this->tokens[ $stackPtr ]['conditions'] ) ) { foreach ( $this->tokens[ $stackPtr ]['conditions'] as $tokenPtr => $tokenCode ) { if ( T_IF === $this->tokens[ $stackPtr ]['conditions'][ $tokenPtr ] ) { From 2f682b862c69bf920737d8f7b087d56f2491c08d Mon Sep 17 00:00:00 2001 From: Tom J Nowell Date: Thu, 9 Aug 2018 02:11:07 +0100 Subject: [PATCH 6/6] fixes the final PHPCS issue in the filters always return sniff --- WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php b/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php index e0487d0a..95e6fe19 100644 --- a/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php +++ b/WordPressVIPMinimum/Sniffs/Filters/AlwaysReturnSniff.php @@ -262,7 +262,7 @@ private function isInsideIfConditonal( $stackPtr ) { reset( $this->tokens[ $stackPtr ]['conditions'] ); if ( true === array_key_exists( 'conditions', $this->tokens[ $stackPtr ] ) - && true === is_array( $this->tokens[ $stackPtr ]['conditions'] ) + && true === is_array( $this->tokens[ $stackPtr ]['conditions'] ) && false === empty( $this->tokens[ $stackPtr ]['conditions'] ) ) { foreach ( $this->tokens[ $stackPtr ]['conditions'] as $tokenPtr => $tokenCode ) {