Skip to content

Commit 45bf583

Browse files
GaryJonesGaryJones
authored
Merge pull request #867 from Automattic/feature/performance-cachevalueoverride-initial-improvements
2 parents 3bc61bd + 3202749 commit 45bf583

File tree

7 files changed

+223
-74
lines changed

7 files changed

+223
-74
lines changed

‎WordPressVIPMinimum/Sniffs/Performance/CacheValueOverrideSniff.php‎

Lines changed: 63 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -10,46 +10,61 @@
1010
namespace WordPressVIPMinimum\Sniffs\Performance;
1111

1212
use PHP_CodeSniffer\Util\Tokens;
13-
use WordPressVIPMinimum\Sniffs\Sniff;
13+
use PHPCSUtils\Tokens\Collections;
14+
use PHPCSUtils\Utils\Conditions;
15+
use WordPressCS\WordPress\AbstractFunctionRestrictionsSniff;
1416

1517
/**
1618
* This sniff check whether a cached value is being overridden.
1719
*/
18-
class CacheValueOverrideSniff extends Sniff {
20+
class CacheValueOverrideSniff extends AbstractFunctionRestrictionsSniff {
1921

2022
/**
21-
* Returns the token types that this sniff is interested in.
23+
* Groups of functions to restrict.
2224
*
23-
* @return array<int|string>
25+
* @return array<string, array<string, array<string>>>
2426
*/
25-
public function register() {
26-
return [ T_STRING ];
27+
public function getGroups() {
28+
return [
29+
'wp_cache_get' => [
30+
'functions' => [ 'wp_cache_get' ],
31+
],
32+
];
2733
}
2834

29-
3035
/**
31-
* Processes the tokens that this sniff is interested in.
36+
* Process a matched token.
3237
*
33-
* @param int $stackPtr The position in the stack where the token was found.
38+
* @param int $stackPtr The position of the current token in the stack.
39+
* @param string $group_name The name of the group which was matched.
40+
* @param string $matched_content The token content (function name) which was matched
41+
* in lowercase.
3442
*
3543
* @return void
3644
*/
37-
public function process_token( $stackPtr ) {
38-
39-
$functionName = $this->tokens[ $stackPtr ]['content'];
45+
public function process_matched_token( $stackPtr, $group_name, $matched_content ) {
46+
$openBracket = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true );
47+
if ( $openBracket === false || isset( $this->tokens[ $openBracket ]['parenthesis_closer'] ) === false ) {
48+
// Import use statement for function or parse error/live coding. Ignore.
49+
return;
50+
}
4051

41-
if ( $functionName !== 'wp_cache_get' ) {
42-
// Not a function we are looking for.
52+
$closeBracket = $this->tokens[ $openBracket ]['parenthesis_closer'];
53+
$firstNonEmpty = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $openBracket + 1 ), null, true );
54+
$nextNonEmpty = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $firstNonEmpty + 1 ), null, true );
55+
if ( $nextNonEmpty === false ) {
56+
// Parse error/live coding. Ignore.
4357
return;
4458
}
4559

46-
if ( $this->isFunctionCall( $stackPtr ) === false ) {
47-
// Not a function call.
60+
if ( $this->tokens[ $firstNonEmpty ]['code'] === T_ELLIPSIS
61+
&& $nextNonEmpty === $closeBracket
62+
) {
63+
// First class callable. Ignore.
4864
return;
4965
}
5066

5167
$variablePos = $this->isVariableAssignment( $stackPtr );
52-
5368
if ( $variablePos === false ) {
5469
// Not a variable assignment.
5570
return;
@@ -58,56 +73,50 @@ public function process_token( $stackPtr ) {
5873
$variableToken = $this->tokens[ $variablePos ];
5974
$variableName = $variableToken['content'];
6075

61-
// Find the next non-empty token.
62-
$openBracket = $this->phpcsFile->findNext( Tokens::$emptyTokens, $stackPtr + 1, null, true );
76+
// Figure out the scope we need to search in.
77+
$searchEnd = $this->phpcsFile->numTokens;
78+
$functionPtr = Conditions::getLastCondition( $this->phpcsFile, $stackPtr, [ T_FUNCTION, T_CLOSURE ] );
79+
if ( $functionPtr !== false && isset( $this->tokens[ $functionPtr ]['scope_closer'] ) ) {
80+
$searchEnd = $this->tokens[ $functionPtr ]['scope_closer'];
81+
}
6382

64-
// Find the closing bracket.
65-
$closeBracket = $this->tokens[ $openBracket ]['parenthesis_closer'];
83+
$nextVariableOccurrence = false;
84+
for ( $i = $closeBracket + 1; $i < $searchEnd; $i++ ) {
85+
if ( $this->tokens[ $i ]['code'] === T_VARIABLE && $this->tokens[ $i ]['content'] === $variableName ) {
86+
$nextVariableOccurrence = $i;
87+
break;
88+
}
89+
90+
// Skip over any and all closed scopes.
91+
if ( isset( Collections::closedScopes()[ $this->tokens[ $i ]['code'] ] ) ) {
92+
if ( isset( $this->tokens[ $i ]['scope_closer'] ) ) {
93+
$i = $this->tokens[ $i ]['scope_closer'];
94+
}
95+
}
96+
}
6697

67-
$nextVariableOccurrence = $this->phpcsFile->findNext( T_VARIABLE, $closeBracket + 1, null, false, $variableName );
98+
if ( $nextVariableOccurrence === false ) {
99+
return;
100+
}
68101

69-
$rightAfterNextVariableOccurence = $this->phpcsFile->findNext( Tokens::$emptyTokens, $nextVariableOccurrence + 1, null, true, null, true );
102+
$rightAfterNextVariableOccurence = $this->phpcsFile->findNext( Tokens::$emptyTokens, $nextVariableOccurrence + 1, $searchEnd, true, null, true );
70103

71-
if ( $this->tokens[ $rightAfterNextVariableOccurence ]['code'] !== T_EQUAL ) {
104+
if ( $rightAfterNextVariableOccurence === false
105+
|| $this->tokens[ $rightAfterNextVariableOccurence ]['code'] !== T_EQUAL
106+
) {
72107
// Not a value override.
73108
return;
74109
}
75110

76-
$valueAfterEqualSign = $this->phpcsFile->findNext( Tokens::$emptyTokens, $rightAfterNextVariableOccurence + 1, null, true, null, true );
111+
$valueAfterEqualSign = $this->phpcsFile->findNext( Tokens::$emptyTokens, $rightAfterNextVariableOccurence + 1, $searchEnd, true, null, true );
77112

78-
if ( $this->tokens[ $valueAfterEqualSign ]['code'] === T_FALSE ) {
113+
if ( $valueAfterEqualSign !== false && $this->tokens[ $valueAfterEqualSign ]['code'] === T_FALSE ) {
79114
$message = 'Obtained cached value in `%s` is being overridden. Disabling caching?';
80115
$data = [ $variableName ];
81116
$this->phpcsFile->addError( $message, $nextVariableOccurrence, 'CacheValueOverride', $data );
82117
}
83118
}
84119

85-
/**
86-
* Check whether the examined code is a function call.
87-
*
88-
* @param int $stackPtr The position of the current token in the stack.
89-
*
90-
* @return bool
91-
*/
92-
private function isFunctionCall( $stackPtr ) {
93-
94-
// Find the next non-empty token.
95-
$openBracket = $this->phpcsFile->findNext( Tokens::$emptyTokens, $stackPtr + 1, null, true );
96-
97-
if ( $this->tokens[ $openBracket ]['code'] !== T_OPEN_PARENTHESIS ) {
98-
// Not a function call.
99-
return false;
100-
}
101-
102-
// Find the previous non-empty token.
103-
$search = Tokens::$emptyTokens;
104-
$search[] = T_BITWISE_AND;
105-
$previous = $this->phpcsFile->findPrevious( $search, $stackPtr - 1, null, true );
106-
107-
// It's a function definition, not a function call, so return false.
108-
return ! ( $this->tokens[ $previous ]['code'] === T_FUNCTION );
109-
}
110-
111120
/**
112121
* Check whether the examined code is a variable assignment.
113122
*
@@ -117,9 +126,10 @@ private function isFunctionCall( $stackPtr ) {
117126
*/
118127
private function isVariableAssignment( $stackPtr ) {
119128

120-
// Find the previous non-empty token.
129+
// Find the previous non-empty token, but allow for FQN function calls.
121130
$search = Tokens::$emptyTokens;
122131
$search[] = T_BITWISE_AND;
132+
$search[] = T_NS_SEPARATOR;
123133
$previous = $this->phpcsFile->findPrevious( $search, $stackPtr - 1, null, true );
124134

125135
if ( $this->tokens[ $previous ]['code'] !== T_EQUAL ) {

‎WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.1.inc‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
<?php
2+
3+
/*
4+
* Not the sniff target.
5+
*/
6+
use function wp_cache_get;
7+
8+
$partiallyQualifiedFunction = my\ns\wp_cache_get(); $partiallyQualifiedFunction = false;
9+
$methodCall = $this->wp_cache_get(); $methodCall = false;
10+
$nullsafeMethodCall = $this?->wp_cache_get(); $nullsafeMethodCall = false;
11+
$staticMethodCall = MyClass::wp_cache_get(); $staticMethodCall = false;
12+
echo WP_CACHE_GET;
13+
$namespacedFunction = namespace\wp_cache_get(); $namespacedFunction = false;
14+
15+
function &wp_cache_get() {}
16+
17+
// Looks like a function call, but is a PHP 8.0+ class instantiation via an attribute.
18+
#[WP_Cache_Get('text')]
19+
function foo() {}
20+
21+
22+
/*
23+
* These should all be okay.
24+
*/
25+
$good_wp_users = wp_cache_get( md5( self::CACHE_KEY . '_wp_users'), self::CACHE_GROUP );
26+
27+
if ( empty( $good_wp_users ) ) {
28+
$good_wp_users = false;
29+
$good_wp_users = get_users();
30+
}
31+
32+
function NoAssignment() {
33+
$unrelated_var = false;
34+
\wp_cache_get( $key, $group ) === false && do_something_to_create_new_cache();
35+
$unrelated_var = false;
36+
}
37+
38+
function functionCallsAreNotCaseSensitiveGood() {
39+
$retrieved = WP_Cache_Get( ...$params );
40+
if ( empty( $retrieved ) ) {
41+
$retrieved = false;
42+
}
43+
}
44+
45+
function fqnFunctionCallGood() {
46+
$no_params_not_our_concern = \wp_cache_get();
47+
if ( empty( $no_params_not_our_concern ) ) {
48+
$no_params_not_our_concern = false;
49+
}
50+
}
51+
52+
function cacheOverrideDoesntAssignFalse() {
53+
$not_false = wp_cache_get( ...$params );
54+
$not_false = true;
55+
}
56+
57+
58+
function cacheOverrideAssignsUnknownValue( $unknown ) {
59+
$not_false = wp_cache_get( ...$params );
60+
$not_false = $unknown;
61+
}
62+
63+
64+
/*
65+
* These should all be flagged.
66+
*/
67+
$bad_wp_users = wp_cache_get( md5( self::CACHE_KEY . '_wp_users'), self::CACHE_GROUP );
68+
$bad_wp_users = false; // Bad.
69+
70+
if ( empty( $bad_wp_users ) ) {
71+
$bad_wp_users = get_users();
72+
}
73+
74+
function functionCallsAreNotCaseSensitiveBad() {
75+
$cache_retrieved = WP_Cache_Get( ...$params );
76+
// Comment.
77+
do_something_else();
78+
$cache_retrieved = false; // Bad.
79+
}
80+
81+
function fqnFunctionCallBad() {
82+
$shouldBeCaught = \wp_cache_get();
83+
$shouldBeCaught = false; // Bad.
84+
}
85+
86+
// Ignore PHP 8.1 first class callable.
87+
// Ignore as the assignment is not for the return value of the function, but for the callable.
88+
function firstClassCallable() {
89+
$callable = wp_cache_get(...);
90+
$callable = false; // OK.
91+
}
92+
93+
// Bug fix - the variable re-assignment must be in the same scope.
94+
function controlStructuresAreNotClosedScopes() {
95+
if (false === ($cache = \wp_cache_get( $key, $group ))) {
96+
// Create new cache.
97+
}
98+
$cache = false; // Bad.
99+
}
100+
101+
102+
function closedScopeA() {
103+
$myCache = \wp_cache_get();
104+
}
105+
106+
function closedScopeB($myCache = false) {} // OK.
107+
108+
function closedScopeC() {
109+
$yourCache = wp_cache_get();
110+
111+
$closure = function () {
112+
$yourCache = false; // OK.
113+
};
114+
}
115+
116+
$globalScope = wp_cache_get();
117+
function closedScopeD() {
118+
$globalScope = false; // OK.
119+
}
120+
121+
$globalScope = wp_cache_get();
122+
function closedScopeE($globalScope = false;) {} // OK.
123+
124+
$globalScope = wp_cache_get();
125+
class FooBar {
126+
function method($globalScope = false) {} // OK.
127+
}

‎WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.2.inc‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
<?php
2+
3+
// Parse error/live coding test.
4+
// This should be the only test in the file.
5+
$liveCoding = wp_cache_get()

‎WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.3.inc‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
<?php
2+
3+
// Parse error/live coding test.
4+
// This should be the only test in the file.
5+
$liveCoding = wp_cache_get();
6+
$liveCoding

‎WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.4.inc‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
<?php
2+
3+
// Parse error/live coding test.
4+
// This should be the only test in the file.
5+
$liveCoding = wp_cache_get();
6+
$liveCoding =

‎WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.inc‎

Lines changed: 0 additions & 17 deletions
This file was deleted.

‎WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.php‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,24 @@ class CacheValueOverrideUnitTest extends AbstractSniffUnitTest {
1919
/**
2020
* Returns the lines where errors should occur.
2121
*
22+
* @param string $testFile The name of the file being tested.
23+
*
2224
* @return array<int, int> Key is the line number, value is the number of expected errors.
2325
*/
24-
public function getErrorList() {
25-
return [
26-
5 => 1,
27-
];
26+
public function getErrorList( $testFile = '' ) {
27+
28+
switch ( $testFile ) {
29+
case 'CacheValueOverrideUnitTest.1.inc':
30+
return [
31+
68 => 1,
32+
78 => 1,
33+
83 => 1,
34+
98 => 1,
35+
];
36+
37+
default:
38+
return [];
39+
}
2840
}
2941

3042
/**

0 commit comments

Comments
 (0)