Skip to content

Commit 4a88562

Browse files
jrfnljrfnl
committed
Performance/CacheValueOverride: extend AbstractFunctionRestrictionsSniff
As things were, the determination of whether or not a `T_STRING` is a call to the global WP native `wp_cache_get()` function was severely flawed. By switching the sniff over to be based on the WordPressCS `AbstractFunctionRestrictionsSniff` class, this flaw is mitigated. This flaw did not lead to false positive due to the subsequent token walking being very rigid. Includes adding a slew of additional tests to document the sniff behaviour. Additionally, the tests have been made more comprehensive and varied by: * Testing against false positives for calls to methods or namespaced function calls (= the issue being addressed in this PR). * Testing against false positives for attribute class using the same name as the function. * Ensure function import `use` statements are not flagged. We're not interested in those. * Adding more variations to the pre-existing tests: - Non-lowercase function call(s).
1 parent aef212e commit 4a88562

File tree

3 files changed

+88
-59
lines changed

3 files changed

+88
-59
lines changed

‎WordPressVIPMinimum/Sniffs/Performance/CacheValueOverrideSniff.php‎

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

1212
use PHP_CodeSniffer\Util\Tokens;
13-
use WordPressVIPMinimum\Sniffs\Sniff;
13+
use WordPressCS\WordPress\AbstractFunctionRestrictionsSniff;
1414

1515
/**
1616
* This sniff check whether a cached value is being overridden.
1717
*/
18-
class CacheValueOverrideSniff extends Sniff {
18+
class CacheValueOverrideSniff extends AbstractFunctionRestrictionsSniff {
1919

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

29-
3033
/**
31-
* Processes the tokens that this sniff is interested in.
34+
* Process a matched token.
3235
*
33-
* @param int $stackPtr The position in the stack where the token was found.
36+
* @param int $stackPtr The position of the current token in the stack.
37+
* @param string $group_name The name of the group which was matched.
38+
* @param string $matched_content The token content (function name) which was matched
39+
* in lowercase.
3440
*
3541
* @return void
3642
*/
37-
public function process_token( $stackPtr ) {
38-
39-
$functionName = $this->tokens[ $stackPtr ]['content'];
40-
41-
if ( $functionName !== 'wp_cache_get' ) {
42-
// Not a function we are looking for.
43-
return;
44-
}
45-
46-
if ( $this->isFunctionCall( $stackPtr ) === false ) {
47-
// Not a function call.
48-
return;
49-
}
50-
43+
public function process_matched_token( $stackPtr, $group_name, $matched_content ) {
5144
$variablePos = $this->isVariableAssignment( $stackPtr );
52-
5345
if ( $variablePos === false ) {
5446
// Not a variable assignment.
5547
return;
@@ -82,32 +74,6 @@ public function process_token( $stackPtr ) {
8274
}
8375
}
8476

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-
11177
/**
11278
* Check whether the examined code is a variable assignment.
11379
*

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

Lines changed: 70 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,79 @@
11
<?php
22

3-
// Bad code start.
4-
$bad_wp_users = wp_cache_get( md5( self::CACHE_KEY . '_wp_users'), self::CACHE_GROUP );
5-
$bad_wp_users = false; // Bad.
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() {}
620

7-
if ( empty( $bad_wp_users ) ) {
8-
$bad_wp_users = get_users();
9-
}
1021

11-
// Good code start.
22+
/*
23+
* These should all be okay.
24+
*/
1225
$good_wp_users = wp_cache_get( md5( self::CACHE_KEY . '_wp_users'), self::CACHE_GROUP );
1326

1427
if ( empty( $good_wp_users ) ) {
1528
$good_wp_users = false;
1629
$good_wp_users = get_users();
17-
}
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+
}

‎WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,8 @@ public function getErrorList( $testFile = '' ) {
2828
switch ( $testFile ) {
2929
case 'CacheValueOverrideUnitTest.1.inc':
3030
return [
31-
5 => 1,
31+
68 => 1,
32+
78 => 1,
3233
];
3334

3435
default:

0 commit comments

Comments
 (0)