From 1feffcae0df4fddc32ee53311d2d64e069d8fc22 Mon Sep 17 00:00:00 2001 From: kevinfodness Date: Tue, 8 Sep 2020 15:52:02 -0400 Subject: [PATCH 1/6] Ignore return values when a filter callback is abstract --- .../Hooks/AlwaysReturnInFilterSniff.php | 9 +++++++++ .../Hooks/AlwaysReturnInFilterUnitTest.inc | 20 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php b/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php index f5ea5734..6b3689ca 100644 --- a/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php +++ b/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php @@ -175,6 +175,15 @@ private function processFunction( $stackPtr, $start = 0, $end = null ) { */ private function processFunctionBody( $stackPtr ) { + /** + * Stop if the constructor doesn't have a body, like when it is abstract. + * + * @see https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Standards/Generic/Sniffs/NamingConventions/ConstructorNameSniff.php#L87-L90 + */ + if ( false === isset( $this->tokens[ $stackPtr ]['scope_closer'] ) ) { + return; + } + $argPtr = $this->phpcsFile->findNext( array_merge( Tokens::$emptyTokens, [ T_STRING, T_OPEN_PARENTHESIS ] ), $stackPtr + 1, diff --git a/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc b/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc index 4f9d6d00..be74c9e3 100644 --- a/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc @@ -172,3 +172,23 @@ class bad_example_class_short_array { // Error. } } +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'; + } +} From adccc63c628e9440244a9d2ed3500ef90f5a3517 Mon Sep 17 00:00:00 2001 From: Gary Jones Date: Wed, 9 Sep 2020 08:17:54 +0100 Subject: [PATCH 2/6] Avoid yoda condition --- WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php b/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php index 6b3689ca..14cbb55b 100644 --- a/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php +++ b/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php @@ -180,7 +180,7 @@ private function processFunctionBody( $stackPtr ) { * * @see https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Standards/Generic/Sniffs/NamingConventions/ConstructorNameSniff.php#L87-L90 */ - if ( false === isset( $this->tokens[ $stackPtr ]['scope_closer'] ) ) { + if ( isset( $this->tokens[ $stackPtr ]['scope_closer'] ) === false ) { return; } From 015b7402d720aae3746611103867768ca09a52c4 Mon Sep 17 00:00:00 2001 From: Gary Jones Date: Wed, 9 Sep 2020 09:22:12 +0100 Subject: [PATCH 3/6] DocBlock fix --- WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php b/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php index 14cbb55b..c546693a 100644 --- a/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php +++ b/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php @@ -176,7 +176,7 @@ private function processFunction( $stackPtr, $start = 0, $end = null ) { private function processFunctionBody( $stackPtr ) { /** - * Stop if the constructor doesn't have a body, like when it is abstract. + * Stop if the function doesn't have a body, like when it is abstract. * * @see https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Standards/Generic/Sniffs/NamingConventions/ConstructorNameSniff.php#L87-L90 */ From 8e4077d32b8aceb45cd645bb4e6074af05f101dd Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 9 Sep 2020 17:21:44 +0200 Subject: [PATCH 4/6] AlwaysReturnInFilter: update unit tests. * Adjust the "abstract method" test to: 1. Expect a warning. 2. Prevent the hook in getting confused with other functions in the same test file. * Remove the "abstract method implementation" test as it wasn't testing anything. 1. The sniff does not look for child classes, so wouldn't examine that code snippet anyway. Adding this functionality is not that useful either as in most cases, the child class will not be in the same file as the abstract parent class. 2. And even if the sniff did examine it, it would still not recognize it as a child class as the class doesn't `extend` the abstract. * Add a test for a typical case where declared functions do not have a scope opener/closer due to a tokenizer bug. PR squizlabs/PHP_CodeSniffer 3066 is open upstream to fix this. * Add a "live coding" test case. --- .../Hooks/AlwaysReturnInFilterUnitTest.inc | 29 ++++++++++--------- .../Hooks/AlwaysReturnInFilterUnitTest.php | 4 ++- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc b/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc index be74c9e3..4d29e003 100644 --- a/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.inc @@ -172,23 +172,26 @@ class bad_example_class_short_array { // Error. } } -abstract class good_example_abstract_class { // Ok. +abstract class warning_filter_points_to_abstract_method { public function __construct() { - add_filter( 'good_example_class_filter', [ $this, 'class_filter' ] ); + add_filter( 'good_example_abstract_class', [ $this, 'abstract_method' ] ); } - abstract public function class_filter( $param ); + abstract public function abstract_method( $param ); // Warning. } -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'; +class tokenizer_bug_test { + public function __construct() { + add_filter( 'tokenizer_bug', [ $this, 'tokenizer_bug' ] ); } + + public function tokenizer_bug( $param ): namespace\Foo {} // Ok (tokenizer bug in PHPCS < 3.5.7/3.6.0). } + +// Intentional parse error. This has to be the last test in the file! +class parse_error_test { + public function __construct() { + add_filter( 'parse_error', [ $this, 'parse_error' ] ); + } + + public function parse_error( $param ) // Ok, parse error ignored. diff --git a/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.php b/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.php index 747e0b83..c47e881c 100644 --- a/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.php +++ b/WordPressVIPMinimum/Tests/Hooks/AlwaysReturnInFilterUnitTest.php @@ -40,7 +40,9 @@ public function getErrorList() { * @return array => */ public function getWarningList() { - return []; + return [ + 180 => 1, + ]; } } From 4356274fd99676fa47a8efc0b36dfe497f5ee9e8 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 9 Sep 2020 17:33:20 +0200 Subject: [PATCH 5/6] AlwaysReturnInFilter: differentiate between abstract method and parse error This implements the suggestion made in https://github.com/Automattic/VIP-Coding-Standards/issues/580#issuecomment-689119006 If the method a filter callback points to is an abstract method, a warning will be thrown asking for manual inspection of the child class implementations of the abstract method. In case of parse or tokenizer error, the sniff will bow out and stay silent. --- .../Hooks/AlwaysReturnInFilterSniff.php | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php b/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php index c546693a..2cda2587 100644 --- a/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php +++ b/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php @@ -175,12 +175,18 @@ private function processFunction( $stackPtr, $start = 0, $end = null ) { */ private function processFunctionBody( $stackPtr ) { - /** - * Stop if the function doesn't have a body, like when it is abstract. - * - * @see https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Standards/Generic/Sniffs/NamingConventions/ConstructorNameSniff.php#L87-L90 - */ - if ( isset( $this->tokens[ $stackPtr ]['scope_closer'] ) === false ) { + $filterName = $this->tokens[ $this->filterNamePtr ]['content']; + + $methodProps = $this->phpcsFile->getMethodProperties( $stackPtr ); + if ( $methodProps['is_abstract'] === true ) { + $message = 'The callback for the `%s` filter hook-in points to an abstract method. Please, make sure that all child class implementations of this abstract method always return a value.'; + $data = [ $filterName ]; + $this->phpcsFile->addWarning( $message, $stackPtr, 'AbstractMethod', $data ); + return; + } + + if ( isset( $this->tokens[ $stackPtr ]['scope_opener'], $this->tokens[ $stackPtr ]['scope_closer'] ) === false ) { + // Live coding, parse or tokenizer error. return; } @@ -198,8 +204,6 @@ private function processFunctionBody( $stackPtr ) { return; } - $filterName = $this->tokens[ $this->filterNamePtr ]['content']; - $functionBodyScopeStart = $this->tokens[ $stackPtr ]['scope_opener']; $functionBodyScopeEnd = $this->tokens[ $stackPtr ]['scope_closer']; From 6c190d2b698dace8e14542870d53b03033f8244e Mon Sep 17 00:00:00 2001 From: Gary Jones Date: Sun, 13 Sep 2020 10:36:43 +0100 Subject: [PATCH 6/6] Update WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php Co-authored-by: Rebecca Hum <16962021+rebeccahum@users.noreply.github.com> --- WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php b/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php index 2cda2587..718349df 100644 --- a/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php +++ b/WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php @@ -179,7 +179,7 @@ private function processFunctionBody( $stackPtr ) { $methodProps = $this->phpcsFile->getMethodProperties( $stackPtr ); if ( $methodProps['is_abstract'] === true ) { - $message = 'The callback for the `%s` filter hook-in points to an abstract method. Please, make sure that all child class implementations of this abstract method always return a value.'; + $message = 'The callback for the `%s` filter hook-in points to an abstract method. Please ensure that child class implementations of this method always return a value.'; $data = [ $filterName ]; $this->phpcsFile->addWarning( $message, $stackPtr, 'AbstractMethod', $data ); return;