LowExpiryCacheTime: add extra warning + lots of bug fixes #587

jrfnl · 2020-10-08T23:20:25Z

TL;DR: This implements an initial set of improvements for the LowExpiryCacheTime sniff, related to #532, and addresses issue #572.

Commit Details

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"

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.

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 evaluated,
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 note 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.

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 evaluated 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.

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.

LowExpiryCacheTime: add new warning / manual inspection needed

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.

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.

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 evaluated and only throw a warning if the resulting cache time is lower than 300.

Includes unit tests.

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:

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:

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.

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.

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.

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.

LowExpiryCacheTime: add a few more unit tests

... safeguarding that certain situations are handled correctly and documenting the sniff behaviour in those cases.

LowExpiryCacheTime: add note about the object cache implementation used

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"

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.

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.

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.

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.

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.

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.

…ions 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.

…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.

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.

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.

…d 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.

... safeguarding that certain situations are handled correctly and documenting the sniff behaviour in those cases.

GaryJones · 2020-10-09T00:53:42Z

I ❤️ it!

I think the only question would be on not triggering a violation when the time is 0.

What use case is there to add to an in-memory or persistent object cache, but have a TTL of 0, such that it wouldn't be able to be retrieved?

Just because the function parameter has got a default of 0, doesn't mean that would necessarily be a good / acceptable value for use on VIP.

jrfnl · 2020-10-09T01:14:46Z

I think the only question would be on not triggering a violation when the time is 0.

What use case is there to add to an in-memory or persistent object cache, but have a TTL of 0, such that it wouldn't be able to be retrieved?

Just because the function parameter has got a default of 0, doesn't mean that would necessarily be a good / acceptable value for use on VIP.

As per the info in the commit message:

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, the original intention in the WP implementation is that the cache would never expire.
In VIP object cache, it will revert to the default expiration period (=0).

Note: in the VIP implementation, it will also revert to the default expiration period when $expire is set to more than the maximum (30 days).

I'd suspect/hope that the default_expiration is overloaded with an actual value on the production servers, but that should be checked.

All the same, passing 0 is effectively the same as not passing the value and the sniff explicitly bows out when the parameter is not set, so either it should throw a warning when 0 or not set or it shouldn't for both cases. As it was, it was inconsistent.

https://github.com/Automattic/wp-memcached/blob/811243804a892a4609a4581d9479fa80c4e4ac8d/object-cache.php#L133-L134

https://github.com/Automattic/wp-memcached/blob/811243804a892a4609a4581d9479fa80c4e4ac8d/object-cache.php#L164-L167

GaryJones · 2020-10-10T02:49:30Z

Sorry, you're right - 0 means cache for infinity - which is a good thing.

My brain wasn't taking that fact in.

jrfnl · 2020-10-10T03:00:21Z

Happens to the best of us ;-)

jrfnl added 13 commits October 9, 2020 01:06

LowExpiryCacheTime: minor efficiency tweak

c67a36a

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.

LowExpiryCacheTime: add a few more unit tests

82ce04a

... safeguarding that certain situations are handled correctly and documenting the sniff behaviour in those cases.

jrfnl added Type: Bug Type: Enhancement Standard: VIPMinimum labels Oct 8, 2020

jrfnl added this to the 2.3.0 milestone Oct 8, 2020

jrfnl requested a review from a team as a code owner October 8, 2020 23:20

LowExpiryCacheTime: add note about the object cache implementation used

32aed75

rebeccahum approved these changes Oct 9, 2020

View reviewed changes

rebeccahum merged commit d374355 into develop Oct 9, 2020

rebeccahum deleted the fix/572-lowexpirytime-flag-4th-param-when-undetermined branch October 9, 2020 18:57

jrfnl mentioned this pull request Apr 16, 2021

Update changelog to reflect 2.3.0 #662

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LowExpiryCacheTime: add extra warning + lots of bug fixes #587

LowExpiryCacheTime: add extra warning + lots of bug fixes #587

jrfnl commented Oct 8, 2020 •

edited

Loading

GaryJones commented Oct 9, 2020

jrfnl commented Oct 9, 2020

GaryJones commented Oct 10, 2020

jrfnl commented Oct 10, 2020

LowExpiryCacheTime: add extra warning + lots of bug fixes #587

LowExpiryCacheTime: add extra warning + lots of bug fixes #587

Conversation

jrfnl commented Oct 8, 2020 • edited Loading

Commit Details

LowExpiryCacheTime: improve error message

LowExpiryCacheTime: improve error line precision

LowExpiryCacheTime: bug fix - calculations with simple floats

LowExpiryCacheTime: bug fix - allow for comments

LowExpiryCacheTime: minor efficiency tweak

LowExpiryCacheTime: add new warning / manual inspection needed

LowExpiryCacheTime: bug fix - allow for all arithmetic operators

LowExpiryCacheTime: bug fix - allow for parentheses grouping calculations

LowExpiryCacheTime: bug fix - allow for cache time being passed as a string

LowExpiryCacheTime: bug fix - allow for a cache time of 0

LowExpiryCacheTime: feature completeness - allow for true

LowExpiryCacheTime: bug fix - allow for WP time constants being passed as FQN

LowExpiryCacheTime: add a few more unit tests

LowExpiryCacheTime: add note about the object cache implementation used

GaryJones commented Oct 9, 2020

jrfnl commented Oct 9, 2020

GaryJones commented Oct 10, 2020

jrfnl commented Oct 10, 2020

jrfnl commented Oct 8, 2020 •

edited

Loading

LowExpiryCacheTime: feature completeness - allow for `true`