From dac90e5ead98bf9b0b8a38f366a9eacea36326ea Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 31 Mar 2021 03:49:04 +0200 Subject: [PATCH] LowExpiryCacheTime: improve handling of potential parse errors in evaled code Issue reported by GaryJones in Automattic/VIP-Coding-Standards 628#issuecomment-790653774 In rare cases, it is possible that the call to `eval()` with "safe" tokens only, would still result in a parse error. An example of such a case is when the PHP 5.6 `**` (`T_POW`) operator is used. PHPCS backfills the tokenization, which means that the operator is correctly recognized by the sniff as a "safe" token to use in the `eval()` statement. However, when the sniff is being run on PHP 5.4/5.5, the `eval()` will also be run on PHP 5.4/5.5 and while PHPCS backfills the token, PHP does not, resulting in a parse error in the `eval`-ed code, upon which the `eval()` will return `false`. This commit: * Silences the parse error notice. * Checks the output of the `eval()` for it being boolean `false` and if so, flags the statement for manual inspection, same as when any "non-safe" token is encountered. --- .../Performance/LowExpiryCacheTimeSniff.php | 20 +++++++++++++++---- .../LowExpiryCacheTimeUnitTest.php | 1 + 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php index 7137c698..23639560 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php @@ -77,6 +77,10 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $reportPtr = null; $openParens = 0; + $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"'; + $error_code = 'CacheTimeUndetermined'; + $data = [ $matched_content, $parameters[4]['raw'] ]; + for ( $i = $param['start']; $i <= $param['end']; $i++ ) { if ( isset( Tokens::$emptyTokens[ $this->tokens[ $i ]['code'] ] ) === true ) { $tokensAsString .= ' '; @@ -151,9 +155,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p } // 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 ); + $this->phpcsFile->addWarning( $message, $reportPtr, $error_code, $data ); return; } @@ -177,7 +179,17 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p } } - $time = eval( "return $tokensAsString;" ); // phpcs:ignore Squiz.PHP.Eval -- No harm here. + $time = @eval( "return $tokensAsString;" ); // phpcs:ignore Squiz.PHP.Eval,WordPress.PHP.NoSilencedErrors -- No harm here. + + if ( $time === false ) { + /* + * The eval resulted in a parse error. This will only happen for backfilled + * arithmetic operator tokens, like T_POW, on PHP versions in which the token + * did not exist. In that case, flag for manual inspection. + */ + $this->phpcsFile->addWarning( $message, $reportPtr, $error_code, $data ); + return; + } if ( $time < 300 && (int) $time !== 0 ) { $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.php b/WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php index 9c6e98f8..f3f6928d 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, + 82 => ( PHP_VERSION_ID > 50600 ) ? 0 : 1, 88 => 1, 94 => 1, 95 => 1,