Automattic · rebeccahum · Oct 9, 2020 · Oct 8, 2020 · Oct 8, 2020 · Oct 8, 2020
@@ -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 );
 		}
 	}
 }
@@ -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.
@@ -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,
 		];
 	}
 }
-Original file line number
+Diff line change
@@ Expand Up / @@ -34,18 +34,39 @@ public function getErrorList() { @@
     	 */
     	public function getWarningList() {
     		return [
-=> 1,
-=> 1,
-=> 1,
-=> 1,
-=> 1,
-=> 1,
-=> 1,
-=> 1,
-=> 1,
-=> 1,
-=> 1,
-=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
+=> 1,
     		];
     	}
     }