diff --git a/WordPressVIPMinimum/Sniffs/Performance/CacheValueOverrideSniff.php b/WordPressVIPMinimum/Sniffs/Performance/CacheValueOverrideSniff.php index 83bdccc4..4a06a7f1 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/CacheValueOverrideSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/CacheValueOverrideSniff.php @@ -10,46 +10,61 @@ namespace WordPressVIPMinimum\Sniffs\Performance; use PHP_CodeSniffer\Util\Tokens; -use WordPressVIPMinimum\Sniffs\Sniff; +use PHPCSUtils\Tokens\Collections; +use PHPCSUtils\Utils\Conditions; +use WordPressCS\WordPress\AbstractFunctionRestrictionsSniff; /** * This sniff check whether a cached value is being overridden. */ -class CacheValueOverrideSniff extends Sniff { +class CacheValueOverrideSniff extends AbstractFunctionRestrictionsSniff { /** - * Returns the token types that this sniff is interested in. + * Groups of functions to restrict. * - * @return array + * @return array>> */ - public function register() { - return [ T_STRING ]; + public function getGroups() { + return [ + 'wp_cache_get' => [ + 'functions' => [ 'wp_cache_get' ], + ], + ]; } - /** - * Processes the tokens that this sniff is interested in. + * Process a matched token. * - * @param int $stackPtr The position in the stack where the token was found. + * @param int $stackPtr The position of the current token in the stack. + * @param string $group_name The name of the group which was matched. + * @param string $matched_content The token content (function name) which was matched + * in lowercase. * * @return void */ - public function process_token( $stackPtr ) { - - $functionName = $this->tokens[ $stackPtr ]['content']; + public function process_matched_token( $stackPtr, $group_name, $matched_content ) { + $openBracket = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); + if ( $openBracket === false || isset( $this->tokens[ $openBracket ]['parenthesis_closer'] ) === false ) { + // Import use statement for function or parse error/live coding. Ignore. + return; + } - if ( $functionName !== 'wp_cache_get' ) { - // Not a function we are looking for. + $closeBracket = $this->tokens[ $openBracket ]['parenthesis_closer']; + $firstNonEmpty = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $openBracket + 1 ), null, true ); + $nextNonEmpty = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $firstNonEmpty + 1 ), null, true ); + if ( $nextNonEmpty === false ) { + // Parse error/live coding. Ignore. return; } - if ( $this->isFunctionCall( $stackPtr ) === false ) { - // Not a function call. + if ( $this->tokens[ $firstNonEmpty ]['code'] === T_ELLIPSIS + && $nextNonEmpty === $closeBracket + ) { + // First class callable. Ignore. return; } $variablePos = $this->isVariableAssignment( $stackPtr ); - if ( $variablePos === false ) { // Not a variable assignment. return; @@ -58,56 +73,50 @@ public function process_token( $stackPtr ) { $variableToken = $this->tokens[ $variablePos ]; $variableName = $variableToken['content']; - // Find the next non-empty token. - $openBracket = $this->phpcsFile->findNext( Tokens::$emptyTokens, $stackPtr + 1, null, true ); + // Figure out the scope we need to search in. + $searchEnd = $this->phpcsFile->numTokens; + $functionPtr = Conditions::getLastCondition( $this->phpcsFile, $stackPtr, [ T_FUNCTION, T_CLOSURE ] ); + if ( $functionPtr !== false && isset( $this->tokens[ $functionPtr ]['scope_closer'] ) ) { + $searchEnd = $this->tokens[ $functionPtr ]['scope_closer']; + } - // Find the closing bracket. - $closeBracket = $this->tokens[ $openBracket ]['parenthesis_closer']; + $nextVariableOccurrence = false; + for ( $i = $closeBracket + 1; $i < $searchEnd; $i++ ) { + if ( $this->tokens[ $i ]['code'] === T_VARIABLE && $this->tokens[ $i ]['content'] === $variableName ) { + $nextVariableOccurrence = $i; + break; + } + + // Skip over any and all closed scopes. + if ( isset( Collections::closedScopes()[ $this->tokens[ $i ]['code'] ] ) ) { + if ( isset( $this->tokens[ $i ]['scope_closer'] ) ) { + $i = $this->tokens[ $i ]['scope_closer']; + } + } + } - $nextVariableOccurrence = $this->phpcsFile->findNext( T_VARIABLE, $closeBracket + 1, null, false, $variableName ); + if ( $nextVariableOccurrence === false ) { + return; + } - $rightAfterNextVariableOccurence = $this->phpcsFile->findNext( Tokens::$emptyTokens, $nextVariableOccurrence + 1, null, true, null, true ); + $rightAfterNextVariableOccurence = $this->phpcsFile->findNext( Tokens::$emptyTokens, $nextVariableOccurrence + 1, $searchEnd, true, null, true ); - if ( $this->tokens[ $rightAfterNextVariableOccurence ]['code'] !== T_EQUAL ) { + if ( $rightAfterNextVariableOccurence === false + || $this->tokens[ $rightAfterNextVariableOccurence ]['code'] !== T_EQUAL + ) { // Not a value override. return; } - $valueAfterEqualSign = $this->phpcsFile->findNext( Tokens::$emptyTokens, $rightAfterNextVariableOccurence + 1, null, true, null, true ); + $valueAfterEqualSign = $this->phpcsFile->findNext( Tokens::$emptyTokens, $rightAfterNextVariableOccurence + 1, $searchEnd, true, null, true ); - if ( $this->tokens[ $valueAfterEqualSign ]['code'] === T_FALSE ) { + if ( $valueAfterEqualSign !== false && $this->tokens[ $valueAfterEqualSign ]['code'] === T_FALSE ) { $message = 'Obtained cached value in `%s` is being overridden. Disabling caching?'; $data = [ $variableName ]; $this->phpcsFile->addError( $message, $nextVariableOccurrence, 'CacheValueOverride', $data ); } } - /** - * Check whether the examined code is a function call. - * - * @param int $stackPtr The position of the current token in the stack. - * - * @return bool - */ - private function isFunctionCall( $stackPtr ) { - - // Find the next non-empty token. - $openBracket = $this->phpcsFile->findNext( Tokens::$emptyTokens, $stackPtr + 1, null, true ); - - if ( $this->tokens[ $openBracket ]['code'] !== T_OPEN_PARENTHESIS ) { - // Not a function call. - return false; - } - - // Find the previous non-empty token. - $search = Tokens::$emptyTokens; - $search[] = T_BITWISE_AND; - $previous = $this->phpcsFile->findPrevious( $search, $stackPtr - 1, null, true ); - - // It's a function definition, not a function call, so return false. - return ! ( $this->tokens[ $previous ]['code'] === T_FUNCTION ); - } - /** * Check whether the examined code is a variable assignment. * @@ -117,9 +126,10 @@ private function isFunctionCall( $stackPtr ) { */ private function isVariableAssignment( $stackPtr ) { - // Find the previous non-empty token. + // Find the previous non-empty token, but allow for FQN function calls. $search = Tokens::$emptyTokens; $search[] = T_BITWISE_AND; + $search[] = T_NS_SEPARATOR; $previous = $this->phpcsFile->findPrevious( $search, $stackPtr - 1, null, true ); if ( $this->tokens[ $previous ]['code'] !== T_EQUAL ) { diff --git a/WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.1.inc b/WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.1.inc new file mode 100644 index 00000000..75874857 --- /dev/null +++ b/WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.1.inc @@ -0,0 +1,127 @@ +wp_cache_get(); $methodCall = false; +$nullsafeMethodCall = $this?->wp_cache_get(); $nullsafeMethodCall = false; +$staticMethodCall = MyClass::wp_cache_get(); $staticMethodCall = false; +echo WP_CACHE_GET; +$namespacedFunction = namespace\wp_cache_get(); $namespacedFunction = false; + +function &wp_cache_get() {} + +// Looks like a function call, but is a PHP 8.0+ class instantiation via an attribute. +#[WP_Cache_Get('text')] +function foo() {} + + +/* + * These should all be okay. + */ +$good_wp_users = wp_cache_get( md5( self::CACHE_KEY . '_wp_users'), self::CACHE_GROUP ); + +if ( empty( $good_wp_users ) ) { + $good_wp_users = false; + $good_wp_users = get_users(); +} + +function NoAssignment() { + $unrelated_var = false; + \wp_cache_get( $key, $group ) === false && do_something_to_create_new_cache(); + $unrelated_var = false; +} + +function functionCallsAreNotCaseSensitiveGood() { + $retrieved = WP_Cache_Get( ...$params ); + if ( empty( $retrieved ) ) { + $retrieved = false; + } +} + +function fqnFunctionCallGood() { + $no_params_not_our_concern = \wp_cache_get(); + if ( empty( $no_params_not_our_concern ) ) { + $no_params_not_our_concern = false; + } +} + +function cacheOverrideDoesntAssignFalse() { + $not_false = wp_cache_get( ...$params ); + $not_false = true; +} + + +function cacheOverrideAssignsUnknownValue( $unknown ) { + $not_false = wp_cache_get( ...$params ); + $not_false = $unknown; +} + + +/* + * These should all be flagged. + */ +$bad_wp_users = wp_cache_get( md5( self::CACHE_KEY . '_wp_users'), self::CACHE_GROUP ); +$bad_wp_users = false; // Bad. + +if ( empty( $bad_wp_users ) ) { + $bad_wp_users = get_users(); +} + +function functionCallsAreNotCaseSensitiveBad() { + $cache_retrieved = WP_Cache_Get( ...$params ); + // Comment. + do_something_else(); + $cache_retrieved = false; // Bad. +} + +function fqnFunctionCallBad() { + $shouldBeCaught = \wp_cache_get(); + $shouldBeCaught = false; // Bad. +} + +// Ignore PHP 8.1 first class callable. +// Ignore as the assignment is not for the return value of the function, but for the callable. +function firstClassCallable() { + $callable = wp_cache_get(...); + $callable = false; // OK. +} + +// Bug fix - the variable re-assignment must be in the same scope. +function controlStructuresAreNotClosedScopes() { + if (false === ($cache = \wp_cache_get( $key, $group ))) { + // Create new cache. + } + $cache = false; // Bad. +} + + +function closedScopeA() { + $myCache = \wp_cache_get(); +} + +function closedScopeB($myCache = false) {} // OK. + +function closedScopeC() { + $yourCache = wp_cache_get(); + + $closure = function () { + $yourCache = false; // OK. + }; +} + +$globalScope = wp_cache_get(); +function closedScopeD() { + $globalScope = false; // OK. +} + +$globalScope = wp_cache_get(); +function closedScopeE($globalScope = false;) {} // OK. + +$globalScope = wp_cache_get(); +class FooBar { + function method($globalScope = false) {} // OK. +} diff --git a/WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.2.inc b/WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.2.inc new file mode 100644 index 00000000..37435fd1 --- /dev/null +++ b/WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.2.inc @@ -0,0 +1,5 @@ + Key is the line number, value is the number of expected errors. */ - public function getErrorList() { - return [ - 5 => 1, - ]; + public function getErrorList( $testFile = '' ) { + + switch ( $testFile ) { + case 'CacheValueOverrideUnitTest.1.inc': + return [ + 68 => 1, + 78 => 1, + 83 => 1, + 98 => 1, + ]; + + default: + return []; + } } /**