From 1b3d82e3c35e2cb7bf7117556b78a8a8c01309dd Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 14 Sep 2023 05:30:34 +0200 Subject: [PATCH 1/4] Tools/BrainMonkeyRaceCondition: simplify/use PHPCSUtils in more places --- .../Tools/BrainMonkeyRaceConditionSniff.php | 33 ++++--------------- 1 file changed, 7 insertions(+), 26 deletions(-) diff --git a/Yoast/Sniffs/Tools/BrainMonkeyRaceConditionSniff.php b/Yoast/Sniffs/Tools/BrainMonkeyRaceConditionSniff.php index 57e02283..5ea6c19f 100644 --- a/Yoast/Sniffs/Tools/BrainMonkeyRaceConditionSniff.php +++ b/Yoast/Sniffs/Tools/BrainMonkeyRaceConditionSniff.php @@ -3,8 +3,11 @@ namespace YoastCS\Yoast\Sniffs\Tools; use PHP_CodeSniffer\Util\Tokens; +use PHPCSUtils\Tokens\Collections; +use PHPCSUtils\Utils\Conditions; use PHPCSUtils\Utils\FunctionDeclarations; use PHPCSUtils\Utils\PassedParameters; +use PHPCSUtils\Utils\Scopes; use PHPCSUtils\Utils\TextStrings; use WordPressCS\WordPress\Sniff; @@ -46,41 +49,19 @@ public function process_token( $stackPtr ) { $prevNonEmpty = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true ); if ( $prevNonEmpty === false - || $this->tokens[ $prevNonEmpty ]['code'] === \T_DOUBLE_COLON - || $this->tokens[ $prevNonEmpty ]['code'] === \T_OBJECT_OPERATOR - || $this->tokens[ $prevNonEmpty ]['type'] === 'T_NULLSAFE_OBJECT_OPERATOR' + || isset( Collections::objectOperators()[ $this->tokens[ $prevNonEmpty ]['code'] ] ) || $this->tokens[ $prevNonEmpty ]['code'] === \T_FUNCTION ) { // Method call or function declaration, not a function call. return; } - // Make sure the token is in a class method. - if ( empty( $this->tokens[ $stackPtr ]['conditions'] ) ) { + $functionToken = Conditions::getLastCondition( $this->phpcsFile, $stackPtr, [ \T_FUNCTION ] ); + if ( $functionToken === false ) { return; } - $conditions = $this->tokens[ $stackPtr ]['conditions']; - $conditions = \array_reverse( $conditions, true ); - - $seenOO = false; - $seenFn = false; - $functionToken = false; - - foreach ( $conditions as $token => $condition ) { - if ( $seenFn === true ) { - // First condition after the function condition MUST be OO, otherwise it's not a method. - $seenOO = isset( Tokens::$ooScopeTokens[ $condition ] ); - break; - } - - if ( $condition === \T_FUNCTION ) { - $functionToken = $token; - $seenFn = true; - } - } - - if ( $seenFn === false || $seenOO === false || $functionToken === false ) { + if ( Scopes::isOOMethod( $this->phpcsFile, $functionToken ) === false ) { return; } From 6ea3d9ba362b47c254f8d5d8f86a3d1c2e25ca3a Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 14 Sep 2023 05:49:37 +0200 Subject: [PATCH 2/4] Tools/BrainMonkeyRaceCondition: decouple from WPCS sniff .... as extending the WPCS sniff is now no longer needed, as the utility functions we used from that sniff before, have all been replaced with utilities from PHPCSUtils. --- .../Tools/BrainMonkeyRaceConditionSniff.php | 59 ++++++++++--------- 1 file changed, 32 insertions(+), 27 deletions(-) diff --git a/Yoast/Sniffs/Tools/BrainMonkeyRaceConditionSniff.php b/Yoast/Sniffs/Tools/BrainMonkeyRaceConditionSniff.php index 5ea6c19f..c7e84ea9 100644 --- a/Yoast/Sniffs/Tools/BrainMonkeyRaceConditionSniff.php +++ b/Yoast/Sniffs/Tools/BrainMonkeyRaceConditionSniff.php @@ -2,6 +2,8 @@ namespace YoastCS\Yoast\Sniffs\Tools; +use PHP_CodeSniffer\Files\File; +use PHP_CodeSniffer\Sniffs\Sniff; use PHP_CodeSniffer\Util\Tokens; use PHPCSUtils\Tokens\Collections; use PHPCSUtils\Utils\Conditions; @@ -9,7 +11,6 @@ use PHPCSUtils\Utils\PassedParameters; use PHPCSUtils\Utils\Scopes; use PHPCSUtils\Utils\TextStrings; -use WordPressCS\WordPress\Sniff; /** * Sniff to detect a particular nasty race condition which can occur in tests using the BrainMonkey utilities. @@ -17,8 +18,9 @@ * @link https://github.com/Yoast/yoastcs/issues/264 * * @since 2.3.0 + * @since 3.0.0 This sniff no longer extends the WPCS abstract Sniff class. */ -final class BrainMonkeyRaceConditionSniff extends Sniff { +final class BrainMonkeyRaceConditionSniff implements Sniff { /** * Returns an array of tokens this test wants to listen for. @@ -30,43 +32,46 @@ public function register() { } /** - * Processes a sniff when one of its tokens is encountered. + * Processes this test, when one of its tokens is encountered. * - * @param int $stackPtr The position of the current token in the stack. + * @param File $phpcsFile The file being scanned. + * @param int $stackPtr The position of the current token in the stack passed in $tokens. * * @return void */ - public function process_token( $stackPtr ) { - if ( \strtolower( $this->tokens[ $stackPtr ]['content'] ) !== 'expect' ) { + public function process( File $phpcsFile, $stackPtr ) { + $tokens = $phpcsFile->getTokens(); + + if ( \strtolower( $tokens[ $stackPtr ]['content'] ) !== 'expect' ) { return; } - $nextNonEmpty = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); - if ( $nextNonEmpty === false || $this->tokens[ $nextNonEmpty ]['code'] !== \T_OPEN_PARENTHESIS ) { + $nextNonEmpty = $phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); + if ( $nextNonEmpty === false || $tokens[ $nextNonEmpty ]['code'] !== \T_OPEN_PARENTHESIS ) { // Definitely not a function call. return; } - $prevNonEmpty = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true ); + $prevNonEmpty = $phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true ); if ( $prevNonEmpty === false - || isset( Collections::objectOperators()[ $this->tokens[ $prevNonEmpty ]['code'] ] ) - || $this->tokens[ $prevNonEmpty ]['code'] === \T_FUNCTION + || isset( Collections::objectOperators()[ $tokens[ $prevNonEmpty ]['code'] ] ) + || $tokens[ $prevNonEmpty ]['code'] === \T_FUNCTION ) { // Method call or function declaration, not a function call. return; } - $functionToken = Conditions::getLastCondition( $this->phpcsFile, $stackPtr, [ \T_FUNCTION ] ); + $functionToken = Conditions::getLastCondition( $phpcsFile, $stackPtr, [ \T_FUNCTION ] ); if ( $functionToken === false ) { return; } - if ( Scopes::isOOMethod( $this->phpcsFile, $functionToken ) === false ) { + if ( Scopes::isOOMethod( $phpcsFile, $functionToken ) === false ) { return; } // Check that this is an expect() for one of the hook functions. - $param = PassedParameters::getParameter( $this->phpcsFile, $stackPtr, 1, 'function_name' ); + $param = PassedParameters::getParameter( $phpcsFile, $stackPtr, 1, 'function_name' ); if ( empty( $param ) ) { return; } @@ -74,19 +79,19 @@ public function process_token( $stackPtr ) { $expected = Tokens::$emptyTokens; $expected[] = \T_CONSTANT_ENCAPSED_STRING; - $hasUnexpected = $this->phpcsFile->findNext( $expected, $param['start'], ( $param['end'] + 1 ), true ); + $hasUnexpected = $phpcsFile->findNext( $expected, $param['start'], ( $param['end'] + 1 ), true ); if ( $hasUnexpected !== false ) { return; } - $text = $this->phpcsFile->findNext( Tokens::$emptyTokens, $param['start'], ( $param['end'] + 1 ), true ); - $textContent = TextStrings::stripQuotes( $this->tokens[ $text ]['content'] ); + $text = $phpcsFile->findNext( Tokens::$emptyTokens, $param['start'], ( $param['end'] + 1 ), true ); + $textContent = TextStrings::stripQuotes( $tokens[ $text ]['content'] ); if ( $textContent !== 'apply_filters' && $textContent !== 'do_action' ) { return; } // Now walk the contents of the function declaration to see if we can find the other function call. - if ( isset( $this->tokens[ $functionToken ]['scope_opener'], $this->tokens[ $functionToken ]['scope_closer'] ) === false ) { + if ( isset( $tokens[ $functionToken ]['scope_opener'], $tokens[ $functionToken ]['scope_closer'] ) === false ) { // We don't know the start or end of the function. return; } @@ -96,33 +101,33 @@ public function process_token( $stackPtr ) { $targetContent = 'expectapplied'; } - $start = $this->tokens[ $functionToken ]['scope_opener']; - $end = $this->tokens[ $functionToken ]['scope_closer']; + $start = $tokens[ $functionToken ]['scope_opener']; + $end = $tokens[ $functionToken ]['scope_closer']; for ( $i = $start; $i < $end; $i++ ) { - if ( $this->tokens[ $i ]['code'] !== \T_STRING ) { + if ( $tokens[ $i ]['code'] !== \T_STRING ) { continue; } - if ( \strtolower( $this->tokens[ $i ]['content'] ) !== $targetContent ) { + if ( \strtolower( $tokens[ $i ]['content'] ) !== $targetContent ) { continue; } // Make sure it is a function call. - $next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $i + 1 ), null, true ); - if ( $next === false || $this->tokens[ $next ]['code'] !== \T_OPEN_PARENTHESIS ) { + $next = $phpcsFile->findNext( Tokens::$emptyTokens, ( $i + 1 ), null, true ); + if ( $next === false || $tokens[ $next ]['code'] !== \T_OPEN_PARENTHESIS ) { continue; } // Okay, we have found the race condition. Throw error. $message = 'The %s() test method contains both a call to Monkey\Functions\expect( %s ), as well as a call to %s(). This causes a race condition which will cause the tests to fail. Only use one of these in a test.'; $data = [ - FunctionDeclarations::getName( $this->phpcsFile, $functionToken ), - $this->tokens[ $text ]['content'], + FunctionDeclarations::getName( $phpcsFile, $functionToken ), + $tokens[ $text ]['content'], ( $targetContent === 'expectdone' ) ? 'Monkey\Actions\expectDone' : 'Monkey\Filters\expectApplied', ]; - $this->phpcsFile->addError( $message, $functionToken, 'Found', $data ); + $phpcsFile->addError( $message, $functionToken, 'Found', $data ); break; } } From 85c9bd12b23b53fbc83191cea4e58bbf0b282478 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 31 Oct 2023 00:55:27 +0100 Subject: [PATCH 3/4] Tools/BrainMonkeyRaceCondition: add tests with PHP 8.0+ named parameters The commit in PR 322 in which the function call retrieving the parameter was changed from the WPCS method to the PHPCSUtils method already added support for this. This commit just adds some tests to safeguard it. --- .../BrainMonkeyRaceConditionUnitTest.inc | 37 +++++++++++++++++++ .../BrainMonkeyRaceConditionUnitTest.php | 2 + 2 files changed, 39 insertions(+) diff --git a/Yoast/Tests/Tools/BrainMonkeyRaceConditionUnitTest.inc b/Yoast/Tests/Tools/BrainMonkeyRaceConditionUnitTest.inc index bce0ba9f..cb7ddb3f 100644 --- a/Yoast/Tests/Tools/BrainMonkeyRaceConditionUnitTest.inc +++ b/Yoast/Tests/Tools/BrainMonkeyRaceConditionUnitTest.inc @@ -153,4 +153,41 @@ class BrainMonkeyBasedTest extends TestCase { // Do something to satisfy expectations. } + + /* + * Tests involving PHP 8.0+ named parameters. + */ + public function test_NOT_race_condition_wrong_param_name() { + Functions\expect( not_functionname: "apply_filters")->times(5); + + Filters\ExpectDone ( 'filter_name' ); + + // Do something to satisfy expectations. + } + + public function test_NOT_race_condition_named_param_not_for_hook() { + Functions\expect( function_name: "other_function")->times(5); + + Filters\ExpectDone ( 'filter_name' ); + + // Do something to satisfy expectations. + } + + public function test_filter_race_condition_named_param() { + Functions\expect( function_name: "apply_filters")->twice(); + + Filters\ExpectApplied ( 'filter_name' ) + ->with( true ); + + // Do something to satisfy expectations. + } + + public function test_action_race_condition_named_param_unconventional_order() { + \Brain\Monkey\Functions\Expect( something: foo, function_name: 'do_action' )->once(); + + Actions\expectDone( 'yoast\action_name' ) + ->with( $param ); + + // Do something to satisfy expectations. + } } diff --git a/Yoast/Tests/Tools/BrainMonkeyRaceConditionUnitTest.php b/Yoast/Tests/Tools/BrainMonkeyRaceConditionUnitTest.php index 6b1d6339..c3db33de 100644 --- a/Yoast/Tests/Tools/BrainMonkeyRaceConditionUnitTest.php +++ b/Yoast/Tests/Tools/BrainMonkeyRaceConditionUnitTest.php @@ -24,6 +24,8 @@ public function getErrorList(): array { 113 => 1, 122 => 1, 131 => 1, + 176 => 1, + 185 => 1, ]; } From 47316934463135d9eb05e49db726ebe4e0a56a0c Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 31 Oct 2023 00:58:33 +0100 Subject: [PATCH 4/4] Tools/BrainMonkeyRaceCondition: minor tweak As we're only looking for a _method_ in a _class_, this condition will never happen under normal circumstances, not even in case of live coding as the function declaration would not be recognized as a _method_ in that case, so the sniff would bow out before reaching this condition. All the same, there's always to possibility that this could happen if PHP adds some new syntax and the PHPCS tokenizer does not support it yet or if there is another Tokenizer issue, so I'm keeping the condition, but marking it as fine to ignore for code coverage. --- Yoast/Sniffs/Tools/BrainMonkeyRaceConditionSniff.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Yoast/Sniffs/Tools/BrainMonkeyRaceConditionSniff.php b/Yoast/Sniffs/Tools/BrainMonkeyRaceConditionSniff.php index c7e84ea9..f72a1b83 100644 --- a/Yoast/Sniffs/Tools/BrainMonkeyRaceConditionSniff.php +++ b/Yoast/Sniffs/Tools/BrainMonkeyRaceConditionSniff.php @@ -92,8 +92,8 @@ public function process( File $phpcsFile, $stackPtr ) { // Now walk the contents of the function declaration to see if we can find the other function call. if ( isset( $tokens[ $functionToken ]['scope_opener'], $tokens[ $functionToken ]['scope_closer'] ) === false ) { - // We don't know the start or end of the function. - return; + // We don't know the start or end of the function. Edge case which can't happen under normal circumstances. + return; // @codeCoverageIgnore } $targetContent = 'expectdone';