From 8f1aa77d9a0131811e65ecd1770eb26dcab6dd76 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 8 Oct 2020 12:55:48 +0200 Subject: [PATCH 01/14] LowExpiryCacheTime: improve error message The original error message read: > Low cache expiry time of "%s", it is recommended to have 300 seconds or more. ... with the `%s` being replaced with the contents of the parameter, like: > Low cache expiry time of "2 * MINUTE_IN_SECONDS", it is recommended to have 300 seconds or more. While this will be clear cut enough when a hard-coded integer is passed, there is a mental overhead as soon as a calculation is involved. This adjusts the error message to be more descriptive: * Displaying the calculated time and annotating this is in seconds. * Only displaying the content of the parameter passed when it is not a hard-coded integer. * Improving the grammar of the message. So, the above example message will now become: > Low cache expiry time of 120 seconds detected. It is recommended to have 300 seconds or more. Found: "2 * MINUTE_IN_SECONDS" --- .../Sniffs/Performance/LowExpiryCacheTimeSniff.php | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php index 34ea78e6..84a7e2fb 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php @@ -81,8 +81,14 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p } if ( $time < 300 ) { - $message = 'Low cache expiry time of "%s", it is recommended to have 300 seconds or more.'; - $data = [ $parameters[4]['raw'] ]; + $message = 'Low cache expiry time of %s seconds detected. It is recommended to have 300 seconds or more.'; + $data = [ $time ]; + + if ( $time !== trim( $parameters[4]['raw'] ) ) { + $message .= ' Found: "%s"'; + $data[] = $parameters[4]['raw']; + } + $this->phpcsFile->addWarning( $message, $stackPtr, 'LowCacheTime', $data ); } } From d7f92aacd4e8306f7cbc4dad650051662ffc864e Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 8 Oct 2020 18:41:20 +0200 Subject: [PATCH 02/14] LowExpiryCacheTime: improve error line precision The error would originally be reported on the line containing the function call to the `wp_cache_*()` function. This changes that to report on the line containing the actual parameter, which for multi-line function calls may not be on the same line as where the function call keyword is found. Includes unit test. --- .../Sniffs/Performance/LowExpiryCacheTimeSniff.php | 8 ++++++-- .../Tests/Performance/LowExpiryCacheTimeUnitTest.inc | 8 ++++++++ .../Tests/Performance/LowExpiryCacheTimeUnitTest.php | 1 + 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php index 84a7e2fb..51f7c109 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php @@ -7,6 +7,7 @@ namespace WordPressVIPMinimum\Sniffs\Performance; +use PHP_CodeSniffer\Util\Tokens; use WordPressCS\WordPress\AbstractFunctionParameterSniff; /** @@ -69,7 +70,8 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p return; } - $time = $parameters[4]['raw']; + $param = $parameters[4]; + $time = $param['raw']; if ( is_numeric( $time ) === false ) { // If using time constants, we need to convert to a number. @@ -89,7 +91,9 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $data[] = $parameters[4]['raw']; } - $this->phpcsFile->addWarning( $message, $stackPtr, 'LowCacheTime', $data ); + $reportPtr = $this->phpcsFile->findNext( Tokens::$emptyTokens, $param['start'], $param['end'], true ); + + $this->phpcsFile->addWarning( $message, $reportPtr, 'LowCacheTime', $data ); } } } diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc index eb5a63cd..2f40479d 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc @@ -38,3 +38,11 @@ wp_cache_replace( 'test', $data, $group, 100 ); // Lower than 300. wp_cache_replace( 'test', $data, $group, 2*MINUTE_IN_SECONDS ); // Lower than 300. wp_cache_replace( 123, $data, null, 1.5 * MINUTE_IN_SECONDS ); // Lower than 300. wp_cache_replace( $testing, $data, '', 1.5 * MINUTE_IN_SECONDS ); // Lower than 300. + +// Test error being reported on the line containing the parameter. +wp_cache_replace( + $testing, + $data, + '', + 1.5 * MINUTE_IN_SECONDS // Lower than 300. +); diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php index 81a4da60..7ee3c02e 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php @@ -46,6 +46,7 @@ public function getWarningList() { 38 => 1, 39 => 1, 40 => 1, + 47 => 1, ]; } } From 74d8b43383be1e7a0de8bd36a62c60e67b1164e3 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 8 Oct 2020 13:35:21 +0200 Subject: [PATCH 03/14] LowExpiryCacheTime: bug fix - calculations with simple floats Until now, calculations using a floating point number would not be handled correctly as floating point numbers were not recognized in the regex and therefore not evaluated. This means that the `( $time < 300 )` comparison would use the raw parameter text value with the replaced time constants for the comparison. To illustrate: * `1.5 * MINUTE_IN_SECONDS` is not numeric, so the condition would be entered. * In the condition the `MINUTE_IN_SECONDS` would be replaced with the actual numeric value, becoming `'1.5 * 60'`, * ... which, due to the `.` in the float `1.5`, would not match the regex and therefore would not be `eval`uated, * Ultimately resulting in a comparison of `if ( '1.5 * 60' < 300 )` - take note of the `1.5 * 60` still being a string! -, * ... which, due to the string to integer type juggle rules, would effectively become `if ( 1.5 < 300 )` - take not of the string now having been changed to a float -... PHP type juggle logic which was applied: > If the string starts with valid numeric data, this will be the value used. Otherwise, the value will be 0 (zero). Valid numeric data is an optional sign, followed by one or more digits (optionally containing a decimal point), followed by an optional exponent. The exponent is an 'e' or 'E' followed by one or more digits. Source: https://www.php.net/manual/en/language.types.string.php This would ultimately result in both false positives as well as false negatives, as illustrated by the added unit tests which would previously both fail, the first with a false positive, the second with a false negative. --- .../Sniffs/Performance/LowExpiryCacheTimeSniff.php | 2 +- .../Tests/Performance/LowExpiryCacheTimeUnitTest.inc | 4 ++++ .../Tests/Performance/LowExpiryCacheTimeUnitTest.php | 1 + 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php index 51f7c109..4f6c61dc 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php @@ -77,7 +77,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p // If using time constants, we need to convert to a number. $time = str_replace( array_keys( $this->wp_time_constants ), $this->wp_time_constants, $time ); - if ( preg_match( '#^[\s\d+*/-]+$#', $time ) > 0 ) { + if ( preg_match( '#^[\s\d+\.*/-]+$#', $time ) > 0 ) { $time = eval( "return $time;" ); // phpcs:ignore Squiz.PHP.Eval -- No harm here. } } diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc index 2f40479d..b26cf1f6 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc @@ -46,3 +46,7 @@ wp_cache_replace( '', 1.5 * MINUTE_IN_SECONDS // Lower than 300. ); + +// Test calculations with floats. +wp_cache_replace( $testing, $data, '', 7.5 * MINUTE_IN_SECONDS ); // OK. +wp_cache_replace( $testing, $data, '', 500 * 0.1 ); // Bad. diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php index 7ee3c02e..fff01238 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php @@ -47,6 +47,7 @@ public function getWarningList() { 39 => 1, 40 => 1, 47 => 1, + 52 => 1, ]; } } From 1e0e6b32fc9f5a3c3b5aedded767d2997f66830a Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 8 Oct 2020 16:43:47 +0200 Subject: [PATCH 04/14] LowExpiryCacheTime: bug fix - allow for comments As the sniff examined the `raw` content of the parameter instead of walking the tokens, the sniff would get confused over comments found within the parameter. As the `raw` content would not be seen as _numeric_ when there is a comment, these parameter values would be subject to the same _string to integer_ type juggle rules as we saw with the handling of floats, leading to both false negatives as well as false positives. This is now fixed by changing the sniff to no longer use the `raw` parameter content, but to use token walking instead and build up the string to be `eval`uated based on the tokens encountered. The token walking is 100% based on the previous implementation - i.e. it allows for the same characters as were previously allowed via the regex -, with the only change in behaviour being the handling of comments. (well... it also adds an allowance for floats written with exponentials, but that would be an edge case no matter what) Includes unit tests. Aside from the very first new test (`/* Deliberately left empty */`), each of these would give the opposite result prior to this fix. Includes making the error message text cleaner for compound parameters, by displaying just the text string used to calculate the time, excluding comments. --- .../Performance/LowExpiryCacheTimeSniff.php | 48 +++++++++++++++---- .../LowExpiryCacheTimeUnitTest.inc | 19 ++++++++ .../LowExpiryCacheTimeUnitTest.php | 1 + 3 files changed, 59 insertions(+), 9 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php index 4f6c61dc..abbc4d57 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php @@ -70,25 +70,55 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p return; } - $param = $parameters[4]; - $time = $param['raw']; + $param = $parameters[4]; + $tokensAsString = ''; - if ( is_numeric( $time ) === false ) { - // If using time constants, we need to convert to a number. - $time = str_replace( array_keys( $this->wp_time_constants ), $this->wp_time_constants, $time ); + for ( $i = $param['start']; $i <= $param['end']; $i++ ) { + if ( isset( Tokens::$emptyTokens[ $this->tokens[ $i ]['code'] ] ) === true ) { + $tokensAsString .= ' '; + continue; + } + + if ( $this->tokens[ $i ]['code'] === T_LNUMBER + || $this->tokens[ $i ]['code'] === T_DNUMBER + ) { + // Integer or float. + $tokensAsString .= $this->tokens[ $i ]['content']; + continue; + } - if ( preg_match( '#^[\s\d+\.*/-]+$#', $time ) > 0 ) { - $time = eval( "return $time;" ); // phpcs:ignore Squiz.PHP.Eval -- No harm here. + if ( $this->tokens[ $i ]['code'] === T_MULTIPLY + || $this->tokens[ $i ]['code'] === T_DIVIDE + || $this->tokens[ $i ]['code'] === T_MINUS + ) { + $tokensAsString .= $this->tokens[ $i ]['content']; + continue; + } + + // If using time constants, we need to convert to a number. + if ( $this->tokens[ $i ]['code'] === T_STRING + && isset( $this->wp_time_constants[ $this->tokens[ $i ]['content'] ] ) === true + ) { + $tokensAsString .= $this->wp_time_constants[ $this->tokens[ $i ]['content'] ]; + continue; } } + if ( $tokensAsString === '' ) { + // Nothing found to evaluate. + return; + } + + $tokensAsString = trim( $tokensAsString ); + $time = eval( "return $tokensAsString;" ); // phpcs:ignore Squiz.PHP.Eval -- No harm here. + if ( $time < 300 ) { $message = 'Low cache expiry time of %s seconds detected. It is recommended to have 300 seconds or more.'; $data = [ $time ]; - if ( $time !== trim( $parameters[4]['raw'] ) ) { + if ( (string) $time !== $tokensAsString ) { $message .= ' Found: "%s"'; - $data[] = $parameters[4]['raw']; + $data[] = $tokensAsString; } $reportPtr = $this->phpcsFile->findNext( Tokens::$emptyTokens, $param['start'], $param['end'], true ); diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc index b26cf1f6..2fa359ab 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc @@ -50,3 +50,22 @@ wp_cache_replace( // Test calculations with floats. wp_cache_replace( $testing, $data, '', 7.5 * MINUTE_IN_SECONDS ); // OK. wp_cache_replace( $testing, $data, '', 500 * 0.1 ); // Bad. + +// Test comment handling. +wp_cache_add( 'test', $data, $group, /* Deliberately left empty */ ); // OK. +wp_cache_add( 'test', $data, $group, 600 * 0.1 /* = 1 minute */ ); // Bad. +wp_cache_add( + 'test', + $data, + $group, + // Cache for 10 minutes. + 600 +); // OK. + +wp_cache_add( + 'test', + $data, + $group, + // phpcs:ignore Stnd.Cat.Sniff -- Just here for testing purposes. + 600 +); // OK. diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php index fff01238..1544f76a 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php @@ -48,6 +48,7 @@ public function getWarningList() { 40 => 1, 47 => 1, 52 => 1, + 56 => 1, ]; } } From c67a36a2e13efcb02be5926ab22dd8048a3b02d5 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 8 Oct 2020 18:58:00 +0200 Subject: [PATCH 05/14] LowExpiryCacheTime: minor efficiency tweak As the error determination has now switched to token walking, we may as well set the `$reportPtr` while walking the tokens, instead of using the `findNext()` method. --- .../Sniffs/Performance/LowExpiryCacheTimeSniff.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php index abbc4d57..e1a5fd59 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php @@ -72,6 +72,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $param = $parameters[4]; $tokensAsString = ''; + $reportPtr = null; for ( $i = $param['start']; $i <= $param['end']; $i++ ) { if ( isset( Tokens::$emptyTokens[ $this->tokens[ $i ]['code'] ] ) === true ) { @@ -79,6 +80,11 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p continue; } + if ( isset( $reportPtr ) === false ) { + // Set the report pointer to the first non-empty token we encounter. + $reportPtr = $i; + } + if ( $this->tokens[ $i ]['code'] === T_LNUMBER || $this->tokens[ $i ]['code'] === T_DNUMBER ) { @@ -121,8 +127,6 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $data[] = $tokensAsString; } - $reportPtr = $this->phpcsFile->findNext( Tokens::$emptyTokens, $param['start'], $param['end'], true ); - $this->phpcsFile->addWarning( $message, $reportPtr, 'LowCacheTime', $data ); } } From 54f6db0fd9de5f2500d56a4fd2d429f90839dedf Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 8 Oct 2020 18:35:19 +0200 Subject: [PATCH 06/14] LowExpiryCacheTime: add new warning / manual inspection needed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This adds a new warning to manual inspect that the cache time is more than 300 seconds when an unexpected token (variable/non-WP-time constant) is encountered. This also prevents the sniff from throwing parse errors for the `eval`-ed code when unexpected tokens are encountered, as the `$tokensAsString` text string could easily contain parse errors now it is based on token walking instead of the regex-based check. In other words: we either need to account for all possible tokens, òr, like is done now, bow out with (or without) a warning when unexpected tokens are encountered. Fixes 572 Includes unit tests. --- .../Sniffs/Performance/LowExpiryCacheTimeSniff.php | 7 +++++++ .../Tests/Performance/LowExpiryCacheTimeUnitTest.inc | 8 ++++++++ .../Tests/Performance/LowExpiryCacheTimeUnitTest.php | 6 ++++++ 3 files changed, 21 insertions(+) diff --git a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php index e1a5fd59..7d728b89 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php @@ -108,6 +108,13 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $tokensAsString .= $this->wp_time_constants[ $this->tokens[ $i ]['content'] ]; continue; } + + // Encountered an unexpected token. Manual inspection needed. + $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"'; + $data = [ $matched_content, $parameters[4]['raw'] ]; + $this->phpcsFile->addWarning( $message, $reportPtr, 'CacheTimeUndetermined', $data ); + + return; } if ( $tokensAsString === '' ) { diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc index 2fa359ab..3beff4d5 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc @@ -69,3 +69,11 @@ wp_cache_add( // phpcs:ignore Stnd.Cat.Sniff -- Just here for testing purposes. 600 ); // OK. + +// Test variable/constant with or without calculation being passed. +wp_cache_set( $key, $data, '', $time ); // Manual inspection warning. +wp_cache_set( $key, $data, '', PREFIX_FIVE_MINUTES ); // Manual inspection warning. +wp_cache_set( $key, $data, '', 20 * $time ); // Manual inspection warning. +wp_cache_set( $key, $data, '', $base + $extra ); // Manual inspection warning. +wp_cache_set( $key, $data, '', 300 + $extra ); // Manual inspection warning. +wp_cache_set( $key, $data, '', PREFIX_CUSTOM_TIME * 5); // Manual inspection warning. diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php index 1544f76a..5f0d67ca 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php @@ -49,6 +49,12 @@ public function getWarningList() { 47 => 1, 52 => 1, 56 => 1, + 74 => 1, + 75 => 1, + 76 => 1, + 77 => 1, + 78 => 1, + 79 => 1, ]; } } From dae7c028e5087508a4fc1d42ff4de06ab170419d Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 8 Oct 2020 19:14:16 +0200 Subject: [PATCH 07/14] LowExpiryCacheTime: bug fix - allow for all arithmetic operators While the `*`, `/` and `-` operators may be the ones most commonly used in the calculations for cache time, PHP contains more arithmetic operators, so let's properly account for them all. Not doing so would (and did) result in false positives/negatives previously. Includes unit tests. --- .../Sniffs/Performance/LowExpiryCacheTimeSniff.php | 5 +---- .../Tests/Performance/LowExpiryCacheTimeUnitTest.inc | 4 ++++ 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php index 7d728b89..bbc4ebdd 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php @@ -93,10 +93,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p continue; } - if ( $this->tokens[ $i ]['code'] === T_MULTIPLY - || $this->tokens[ $i ]['code'] === T_DIVIDE - || $this->tokens[ $i ]['code'] === T_MINUS - ) { + if ( isset( Tokens::$arithmeticTokens[ $this->tokens[ $i ]['code'] ] ) === true ) { $tokensAsString .= $this->tokens[ $i ]['content']; continue; } diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc index 3beff4d5..3b865b9a 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc @@ -77,3 +77,7 @@ wp_cache_set( $key, $data, '', 20 * $time ); // Manual inspection warning. wp_cache_set( $key, $data, '', $base + $extra ); // Manual inspection warning. wp_cache_set( $key, $data, '', 300 + $extra ); // Manual inspection warning. wp_cache_set( $key, $data, '', PREFIX_CUSTOM_TIME * 5); // Manual inspection warning. + +// Test calculations with additional aritmetic operators. +wp_cache_replace( 'test', $data, $group, +5 ** MINUTE_IN_SECONDS ); // OK. +wp_cache_add( 'test', $data, $group, WEEK_IN_SECONDS / 3 + HOUR_IN_SECONDS ); // OK. From 67d54cdabef67323bab309afb26ed7769e83587d Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 8 Oct 2020 19:25:58 +0200 Subject: [PATCH 08/14] LowExpiryCacheTime: bug fix - allow for parentheses grouping calculations Depending on code style rules, it is quite common for calculations to be grouped within parentheses. This adjusts the code to allow for this and prevents false positive "manual inspection" warnings for those type of grouped calculations. The grouped calculation will now be properly `eval`uated and only throw a warning if the resulting cache time is lower than 300. Includes unit tests. --- .../Performance/LowExpiryCacheTimeSniff.php | 28 ++++++++++++++++++- .../LowExpiryCacheTimeUnitTest.inc | 6 ++++ .../LowExpiryCacheTimeUnitTest.php | 1 + 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php index bbc4ebdd..76d6db5d 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php @@ -73,6 +73,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $param = $parameters[4]; $tokensAsString = ''; $reportPtr = null; + $openParens = 0; for ( $i = $param['start']; $i <= $param['end']; $i++ ) { if ( isset( Tokens::$emptyTokens[ $this->tokens[ $i ]['code'] ] ) === true ) { @@ -106,6 +107,18 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p continue; } + if ( $this->tokens[ $i ]['code'] === T_OPEN_PARENTHESIS ) { + $tokensAsString .= $this->tokens[ $i ]['content']; + ++$openParens; + continue; + } + + if ( $this->tokens[ $i ]['code'] === T_CLOSE_PARENTHESIS ) { + $tokensAsString .= $this->tokens[ $i ]['content']; + --$openParens; + continue; + } + // Encountered an unexpected token. Manual inspection needed. $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"'; $data = [ $matched_content, $parameters[4]['raw'] ]; @@ -120,7 +133,20 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p } $tokensAsString = trim( $tokensAsString ); - $time = eval( "return $tokensAsString;" ); // phpcs:ignore Squiz.PHP.Eval -- No harm here. + + if ( $openParens !== 0 ) { + /* + * Shouldn't be possible as that would indicate a parse error in the original code, + * but let's prevent getting parse errors in the `eval`-ed code. + */ + if ( $openParens > 0 ) { + $tokensAsString .= str_repeat( ')', $openParens ); + } else { + $tokensAsString = str_repeat( '(', abs( $openParens ) ) . $tokensAsString; + } + } + + $time = eval( "return $tokensAsString;" ); // phpcs:ignore Squiz.PHP.Eval -- No harm here. if ( $time < 300 ) { $message = 'Low cache expiry time of %s seconds detected. It is recommended to have 300 seconds or more.'; diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc index 3b865b9a..60e03ee9 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc @@ -81,3 +81,9 @@ wp_cache_set( $key, $data, '', PREFIX_CUSTOM_TIME * 5); // Manual inspection war // Test calculations with additional aritmetic operators. wp_cache_replace( 'test', $data, $group, +5 ** MINUTE_IN_SECONDS ); // OK. wp_cache_add( 'test', $data, $group, WEEK_IN_SECONDS / 3 + HOUR_IN_SECONDS ); // OK. + +// Test calculations grouped with parentheses. +wp_cache_set( $key, $data, '', (24 * 60 * 60) ); // OK. +wp_cache_set( $key, $data, '', (-(2 * 60) + 600) ); // OK. +wp_cache_set( $key, $data, '', (2 * 60) ); // Bad. +wp_cache_set( $key, $data, '', (-(2 * 60) + 600 ); // OK - includes parse error, close parenthesis missing. diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php index 5f0d67ca..1ef15de5 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php @@ -55,6 +55,7 @@ public function getWarningList() { 77 => 1, 78 => 1, 79 => 1, + 88 => 1, ]; } } From 5f32d2f25f042c0c22d00e3e977f91eeb78b17ba Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 8 Oct 2020 19:46:43 +0200 Subject: [PATCH 09/14] LowExpiryCacheTime: bug fix - allow for cache time being passed as a string There is no type declaration in effect for the `$expire` parameter of the `wp_cache_*()` functions and in each of the functions, the parameter is type cast to integer straight away. In the WP native implementation, this is done within the `wp_cache_set()`, wp_cache_add()` and `wp_cache_replace()` functions before these pass off to the `$wp_object_cache->set/add/replace()` methods. See: * https://developer.wordpress.org/reference/functions/wp_cache_set/#source * https://developer.wordpress.org/reference/functions/wp_cache_add/#source * https://developer.wordpress.org/reference/functions/wp_cache_replace/#source In the VIP object cache implementation this is done via a call to `intval()` from within the `$wp_object_cache->set/add/replace()` methods before the `$expire` parameter is used. See: * `add()`: https://github.com/Automattic/wp-memcached/blob/811243804a892a4609a4581d9479fa80c4e4ac8d/object-cache.php#L164 * `replace()`: https://github.com/Automattic/wp-memcached/blob/811243804a892a4609a4581d9479fa80c4e4ac8d/object-cache.php#L474 * `set()`: https://github.com/Automattic/wp-memcached/blob/811243804a892a4609a4581d9479fa80c4e4ac8d/object-cache.php#L522 In other words, in reality, any input which can be type cast to integer is accepted by the function. Aside from that, PHP will type cast to integer internally anyway before the parameter even reaches the function, if a calculation is done/an arithmetic operator is encountered. As things were, a "manual inspection" warning would be thrown for any parameter containing a text string, while for numeric text strings, this is not needed as we can actually calculate the cache time correctly. This adjusts the code to allow for numeric text strings being passed to the function, either on their own or as part of a calculation. While using `is_numeric()` here is not 100% correct, I deem this case a rare one to begin with, so further refinement can be done if bugs would be reported and/or when PHPCSUtils is implemented. Includes unit tests. --- .../Sniffs/Performance/LowExpiryCacheTimeSniff.php | 8 ++++++++ .../Tests/Performance/LowExpiryCacheTimeUnitTest.inc | 6 ++++++ .../Tests/Performance/LowExpiryCacheTimeUnitTest.php | 2 ++ 3 files changed, 16 insertions(+) diff --git a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php index 76d6db5d..ce18b459 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php @@ -119,6 +119,14 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p continue; } + if ( $this->tokens[ $i ]['code'] === T_CONSTANT_ENCAPSED_STRING ) { + $content = $this->strip_quotes( $this->tokens[ $i ]['content'] ); + if ( is_numeric( $content ) === true ) { + $tokensAsString .= $content; + continue; + } + } + // Encountered an unexpected token. Manual inspection needed. $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"'; $data = [ $matched_content, $parameters[4]['raw'] ]; diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc index 60e03ee9..f55db893 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc @@ -87,3 +87,9 @@ wp_cache_set( $key, $data, '', (24 * 60 * 60) ); // OK. wp_cache_set( $key, $data, '', (-(2 * 60) + 600) ); // OK. wp_cache_set( $key, $data, '', (2 * 60) ); // Bad. wp_cache_set( $key, $data, '', (-(2 * 60) + 600 ); // OK - includes parse error, close parenthesis missing. + +// Test handling of numbers passed as strings. +wp_cache_set( 'test', $data, $group, '300' ); // OK - type cast to integer within the function. +wp_cache_set( 'test', $data, $group, '100' * 3 ); // OK - type cast to integer by PHP during the calculation. +wp_cache_set( 'test', $data, $group, '-10' ); // Bad - type cast to integer within the function. +wp_cache_replace( $testing, $data, '', '1.5' * MINUTE_IN_SECONDS ); // Bad - type cast to integer by PHP during the calculation. diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php index 1ef15de5..78e8c8d2 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php @@ -56,6 +56,8 @@ public function getWarningList() { 78 => 1, 79 => 1, 88 => 1, + 94 => 1, + 95 => 1, ]; } } From 244df86d0aeb5477d4d3faf346f63c24ea2c3bfe Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 9 Oct 2020 00:29:33 +0200 Subject: [PATCH 10/14] LowExpiryCacheTime: bug fix - allow for a cache time of 0 The default value of the `$expire` parameter in both the WP native, as well as the VIP object cache implementation, is `0` which translates to "no expiration", or in the VIP object cache to the "default expiration". In other words, even though `0` is lower than `300`, it should not be flagged and should be accepted as a valid value for the cache time. As, as explained in the previous commits, the value of the parameter is cast to integer, non-integer ways of passing the parameter which result in `0`, like passing `false` or `null`, should also be taken into account. Includes unit tests. Each of which would previously have resulted in one of the two warnings. --- .../Sniffs/Performance/LowExpiryCacheTimeSniff.php | 9 ++++++++- .../Tests/Performance/LowExpiryCacheTimeUnitTest.inc | 7 +++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php index ce18b459..6910fbcc 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php @@ -94,6 +94,13 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p continue; } + if ( $this->tokens[ $i ]['code'] === T_FALSE + || $this->tokens[ $i ]['code'] === T_NULL + ) { + $tokensAsString .= 0; + continue; + } + if ( isset( Tokens::$arithmeticTokens[ $this->tokens[ $i ]['code'] ] ) === true ) { $tokensAsString .= $this->tokens[ $i ]['content']; continue; @@ -156,7 +163,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $time = eval( "return $tokensAsString;" ); // phpcs:ignore Squiz.PHP.Eval -- No harm here. - if ( $time < 300 ) { + if ( $time < 300 && (int) $time !== 0 ) { $message = 'Low cache expiry time of %s seconds detected. It is recommended to have 300 seconds or more.'; $data = [ $time ]; diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc index f55db893..00d7f338 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc @@ -93,3 +93,10 @@ wp_cache_set( 'test', $data, $group, '300' ); // OK - type cast to integer withi wp_cache_set( 'test', $data, $group, '100' * 3 ); // OK - type cast to integer by PHP during the calculation. wp_cache_set( 'test', $data, $group, '-10' ); // Bad - type cast to integer within the function. wp_cache_replace( $testing, $data, '', '1.5' * MINUTE_IN_SECONDS ); // Bad - type cast to integer by PHP during the calculation. + +// Test handling of 0 values. `0` is the default value for the parameter and translates internally to "no expiration". +wp_cache_add( 'test', $data, $group, 0 ); // OK. +wp_cache_add( 'test', $data, $group, 0.0 ); // OK. +wp_cache_add( 'test', $data, $group, '0' ); // OK. +wp_cache_add( 'test', $data, $group, false ); // OK. +wp_cache_add( 'test', $data, $group, null ); // OK. From 497f6557f83376eff37aef4bc982119a74e72426 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 9 Oct 2020 00:34:30 +0200 Subject: [PATCH 11/14] LowExpiryCacheTime: feature completeness - allow for `true` As `false`, `null`, integers, floats and plain text strings are now all handled correctly by the sniff, it makes sense to also handle `true` for feature completeness. Along the same lines as detailed in previous commits, `true` would be cast to integer and would become `1`, so let's handle it as such. Includes unit test. This test would previously result in a "undetermined" error, while it will now, correctly, report on the cache time being too low. --- .../Performance/LowExpiryCacheTimeSniff.php | 5 ++ .../LowExpiryCacheTimeUnitTest.inc | 3 ++ .../LowExpiryCacheTimeUnitTest.php | 49 ++++++++++--------- 3 files changed, 33 insertions(+), 24 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php index 6910fbcc..95ad8aff 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php @@ -101,6 +101,11 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p continue; } + if ( $this->tokens[ $i ]['code'] === T_TRUE ) { + $tokensAsString .= 1; + continue; + } + if ( isset( Tokens::$arithmeticTokens[ $this->tokens[ $i ]['code'] ] ) === true ) { $tokensAsString .= $this->tokens[ $i ]['content']; continue; diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc index 00d7f338..cee00932 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc @@ -100,3 +100,6 @@ wp_cache_add( 'test', $data, $group, 0.0 ); // OK. wp_cache_add( 'test', $data, $group, '0' ); // OK. wp_cache_add( 'test', $data, $group, false ); // OK. wp_cache_add( 'test', $data, $group, null ); // OK. + +// Test handling of other scalar values. +wp_cache_add( 'test', $data, $group, true ); // Bad - becomes integer 1. diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php index 78e8c8d2..3331e7da 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php @@ -34,30 +34,31 @@ public function getErrorList() { */ public function getWarningList() { return [ - 27 => 1, - 28 => 1, - 29 => 1, - 30 => 1, - 32 => 1, - 33 => 1, - 34 => 1, - 35 => 1, - 37 => 1, - 38 => 1, - 39 => 1, - 40 => 1, - 47 => 1, - 52 => 1, - 56 => 1, - 74 => 1, - 75 => 1, - 76 => 1, - 77 => 1, - 78 => 1, - 79 => 1, - 88 => 1, - 94 => 1, - 95 => 1, + 27 => 1, + 28 => 1, + 29 => 1, + 30 => 1, + 32 => 1, + 33 => 1, + 34 => 1, + 35 => 1, + 37 => 1, + 38 => 1, + 39 => 1, + 40 => 1, + 47 => 1, + 52 => 1, + 56 => 1, + 74 => 1, + 75 => 1, + 76 => 1, + 77 => 1, + 78 => 1, + 79 => 1, + 88 => 1, + 94 => 1, + 95 => 1, + 105 => 1, ]; } } From 0340d86df6321a2fece7b55ee239df9cdb7e1af0 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 9 Oct 2020 00:58:50 +0200 Subject: [PATCH 12/14] LowExpiryCacheTime: bug fix - allow for WP time constants being passed as FQN In a namespaced file, it is common to use Fully Qualified Names for constructs in the global namespace. Alternatively, these are imported via an import `use` statement. This handles the first case for when a WP time constant is passed as an FQN. The second case should be handled at a later time once PHPCSUtils is being implemented. This commit also adds a number of additional unit tests to safeguard that something which may _look_ like a WP time constant, but isn't, correctly triggers the "Undetermined" warning. --- .../Sniffs/Performance/LowExpiryCacheTimeSniff.php | 9 +++++++++ .../Tests/Performance/LowExpiryCacheTimeUnitTest.inc | 11 +++++++++++ .../Tests/Performance/LowExpiryCacheTimeUnitTest.php | 5 +++++ 3 files changed, 25 insertions(+) diff --git a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php index 95ad8aff..0b9d499c 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php @@ -81,6 +81,15 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p continue; } + if ( $this->tokens[ $i ]['code'] === T_NS_SEPARATOR ) { + /* + * Ignore namespace separators. If it's part of a global WP time constant, it will be + * handled correctly. If it's used in any other context, another token *will* trigger the + * "undetermined" warning anyway. + */ + continue; + } + if ( isset( $reportPtr ) === false ) { // Set the report pointer to the first non-empty token we encounter. $reportPtr = $i; diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc index cee00932..1ffa68f3 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc @@ -103,3 +103,14 @@ wp_cache_add( 'test', $data, $group, null ); // OK. // Test handling of other scalar values. wp_cache_add( 'test', $data, $group, true ); // Bad - becomes integer 1. + +// Test passing just and only one of the time constants, including passing it as an FQN. +wp_cache_set( 'test', $data, $group, HOUR_IN_SECONDS ); // OK. +wp_cache_set( 'test', $data, $group, \MONTH_IN_SECONDS ); // OK. + +// Test passing something which may look like one of the time constants, but isn't. +wp_cache_set( 'test', $data, $group, month_in_seconds ); // Bad - constants are case-sensitive. +wp_cache_set( 'test', $data, $group, HOUR_IN_SECONDS::methodName() ); // Bad - not a constant. +wp_cache_set( 'test', $data, $group, $obj->MONTH_IN_SECONDS ); // Bad - not a constant. +wp_cache_set( 'test', $data, $group, $obj::MONTH_IN_SECONDS ); // Bad - not the WP constant. +wp_cache_set( 'test', $data, $group, PluginNamespace\SubLevel\DAY_IN_SECONDS ); // Bad - not the WP constant. diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php index 3331e7da..684390ab 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php @@ -59,6 +59,11 @@ public function getWarningList() { 94 => 1, 95 => 1, 105 => 1, + 112 => 1, + 113 => 1, + 114 => 1, + 115 => 1, + 116 => 1, ]; } } From 82ce04a6a115bfcecf380c8c0815a7de58b37340 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 9 Oct 2020 01:01:50 +0200 Subject: [PATCH 13/14] LowExpiryCacheTime: add a few more unit tests ... safeguarding that certain situations are handled correctly and documenting the sniff behaviour in those cases. --- .../Tests/Performance/LowExpiryCacheTimeUnitTest.inc | 7 +++++++ .../Tests/Performance/LowExpiryCacheTimeUnitTest.php | 3 +++ 2 files changed, 10 insertions(+) diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc index 1ffa68f3..a6572d6d 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc @@ -114,3 +114,10 @@ wp_cache_set( 'test', $data, $group, HOUR_IN_SECONDS::methodName() ); // Bad - n wp_cache_set( 'test', $data, $group, $obj->MONTH_IN_SECONDS ); // Bad - not a constant. wp_cache_set( 'test', $data, $group, $obj::MONTH_IN_SECONDS ); // Bad - not the WP constant. wp_cache_set( 'test', $data, $group, PluginNamespace\SubLevel\DAY_IN_SECONDS ); // Bad - not the WP constant. + +// Test passing negative number as cache time. +wp_cache_set( 'test', $data, $group, -300 ); // Bad. +wp_cache_add( $testing, $data, 'test_group', -6 * MINUTE_IN_SECONDS ); // Bad. + +// Test more complex logic in the parameter. +wp_cache_add( $key, $data, '', ($toggle ? 200 : 400) ); // Manual inspection warning. diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php index 684390ab..9c6e98f8 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php @@ -64,6 +64,9 @@ public function getWarningList() { 114 => 1, 115 => 1, 116 => 1, + 119 => 1, + 120 => 1, + 123 => 1, ]; } } From 32aed75f7b1538d1fdb6abcca0e312b8fdd1441a Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 9 Oct 2020 01:31:54 +0200 Subject: [PATCH 14/14] LowExpiryCacheTime: add note about the object cache implementation used --- .../Sniffs/Performance/LowExpiryCacheTimeSniff.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php index 0b9d499c..7137c698 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php @@ -13,6 +13,8 @@ /** * This sniff throws a warning when low cache times are set. * + * {@internal VIP uses the Memcached object cache implementation. {@link https://github.com/Automattic/wp-memcached}} + * * @package VIPCS\WordPressVIPMinimum * * @since 0.4.0