diff --git a/Yoast/Sniffs/Tools/BrainMonkeyRaceConditionSniff.php b/Yoast/Sniffs/Tools/BrainMonkeyRaceConditionSniff.php index 57e02283..f72a1b83 100644 --- a/Yoast/Sniffs/Tools/BrainMonkeyRaceConditionSniff.php +++ b/Yoast/Sniffs/Tools/BrainMonkeyRaceConditionSniff.php @@ -2,11 +2,15 @@ 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; use PHPCSUtils\Utils\FunctionDeclarations; 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. @@ -14,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. @@ -27,65 +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 - || $this->tokens[ $prevNonEmpty ]['code'] === \T_DOUBLE_COLON - || $this->tokens[ $prevNonEmpty ]['code'] === \T_OBJECT_OPERATOR - || $this->tokens[ $prevNonEmpty ]['type'] === 'T_NULLSAFE_OBJECT_OPERATOR' - || $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; } - // Make sure the token is in a class method. - if ( empty( $this->tokens[ $stackPtr ]['conditions'] ) ) { + $functionToken = Conditions::getLastCondition( $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( $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; } @@ -93,21 +79,21 @@ 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 ) { - // We don't know the start or end of the function. - return; + if ( isset( $tokens[ $functionToken ]['scope_opener'], $tokens[ $functionToken ]['scope_closer'] ) === false ) { + // We don't know the start or end of the function. Edge case which can't happen under normal circumstances. + return; // @codeCoverageIgnore } $targetContent = 'expectdone'; @@ -115,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; } } 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, ]; }