diff --git a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php index 34ea78e6..7137c698 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php @@ -7,11 +7,14 @@ namespace WordPressVIPMinimum\Sniffs\Performance; +use PHP_CodeSniffer\Util\Tokens; use WordPressCS\WordPress\AbstractFunctionParameterSniff; /** * 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 @@ -69,21 +72,123 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p return; } - $time = $parameters[4]['raw']; + $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 ) { + $tokensAsString .= ' '; + 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; + } + + if ( $this->tokens[ $i ]['code'] === T_LNUMBER + || $this->tokens[ $i ]['code'] === T_DNUMBER + ) { + // Integer or float. + $tokensAsString .= $this->tokens[ $i ]['content']; + continue; + } + + if ( $this->tokens[ $i ]['code'] === T_FALSE + || $this->tokens[ $i ]['code'] === T_NULL + ) { + $tokensAsString .= 0; + 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; + } - 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 ); + 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 ( $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; + } - 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_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'] ]; + $this->phpcsFile->addWarning( $message, $reportPtr, 'CacheTimeUndetermined', $data ); + + return; + } + + if ( $tokensAsString === '' ) { + // Nothing found to evaluate. + return; } - if ( $time < 300 ) { - $message = 'Low cache expiry time of "%s", it is recommended to have 300 seconds or more.'; - $data = [ $parameters[4]['raw'] ]; - $this->phpcsFile->addWarning( $message, $stackPtr, 'LowCacheTime', $data ); + $tokensAsString = trim( $tokensAsString ); + + 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 && (int) $time !== 0 ) { + $message = 'Low cache expiry time of %s seconds detected. It is recommended to have 300 seconds or more.'; + $data = [ $time ]; + + if ( (string) $time !== $tokensAsString ) { + $message .= ' Found: "%s"'; + $data[] = $tokensAsString; + } + + $this->phpcsFile->addWarning( $message, $reportPtr, 'LowCacheTime', $data ); } } } diff --git a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc index eb5a63cd..a6572d6d 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc @@ -38,3 +38,86 @@ 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. +); + +// 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. + +// 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. + +// 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. + +// 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. + +// 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. + +// 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. + +// 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 81a4da60..9c6e98f8 100644 --- a/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php +++ b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php @@ -34,18 +34,39 @@ 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, + 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, + 112 => 1, + 113 => 1, + 114 => 1, + 115 => 1, + 116 => 1, + 119 => 1, + 120 => 1, + 123 => 1, ]; } }