PrefixAllGlobals/GlobalVariablesOverride: detect variables being set via list() #1783
Conversation
Sister-method to the `find_array_open_close()` utility method to find the opener and closer for `list()` constructs, including short lists.
The bracket opener array key will only be set if there is also a bracket closer.
This adds a new utility method which will retrieve an array with the token pointers to the variables which are being assigned to in a `list()` construct, whether short or long list. This utility method takes all currently supported list features in PHP into account and handles the following correctly: * Nested lists * Empty list items * Trailing comma's in lists * Empty lists (no longer allowed as of PHP 7.0.0) * Short lists (PHP 7.1.0+) * Keyed lists (PHP 7.1.0+)
Global variables to which an assignment is made via the long/short `list()` construct should also be prefixed. Includes unit tests. Related to 1774
…ignments Global variables to which an assignment is made via the long/short `list()` construct should also be checked to make sure they don't override a WP global variable. Includes unit tests. Related to 1774
jrfnl
force-pushed
the
feature/1774-globalvarsoverride-prefixallglobals-recognize-list-assignments
branch
from
e711126
to
f47cf7b
Compare
| * @return int|void Integer stack pointer to skip forward or void to continue | ||
| * normal file processing. | ||
| */ | ||
| protected function process_list_assignment( $stackPtr ) { |
There was a problem hiding this comment.
Should this be added as a util method in the Sniff.php like get_list_variables is? Because it's used in two places.
There was a problem hiding this comment.
Fair question. IMO no.
process_...() methods are not utility methods, but sniff processing specific. IMO they can either be in the sniff itself or in an abstract sniff - or possibly a sniff helper trait.
As these two sniffs are basically the polar opposite of each other, a case can be made to refactor them and either work with an abstract class, a trait or let one sniff extend the other.
(and yes, I've been thinking about that myself as well)
That was not what this PR is about though. This PR adds certain missing functionality. To keep things clean and reviewable, the refactor should - again IMO - be done in a separate PR.
There was a problem hiding this comment.
Thanks for the explanation, this makes more sense now. And I agree, implementing either a trait or an abstract class seems like overkill for now.
👍
jrfnl
deleted the
feature/1774-globalvarsoverride-prefixallglobals-recognize-list-assignments
branch
… 2.2.0 WPCS 2.2.0 changes the method signature of the `PrefixAllGlobalsSniff::process_variable_assignment()` method, adding a new `$in_list` parameter. This means that the method signature for the method as overloaded in WPTRTCS needs to be updated to prevent the following PHP warning `Declaration of WPThemeReview\Sniffs\CoreFunctionality\PrefixAllGlobalsSniff::process_variable_assignment($stackPtr) should be compatible with WordPressCS\WordPress\Sniffs\NamingConventions\PrefixAllGlobalsSniff::process_variable_assignment($stackPtr, $in_list = false)`. It it safe to add this new parameter to WPThemeReview already, even though WPCS 2.2.0 hasn't been released yet, as passing _too many_ parameters to a function and/or adding an additional parameter to an overloaded method in a child class does not cause issues in PHP, while having _too few_ parameters will as the above warning demonstrates. Refs: * WordPress/WordPress-Coding-Standards#1783
Global variables to which an assignment is made via the long/short
list()construct should also be prefixed (PrefixAllGlobals), unless they are WordPress native global variables, in which case theGlobalVariablesOverridesniff should kick in and throw an error about overwriting WP globals.Includes unit tests for both sniffs.
Fixes #1774
Loosely related to #764 (short lists, keyed lists)
This PR includes adding two new utility methods to the
Sniffclass:Sniff::find_list_open_close()- Sister-method to thefind_array_open_close()utility method to find the opener and closer forlist()constructs, including short lists.Sniff::get_list_variables()- A utility method which will retrieve an array with the token pointers to the variables which are being assigned to in alist()construct, whether short or long list.This utility method takes all currently supported list features in PHP into account and handles the following correctly: