Skip to content

Commit 05a0547

Browse files
GaryJonesGaryJones
authored
Merge pull request #853 from Automattic/feature/performance-lowexpirycachetime-sniff-review
2 parents a30da72 + 8acb4ae commit 05a0547

File tree

4 files changed

+122
-56
lines changed

4 files changed

+122
-56
lines changed

‎WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php‎

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

1212
use PHP_CodeSniffer\Util\Tokens;
13+
use PHPCSUtils\Utils\Numbers;
14+
use PHPCSUtils\Utils\PassedParameters;
1315
use PHPCSUtils\Utils\TextStrings;
1416
use WordPressCS\WordPress\AbstractFunctionParameterSniff;
1517

@@ -67,21 +69,21 @@ class LowExpiryCacheTimeSniff extends AbstractFunctionParameterSniff {
6769
* normal file processing.
6870
*/
6971
public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) {
70-
if ( isset( $parameters[4] ) === false ) {
72+
$expire_param = PassedParameters::getParameterFromStack( $parameters, 4, 'expire' );
73+
if ( $expire_param === false ) {
7174
// If no cache expiry time, bail (i.e. we don't want to flag for something like feeds where it is cached indefinitely until a hook runs).
7275
return;
7376
}
7477

75-
$param = $parameters[4];
7678
$tokensAsString = '';
7779
$reportPtr = null;
7880
$openParens = 0;
7981

8082
$message = 'Cache expiry time could not be determined. Please inspect that the fourth parameter passed to %s() evaluates to 300 seconds or more. Found: "%s"';
8183
$error_code = 'CacheTimeUndetermined';
82-
$data = [ $matched_content, $parameters[4]['raw'] ];
84+
$data = [ $matched_content, $expire_param['clean'] ];
8385

84-
for ( $i = $param['start']; $i <= $param['end']; $i++ ) {
86+
for ( $i = $expire_param['start']; $i <= $expire_param['end']; $i++ ) {
8587
if ( isset( Tokens::$emptyTokens[ $this->tokens[ $i ]['code'] ] ) === true ) {
8688
$tokensAsString .= ' ';
8789
continue;
@@ -104,8 +106,10 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p
104106
if ( $this->tokens[ $i ]['code'] === T_LNUMBER
105107
|| $this->tokens[ $i ]['code'] === T_DNUMBER
106108
) {
107-
// Integer or float.
108-
$tokensAsString .= $this->tokens[ $i ]['content'];
109+
// Make sure that PHP 7.4 numeric literals and PHP 8.1 explicit octals don't cause problems.
110+
$number_info = Numbers::getCompleteNumber( $this->phpcsFile, $i );
111+
$tokensAsString .= $number_info['decimal'];
112+
$i = $number_info['last_token'];
109113
continue;
110114
}
111115

WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc renamed to WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.1.inc

Lines changed: 51 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,12 @@ wp_cache_set( $testing, $data, 'test_group', 5*MINUTE_IN_SECONDS );
1010
wp_cache_set( 123, $data, 'test_group', 5 * MINUTE_IN_SECONDS );
1111
wp_cache_set( 1234, $data, '', 425 );
1212
wp_cache_set( $testing, $data, null, 350 );
13-
wp_cache_set( $testing, $data );
13+
\wp_cache_set( $testing, $data );
1414
wp_cache_set( 'test', $data, $group );
1515

1616
wp_cache_add( 'test', $data, $group, 300 );
1717
wp_cache_add( $testing, $data, 'test_group', 6*MINUTE_IN_SECONDS );
18-
wp_cache_add( 1234, $data, '', 425 );
18+
WP_CACHE_ADD( 1234, $data, '', 425 );
1919
wp_cache_add( $testing, $data, null, 350 );
2020

2121
wp_cache_replace( 'test', $data, $group, 300 );
@@ -30,13 +30,13 @@ wp_cache_set( 123, $data, null, 1.5 * MINUTE_IN_SECONDS ); // Lower than 300.
3030
wp_cache_set( $testing, $data, '', 1.5 * MINUTE_IN_SECONDS ); // Lower than 300.
3131

3232
wp_cache_add( 'test', $data, $group, 100 ); // Lower than 300.
33-
wp_cache_add( 'test', $data, $group, 2*MINUTE_IN_SECONDS ); // Lower than 300.
33+
\wp_cache_add( 'test', $data, $group, 2*MINUTE_IN_SECONDS ); // Lower than 300.
3434
wp_cache_add( 123, $data, null, 1.5 * MINUTE_IN_SECONDS ); // Lower than 300.
3535
wp_cache_add( $testing, $data, '', 1.5 * MINUTE_IN_SECONDS ); // Lower than 300.
3636

3737
wp_cache_replace( 'test', $data, $group, 100 ); // Lower than 300.
3838
wp_cache_replace( 'test', $data, $group, 2*MINUTE_IN_SECONDS ); // Lower than 300.
39-
wp_cache_replace( 123, $data, null, 1.5 * MINUTE_IN_SECONDS ); // Lower than 300.
39+
WP_CACHE_REPLACE( 123, $data, null, 1.5 * MINUTE_IN_SECONDS ); // Lower than 300.
4040
wp_cache_replace( $testing, $data, '', 1.5 * MINUTE_IN_SECONDS ); // Lower than 300.
4141

4242
// Test error being reported on the line containing the parameter.
@@ -49,7 +49,7 @@ wp_cache_replace(
4949

5050
// Test calculations with floats.
5151
wp_cache_replace( $testing, $data, '', 7.5 * MINUTE_IN_SECONDS ); // OK.
52-
wp_cache_replace( $testing, $data, '', 500 * 0.1 ); // Bad.
52+
wp_cache_replace( $testing, $data, '', 500 * 0.1, ); // Bad.
5353

5454
// Test comment handling.
5555
wp_cache_add( 'test', $data, $group, /* Deliberately left empty */ ); // OK.
@@ -71,11 +71,11 @@ wp_cache_add(
7171
); // OK.
7272

7373
// Test variable/constant with or without calculation being passed.
74-
wp_cache_set( $key, $data, '', $time ); // Manual inspection warning.
74+
WP_Cache_Set( $key, $data, '', $time ); // Manual inspection warning.
7575
wp_cache_set( $key, $data, '', PREFIX_FIVE_MINUTES ); // Manual inspection warning.
76-
wp_cache_set( $key, $data, '', 20 * $time ); // Manual inspection warning.
77-
wp_cache_set( $key, $data, '', $base + $extra ); // Manual inspection warning.
78-
wp_cache_set( $key, $data, '', 300 + $extra ); // Manual inspection warning.
76+
wp_cache_set( $key, $data, '', 20 * $time /*comment*/ ); // Manual inspection warning.
77+
wp_cache_set( $key, $data, '', $base + $extra, ); // Manual inspection warning.
78+
\wp_cache_set( $key, $data, '', 300 + $extra ); // Manual inspection warning.
7979
wp_cache_set( $key, $data, '', PREFIX_CUSTOM_TIME * 5); // Manual inspection warning.
8080

8181
// Test calculations with additional aritmetic operators.
@@ -84,9 +84,9 @@ wp_cache_add( 'test', $data, $group, WEEK_IN_SECONDS / 3 + HOUR_IN_SECONDS ); /
8484

8585
// Test calculations grouped with parentheses.
8686
wp_cache_set( $key, $data, '', (24 * 60 * 60) ); // OK.
87-
wp_cache_set( $key, $data, '', (-(2 * 60) + 600) ); // OK.
87+
wp_cache_set( $key, $data, '', (-(2 * 60) + 600), ); // OK.
8888
wp_cache_set( $key, $data, '', (2 * 60) ); // Bad.
89-
wp_cache_set( $key, $data, '', (-(2 * 60) + 600 ); // OK - includes parse error, close parenthesis missing.
89+
9090

9191
// Test handling of numbers passed as strings.
9292
wp_cache_set( 'test', $data, $group, '300' ); // OK - type cast to integer within the function.
@@ -110,14 +110,52 @@ wp_cache_set( 'test', $data, $group, \MONTH_IN_SECONDS ); // OK.
110110

111111
// Test passing something which may look like one of the time constants, but isn't.
112112
wp_cache_set( 'test', $data, $group, month_in_seconds ); // Bad - constants are case-sensitive.
113-
wp_cache_set( 'test', $data, $group, HOUR_IN_SECONDS::methodName() ); // Bad - not a constant.
113+
wp_cache_set( 'test', $data, $group, /*comment*/ HOUR_IN_SECONDS::methodName() ); // Bad - not a constant.
114114
wp_cache_set( 'test', $data, $group, $obj->MONTH_IN_SECONDS ); // Bad - not a constant.
115115
wp_cache_set( 'test', $data, $group, $obj::MONTH_IN_SECONDS ); // Bad - not the WP constant.
116-
wp_cache_set( 'test', $data, $group, PluginNamespace\SubLevel\DAY_IN_SECONDS ); // Bad - not the WP constant.
116+
WP_Cache_Set( 'test', $data, $group, PluginNamespace\SubLevel\DAY_IN_SECONDS, ); // Bad - not the WP constant.
117117

118118
// Test passing negative number as cache time.
119119
wp_cache_set( 'test', $data, $group, -300 ); // Bad.
120120
wp_cache_add( $testing, $data, 'test_group', -6 * MINUTE_IN_SECONDS ); // Bad.
121121

122122
// Test more complex logic in the parameter.
123123
wp_cache_add( $key, $data, '', ($toggle ? 200 : 400) ); // Manual inspection warning.
124+
125+
// Test handling of non-numeric data in text string.
126+
wp_cache_set( 'test', $data, $group, '' ); // Manual inspection warning.
127+
wp_cache_set( 'test', $data, $group, '300 Mulberry Street' ); // Manual inspection warning.
128+
129+
// Test handling of edge case/parse error.
130+
wp_cache_set( 'test', $data, $group,\); // OK, ignore.
131+
132+
// Test handling of some modern PHP syntaxes.
133+
wp_cache_add('test', $data, $group, ...$params); // PHP 5.6 argument unpacking. Manual inspection warning.
134+
wp_cache_add( $key, $data, '', $toggle ?? 400) ); // PHP 7.0 null coalesce. Manual inspection warning.
135+
add_action('my_action', wp_cache_set(...)); // PHP 8.1 first class callable. OK, ignore.
136+
137+
// Looks like a function call, but is a PHP 8.0+ class instantiation via an attribute.
138+
#[WP_Cache_Replace('text', 'data', 'group', 50)]
139+
function foo() {}
140+
141+
// Alternative numeric base.
142+
wp_cache_set( $key, $data, '', 0620 ); // Octal number. OK (=400).
143+
wp_cache_set( $key, $data, '', 0x190 ); // Hexidecimal number. OK (=400).
144+
wp_cache_set( $key, $data, '', 0b110010000 ); // PHP 5.4 binary number. OK (=400).
145+
wp_cache_set( $key, $data, '', 1_000 ); // PHP 7.4 numeric literal with underscore. OK.
146+
wp_cache_set( $key, $data, '', 0o620 ); // PHP 8.1 octal literal. OK (=400).
147+
148+
wp_cache_set( $key, $data, '', 0226 ); // Octal number. Bad (=150).
149+
wp_cache_set( $key, $data, '', 0x96 ); // Hexidecimal number. Bad (=150).
150+
wp_cache_set( $key, $data, '', 0b10010110 ); // PHP 5.4 binary number. Bad (=150).
151+
wp_cache_set( $key, $data, '', 1_50 ); // PHP 7.4 numeric literal with underscore. Bad.
152+
wp_cache_set( $key, $data, '', 0o226 ); // PHP 8.1 octal literal. Bad (=150).
153+
154+
// Safeguard handling of function calls using PHP 8.0+ named parameters.
155+
wp_cache_add(data: $data, group: $group); // OK, well, not really, missing required $key param, but that's not the concern of this sniff.
156+
wp_cache_replace(data: $data, expire: 400, group: $group); // OK.
157+
wp_cache_add($key, group: $group, data: $data, expires: 100,); // OK, well, not really, typo in param name, but that's not the concern of the sniff.
158+
wp_cache_replace($key, expire: 400, group: $group, data: 100,); // OK.
159+
160+
wp_cache_replace($key, $data, expire: 100 ); // Bad.
161+
wp_cache_replace(expire: 100, data: $data, key: $group); // Bad.

‎WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.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 (missing close parentheses).
4+
// This should be the only test in this file.
5+
wp_cache_set( $key, $data, '', (-(2 * 60) + 600 ); // OK.

‎WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php‎

Lines changed: 56 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -28,44 +28,63 @@ public function getErrorList() {
2828
/**
2929
* Returns the lines where warnings should occur.
3030
*
31+
* @param string $testFile The name of the file being tested.
32+
*
3133
* @return array<int, int> Key is the line number, value is the number of expected warnings.
3234
*/
33-
public function getWarningList() {
34-
return [
35-
27 => 1,
36-
28 => 1,
37-
29 => 1,
38-
30 => 1,
39-
32 => 1,
40-
33 => 1,
41-
34 => 1,
42-
35 => 1,
43-
37 => 1,
44-
38 => 1,
45-
39 => 1,
46-
40 => 1,
47-
47 => 1,
48-
52 => 1,
49-
56 => 1,
50-
74 => 1,
51-
75 => 1,
52-
76 => 1,
53-
77 => 1,
54-
78 => 1,
55-
79 => 1,
56-
82 => ( PHP_VERSION_ID > 50600 ) ? 0 : 1,
57-
88 => 1,
58-
94 => 1,
59-
95 => 1,
60-
105 => 1,
61-
112 => 1,
62-
113 => 1,
63-
114 => 1,
64-
115 => 1,
65-
116 => 1,
66-
119 => 1,
67-
120 => 1,
68-
123 => 1,
69-
];
35+
public function getWarningList( $testFile = '' ) {
36+
switch ( $testFile ) {
37+
case 'LowExpiryCacheTimeUnitTest.1.inc':
38+
return [
39+
27 => 1,
40+
28 => 1,
41+
29 => 1,
42+
30 => 1,
43+
32 => 1,
44+
33 => 1,
45+
34 => 1,
46+
35 => 1,
47+
37 => 1,
48+
38 => 1,
49+
39 => 1,
50+
40 => 1,
51+
47 => 1,
52+
52 => 1,
53+
56 => 1,
54+
74 => 1,
55+
75 => 1,
56+
76 => 1,
57+
77 => 1,
58+
78 => 1,
59+
79 => 1,
60+
82 => ( PHP_VERSION_ID > 50600 ) ? 0 : 1,
61+
88 => 1,
62+
94 => 1,
63+
95 => 1,
64+
105 => 1,
65+
112 => 1,
66+
113 => 1,
67+
114 => 1,
68+
115 => 1,
69+
116 => 1,
70+
119 => 1,
71+
120 => 1,
72+
123 => 1,
73+
126 => 1,
74+
127 => 1,
75+
133 => 1,
76+
134 => 1,
77+
148 => 1,
78+
149 => 1,
79+
150 => 1,
80+
151 => 1,
81+
152 => 1,
82+
160 => 1,
83+
161 => 1,
84+
];
85+
86+
default:
87+
return [];
88+
}
7089
}
7190
}

0 commit comments

Comments
 (0)