Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LowExpiryCacheTimeSniff: Account for random generating number func #445

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

LowExpiryCacheTimeSniff: Account for random generating number func #445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 36 additions & 11 deletions WordPressVIPMinimum/Sniffs/Performance/LowExpiryCacheTimeSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,18 @@ class LowExpiryCacheTimeSniff extends AbstractFunctionParameterSniff {
'YEAR_IN_SECONDS' => 31536000,
];

/**
* List of random generating number functions.
*
* @var array
*/
protected $rand_functions = [
'wp_rand',
'random_int',
'mt_rand',
'rand',
];

/**
* Process the parameters of a matched function.
*
Expand All @@ -69,21 +81,34 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p
return;
}

$time = $parameters[4]['raw'];

if ( false === is_numeric( $time ) ) {
// 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 using time constants, we need to convert to a number.
$time = str_replace( array_keys( $this->wp_time_constants ), $this->wp_time_constants, $parameters[4]['raw'] );

if ( preg_match( '#^[\s\d+*/-]+$#', $time ) > 0 ) {
$time = eval( "return $time;" ); // phpcs:ignore Squiz.PHP.Eval -- No harm here.
$rand_function = false;
foreach ( $this->rand_functions as $fn ) {
if ( false !== strpos( $time, $fn ) ) {
$rand_function = $fn;
break;
}
}

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 );
$times = [];
if ( false !== $rand_function ) {
$times = explode( ',', preg_replace( '/[( )|\(|\)|(' . $rand_function . ')]/', '', $time ) );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this regex is going to miss cases where there are unusual code styling - line breaks before, between, or after string arguments like 20 * HOUR_IN_SECONDS - I'd at least like to see more unit tests, and I think when we switch to be able to use https://phpcsutils.com/phpdoc/classes/PHPCSUtils-Utils-PassedParameters.html#method_getParameters then this will be much easier to be confident with.

} else {
$times[] = $time;
}

foreach ( $times as $time ) {
if ( preg_match( '#^[\s\d+*\/-]+$#', $time ) > 0 ) {
$time = eval( "return $time;" ); // phpcs:ignore Squiz.PHP.Eval -- No harm here.
}
if ( $time < 300 || is_null( $time ) ) {
$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 );
return;
}
}
}
}
10 changes: 10 additions & 0 deletions WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ wp_cache_replace( $testing, $data, 'test_group', 8*MINUTE_IN_SECONDS );
wp_cache_replace( 1234, $data, '', 425 );
wp_cache_replace( $testing, $data, null, 350 );

wp_cache_set( 'test', $data, '', wp_rand( 5*MINUTE_IN_SECONDS, 10*MINUTE_IN_SECONDS ) );
wp_cache_add( 'test', $data, '',rand(400, 20*MINUTE_IN_SECONDS ));
wp_cache_replace( 'test', $data, null, mt_rand( 500, 200*HOUR_IN_SECONDS) );
wp_cache_set( 'test', $data,'', random_int( 200 * HOUR_IN_SECONDS , 350 ) );

// Bad.
wp_cache_set( 'test', $data, $group, 100 ); // Lower than 300.
wp_cache_set( 'test', $data, $group, 2*MINUTE_IN_SECONDS ); // Lower than 300.
Expand All @@ -38,3 +43,8 @@ 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.

wp_cache_set( 'test', $data, '', wp_rand( 5*MINUTE_IN_SECONDS, 1*MINUTE_IN_SECONDS ) ); // Lower than 300.
wp_cache_add( 'test', $data, '',rand(null, 20*MINUTE_IN_SECONDS )); // Lower than 300.
wp_cache_replace( 'test', $data, null, mt_rand( 200 , 200*HOUR_IN_SECONDS) ); // Lower than 300.
wp_cache_set( 'test', $data,'', random_int( 200 * HOUR_IN_SECONDS ,299 ) ); // Lower than 300.
12 changes: 8 additions & 4 deletions WordPressVIPMinimum/Tests/Performance/LowExpiryCacheTimeUnitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,6 @@ public function getErrorList() {
*/
public function getWarningList() {
return [
27 => 1,
28 => 1,
29 => 1,
30 => 1,
32 => 1,
33 => 1,
34 => 1,
Expand All @@ -44,6 +40,14 @@ public function getWarningList() {
38 => 1,
39 => 1,
40 => 1,
42 => 1,
43 => 1,
44 => 1,
45 => 1,
47 => 1,
48 => 1,
49 => 1,
50 => 1,
];
}
}