Skip to content

Commit d374355

Browse files
rebeccahumrebeccahum
authored
Merge pull request #587 from Automattic/fix/572-lowexpirytime-flag-4th-param-when-undetermined
LowExpiryCacheTime: add extra warning + lots of bug fixes
2 parents b0c6578 + 32aed75 commit d374355

File tree

3 files changed

+230
-21
lines changed

3 files changed

+230
-21
lines changed

‎WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php‎

Lines changed: 114 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,14 @@
77

88
namespace WordPressVIPMinimum\Sniffs\Performance;
99

10+
use PHP_CodeSniffer\Util\Tokens;
1011
use WordPressCS\WordPress\AbstractFunctionParameterSniff;
1112

1213
/**
1314
* This sniff throws a warning when low cache times are set.
1415
*
16+
* {@internal VIP uses the Memcached object cache implementation. {@link https://github.com/Automattic/wp-memcached}}
17+
*
1518
* @package VIPCS\WordPressVIPMinimum
1619
*
1720
* @since 0.4.0
@@ -69,21 +72,123 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p
6972
return;
7073
}
7174

72-
$time = $parameters[4]['raw'];
75+
$param = $parameters[4];
76+
$tokensAsString = '';
77+
$reportPtr = null;
78+
$openParens = 0;
79+
80+
for ( $i = $param['start']; $i <= $param['end']; $i++ ) {
81+
if ( isset( Tokens::$emptyTokens[ $this->tokens[ $i ]['code'] ] ) === true ) {
82+
$tokensAsString .= ' ';
83+
continue;
84+
}
85+
86+
if ( $this->tokens[ $i ]['code'] === T_NS_SEPARATOR ) {
87+
/*
88+
* Ignore namespace separators. If it's part of a global WP time constant, it will be
89+
* handled correctly. If it's used in any other context, another token *will* trigger the
90+
* "undetermined" warning anyway.
91+
*/
92+
continue;
93+
}
94+
95+
if ( isset( $reportPtr ) === false ) {
96+
// Set the report pointer to the first non-empty token we encounter.
97+
$reportPtr = $i;
98+
}
99+
100+
if ( $this->tokens[ $i ]['code'] === T_LNUMBER
101+
|| $this->tokens[ $i ]['code'] === T_DNUMBER
102+
) {
103+
// Integer or float.
104+
$tokensAsString .= $this->tokens[ $i ]['content'];
105+
continue;
106+
}
107+
108+
if ( $this->tokens[ $i ]['code'] === T_FALSE
109+
|| $this->tokens[ $i ]['code'] === T_NULL
110+
) {
111+
$tokensAsString .= 0;
112+
continue;
113+
}
114+
115+
if ( $this->tokens[ $i ]['code'] === T_TRUE ) {
116+
$tokensAsString .= 1;
117+
continue;
118+
}
119+
120+
if ( isset( Tokens::$arithmeticTokens[ $this->tokens[ $i ]['code'] ] ) === true ) {
121+
$tokensAsString .= $this->tokens[ $i ]['content'];
122+
continue;
123+
}
73124

74-
if ( is_numeric( $time ) === false ) {
75125
// If using time constants, we need to convert to a number.
76-
$time = str_replace( array_keys( $this->wp_time_constants ), $this->wp_time_constants, $time );
126+
if ( $this->tokens[ $i ]['code'] === T_STRING
127+
&& isset( $this->wp_time_constants[ $this->tokens[ $i ]['content'] ] ) === true
128+
) {
129+
$tokensAsString .= $this->wp_time_constants[ $this->tokens[ $i ]['content'] ];
130+
continue;
131+
}
132+
133+
if ( $this->tokens[ $i ]['code'] === T_OPEN_PARENTHESIS ) {
134+
$tokensAsString .= $this->tokens[ $i ]['content'];
135+
++$openParens;
136+
continue;
137+
}
138+
139+
if ( $this->tokens[ $i ]['code'] === T_CLOSE_PARENTHESIS ) {
140+
$tokensAsString .= $this->tokens[ $i ]['content'];
141+
--$openParens;
142+
continue;
143+
}
77144

78-
if ( preg_match( '#^[\s\d+*/-]+$#', $time ) > 0 ) {
79-
$time = eval( "return $time;" ); // phpcs:ignore Squiz.PHP.Eval -- No harm here.
145+
if ( $this->tokens[ $i ]['code'] === T_CONSTANT_ENCAPSED_STRING ) {
146+
$content = $this->strip_quotes( $this->tokens[ $i ]['content'] );
147+
if ( is_numeric( $content ) === true ) {
148+
$tokensAsString .= $content;
149+
continue;
150+
}
80151
}
152+
153+
// Encountered an unexpected token. Manual inspection needed.
154+
$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"';
155+
$data = [ $matched_content, $parameters[4]['raw'] ];
156+
$this->phpcsFile->addWarning( $message, $reportPtr, 'CacheTimeUndetermined', $data );
157+
158+
return;
159+
}
160+
161+
if ( $tokensAsString === '' ) {
162+
// Nothing found to evaluate.
163+
return;
81164
}
82165

83-
if ( $time < 300 ) {
84-
$message = 'Low cache expiry time of "%s", it is recommended to have 300 seconds or more.';
85-
$data = [ $parameters[4]['raw'] ];
86-
$this->phpcsFile->addWarning( $message, $stackPtr, 'LowCacheTime', $data );
166+
$tokensAsString = trim( $tokensAsString );
167+
168+
if ( $openParens !== 0 ) {
169+
/*
170+
* Shouldn't be possible as that would indicate a parse error in the original code,
171+
* but let's prevent getting parse errors in the `eval`-ed code.
172+
*/
173+
if ( $openParens > 0 ) {
174+
$tokensAsString .= str_repeat( ')', $openParens );
175+
} else {
176+
$tokensAsString = str_repeat( '(', abs( $openParens ) ) . $tokensAsString;
177+
}
178+
}
179+
180+
$time = eval( "return $tokensAsString;" ); // phpcs:ignore Squiz.PHP.Eval -- No harm here.
181+
182+
if ( $time < 300 && (int) $time !== 0 ) {
183+
$message = 'Low cache expiry time of %s seconds detected. It is recommended to have 300 seconds or more.';
184+
$data = [ $time ];
185+
186+
if ( (string) $time !== $tokensAsString ) {
187+
$message .= ' Found: "%s"';
188+
$data[] = $tokensAsString;
189+
}
190+
191+
$this->phpcsFile->addWarning( $message, $reportPtr, 'LowCacheTime', $data );
87192
}
88193
}
89194
}

‎WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,3 +38,86 @@ wp_cache_replace( 'test', $data, $group, 100 ); // Lower than 300.
3838
wp_cache_replace( 'test', $data, $group, 2*MINUTE_IN_SECONDS ); // Lower than 300.
3939
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.
41+
42+
// Test error being reported on the line containing the parameter.
43+
wp_cache_replace(
44+
$testing,
45+
$data,
46+
'',
47+
1.5 * MINUTE_IN_SECONDS // Lower than 300.
48+
);
49+
50+
// Test calculations with floats.
51+
wp_cache_replace( $testing, $data, '', 7.5 * MINUTE_IN_SECONDS ); // OK.
52+
wp_cache_replace( $testing, $data, '', 500 * 0.1 ); // Bad.
53+
54+
// Test comment handling.
55+
wp_cache_add( 'test', $data, $group, /* Deliberately left empty */ ); // OK.
56+
wp_cache_add( 'test', $data, $group, 600 * 0.1 /* = 1 minute */ ); // Bad.
57+
wp_cache_add(
58+
'test',
59+
$data,
60+
$group,
61+
// Cache for 10 minutes.
62+
600
63+
); // OK.
64+
65+
wp_cache_add(
66+
'test',
67+
$data,
68+
$group,
69+
// phpcs:ignore Stnd.Cat.Sniff -- Just here for testing purposes.
70+
600
71+
); // OK.
72+
73+
// Test variable/constant with or without calculation being passed.
74+
wp_cache_set( $key, $data, '', $time ); // Manual inspection warning.
75+
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.
79+
wp_cache_set( $key, $data, '', PREFIX_CUSTOM_TIME * 5); // Manual inspection warning.
80+
81+
// Test calculations with additional aritmetic operators.
82+
wp_cache_replace( 'test', $data, $group, +5 ** MINUTE_IN_SECONDS ); // OK.
83+
wp_cache_add( 'test', $data, $group, WEEK_IN_SECONDS / 3 + HOUR_IN_SECONDS ); // OK.
84+
85+
// Test calculations grouped with parentheses.
86+
wp_cache_set( $key, $data, '', (24 * 60 * 60) ); // OK.
87+
wp_cache_set( $key, $data, '', (-(2 * 60) + 600) ); // OK.
88+
wp_cache_set( $key, $data, '', (2 * 60) ); // Bad.
89+
wp_cache_set( $key, $data, '', (-(2 * 60) + 600 ); // OK - includes parse error, close parenthesis missing.
90+
91+
// Test handling of numbers passed as strings.
92+
wp_cache_set( 'test', $data, $group, '300' ); // OK - type cast to integer within the function.
93+
wp_cache_set( 'test', $data, $group, '100' * 3 ); // OK - type cast to integer by PHP during the calculation.
94+
wp_cache_set( 'test', $data, $group, '-10' ); // Bad - type cast to integer within the function.
95+
wp_cache_replace( $testing, $data, '', '1.5' * MINUTE_IN_SECONDS ); // Bad - type cast to integer by PHP during the calculation.
96+
97+
// Test handling of 0 values. `0` is the default value for the parameter and translates internally to "no expiration".
98+
wp_cache_add( 'test', $data, $group, 0 ); // OK.
99+
wp_cache_add( 'test', $data, $group, 0.0 ); // OK.
100+
wp_cache_add( 'test', $data, $group, '0' ); // OK.
101+
wp_cache_add( 'test', $data, $group, false ); // OK.
102+
wp_cache_add( 'test', $data, $group, null ); // OK.
103+
104+
// Test handling of other scalar values.
105+
wp_cache_add( 'test', $data, $group, true ); // Bad - becomes integer 1.
106+
107+
// Test passing just and only one of the time constants, including passing it as an FQN.
108+
wp_cache_set( 'test', $data, $group, HOUR_IN_SECONDS ); // OK.
109+
wp_cache_set( 'test', $data, $group, \MONTH_IN_SECONDS ); // OK.
110+
111+
// Test passing something which may look like one of the time constants, but isn't.
112+
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.
114+
wp_cache_set( 'test', $data, $group, $obj->MONTH_IN_SECONDS ); // Bad - not a constant.
115+
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.
117+
118+
// Test passing negative number as cache time.
119+
wp_cache_set( 'test', $data, $group, -300 ); // Bad.
120+
wp_cache_add( $testing, $data, 'test_group', -6 * MINUTE_IN_SECONDS ); // Bad.
121+
122+
// Test more complex logic in the parameter.
123+
wp_cache_add( $key, $data, '', ($toggle ? 200 : 400) ); // Manual inspection warning.

‎WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php‎

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -34,18 +34,39 @@ public function getErrorList() {
3434
*/
3535
public function getWarningList() {
3636
return [
37-
27 => 1,
38-
28 => 1,
39-
29 => 1,
40-
30 => 1,
41-
32 => 1,
42-
33 => 1,
43-
34 => 1,
44-
35 => 1,
45-
37 => 1,
46-
38 => 1,
47-
39 => 1,
48-
40 => 1,
37+
27 => 1,
38+
28 => 1,
39+
29 => 1,
40+
30 => 1,
41+
32 => 1,
42+
33 => 1,
43+
34 => 1,
44+
35 => 1,
45+
37 => 1,
46+
38 => 1,
47+
39 => 1,
48+
40 => 1,
49+
47 => 1,
50+
52 => 1,
51+
56 => 1,
52+
74 => 1,
53+
75 => 1,
54+
76 => 1,
55+
77 => 1,
56+
78 => 1,
57+
79 => 1,
58+
88 => 1,
59+
94 => 1,
60+
95 => 1,
61+
105 => 1,
62+
112 => 1,
63+
113 => 1,
64+
114 => 1,
65+
115 => 1,
66+
116 => 1,
67+
119 => 1,
68+
120 => 1,
69+
123 => 1,
4970
];
5071
}
5172
}

0 commit comments

Comments
 (0)