GlobalVariablesOverride: detect variable overrides in global namespace #1457
Conversation
This file contains 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
... and allow for multiple test case files in the test provider file.
* Register class/trait tokens. * Bow out earlier if a class/trait token turns out to translate to a test class. * Skip over the rest of that class.
…ss_token() method
If the token being examined is a `T_VARIABLE` token and has a whitelist comment, the sniff can bow out before examining the token in detail. Includes splitting the `maybe_add_error()` method up to prevent checking twice for a whitelist comment. For the `T_GLOBAL` token this is not possible as the whitelist comment will not be on the line with the `$stackPtr` pointing to the `global` keyword.
… scope * Minor simplification of how the `$search` array is gathered. * Search only within the function scope. * Skip over nested functions/closures/classes. * Minor code changes to lower the nesting levels.
…scope Examine variables found in the global scope and always examine the `$GLOBALS` variable independently of scope. * Minor efficiency fix when checking the array key of a `$GLOBALS` variable - no need to walk the token array twice. * Error prevention for when the `$GLOBALS` array key would not contain a `T_CONSTANT_ENCAPSED_STRING` token. * Don't throw errors for default values for function parameters in function declarations. * Don't throw errors for class properties.
By setting the `treat_files_as_scoped` property to `true`, the sniff will regard each file as being included from with a function, making it scoped to the function holding the `include` statement. This basically allows for the sniff to revert back to the original behaviour of only examining variables in the global scope if a `global` statement had been found or if the variable used was `$GLOBALS`.
… to $GLOBALS is found Previously, the error message would just show the rather non-descript `$GLOBALS`, now it will show `$GLOBALS['array_key']`.
|
Anyone have an opinion on this ? |
GaryJones
approved these changes
Aug 25, 2018
|
@WordPress-Coding-Standards/wpcs-admins Anything I can do to move this PR forward ? It is a significant change, so a second review (and if deemed acceptable: approval), would be appreciated. |
JDGrimes
approved these changes
Sep 7, 2018
JDGrimes
deleted the
feature/1395-detect-variable-overrides-in-global-namespace
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.
This PR changes the
GlobalVariablesOverridesniff to always check for variable overrides in the global namespace, independently of there being aglobalstatement or not.While this may throw false positives for theme template files/ plugin view files which are
include-ed from within a function and therefore scoped to that function, there is no guarantee that the file will always be included from within an function in another file, so using the same variable name as used by WP itself, would still be bad practice.All the same, I've added a public
treat_files_as_scopedproperty which when set totruefrom within a custom ruleset, basically reverts the sniff back to the old behaviour.The default is
falseand setting this property totrueshould be discouraged.If - in the future - PHPCS would allow limiting the effect of
// phpcs:setannotations to the file in which the annotation is used, that annotation could be used in the file header for view/template files to revert to the old behaviour just and only for those particular files. See upstream feature request squizlabs/PHP_CodeSniffer#2126 for more information.Additional notes/fixes:
is_foreach_as()method from thePrefixAllGlobalsclass to theSniffbase class so it can also be used by this sniff.T_VARIABLEtokens.globalstatement, nested funcions/closures/classes should always be skipped. Previously, this was only done when theglobalstatement was found in the global namespace.$GLOBALSvariable.Previously, the error message would just show the rather non-descript
$GLOBALS, now it will show$GLOBALS['array_key'].See the individual commits for more detailed information.
Fixes #1395
To do once the PR is merged: