Performance/CacheValueOverride: various improvements #867
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Preliminary commit to allow for adding additional test case files.
As things were, the determination of whether or not a `T_STRING` is a call to the global WP native `wp_cache_get()` function was severely flawed.
By switching the sniff over to be based on the WordPressCS `AbstractFunctionRestrictionsSniff` class, this flaw is mitigated.
This flaw did not lead to false positive due to the subsequent token walking being very rigid.
Includes adding a slew of additional tests to document the sniff behaviour.
Additionally, the tests have been made more comprehensive and varied by:
* Testing against false positives for calls to methods or namespaced function calls (= the issue being addressed in this PR).
* Testing against false positives for attribute class using the same name as the function.
* Ensure function import `use` statements are not flagged. We're not interested in those.
* Adding more variations to the pre-existing tests:
- Non-lowercase function call(s).
…ualified function call The `isVariableAssignment()` method walks back from the function call to see if the result is being assigned, but did not take fully qualified function calls into account. Fixed now. Includes test.
… first class callable assignment If a PHP 8.1+ first class callable is assigned to a variable, the assignment captures the callable, not the result of calling the callable. This commit adds some dedicated code to handle this better and prevent the false positive. Includes tests.
…es in different scopes As things were, the searching for the "next use of the variable to which the cache was assigned" did not limit itself to the current variable scope. In effect, that meant that "function local" variables could be mistaken for global variables and visa versa, as demonstrated by the new tests. Fixed now. Includes tests.
... to guard against the sniff throwing PHP notices/warnings due to parse errors/live coding. Any such PHP notice/warning stops the PHPCS run for a file, so should be avoided. Includes tests.
jrfnl
added
Type: Bug
Type: Maintenance
Type: False positive
PHPCSUtils
51 tasks
GaryJones
approved these changes
Jul 30, 2025
GaryJones
deleted the
feature/performance-cachevalueoverride-initial-improvements
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Performance/CacheValueOverride: rename test case file
Preliminary commit to allow for adding additional test case files.
Performance/CacheValueOverride: extend AbstractFunctionRestrictionsSniff
As things were, the determination of whether or not a
T_STRINGis a call to the global WP nativewp_cache_get()function was severely flawed.By switching the sniff over to be based on the WordPressCS
AbstractFunctionRestrictionsSniffclass, this flaw is mitigated.This flaw did not lead to false positive due to the subsequent token walking being very rigid.
Includes adding a slew of additional tests to document the sniff behaviour.
Additionally, the tests have been made more comprehensive and varied by:
usestatements are not flagged. We're not interested in those.Performance/CacheValueOverride: bug fix - false negative with fully qualified function call
The
isVariableAssignment()method walks back from the function call to see if the result is being assigned, but did not take fully qualified function calls into account.Fixed now.
Includes test.
Performance/CacheValueOverride: bug fix - false positive for PHP 8.1+ first class callable assignment
If a PHP 8.1+ first class callable is assigned to a variable, the assignment captures the callable, not the result of calling the callable.
This commit adds some dedicated code to handle this better and prevent the false positive.
Includes tests.
Performance/CacheValueOverride: bug fix - false positives for variables in different scopes
As things were, the searching for the "next use of the variable to which the cache was assigned" did not limit itself to the current variable scope.
In effect, that meant that "function local" variables could be mistaken for global variables and visa versa, as demonstrated by the new tests.
Fixed now.
Includes tests.
Performance/CacheValueOverride: add some more defensive coding
... to guard against the sniff throwing PHP notices/warnings due to parse errors/live coding.
Any such PHP notice/warning stops the PHPCS run for a file, so should be avoided.
Includes tests.
Related to #530