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.

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

Improve ParameterShadowSuperGlobals sniff (code review). #180

Merged

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Aug 17, 2016

It seems this sniff was missed both when implementing the PHPCompatibility_Sniff base class as well as when the unit tests were added.
This PR fixes that.

Changes:

  • Let class extend PHPCompatibility_Sniff.
  • Use the supportsAbove() function for the compatibility check.
  • Defer to the PHPCS native getMethodParameters() method for getting the parameter names.
  • Do a case-insensitive parameter name compare. While variable names are case-sensitive in PHP, it would still be a really bad idea to bypass the "parameter shadowing super globals" issue by using a lowercase version of the variable name.
  • Add unit tests.

It seems this sniff was missed both when implementing the `PHPCompatibility_Sniff` base class as well as when the unit tests were added.
This PR fixes that.

Changes:
* Let class extend `PHPCompatibility_Sniff`.
* Use the `supportsAbove()` function for the compatibility check.
* Defer to the PHPCS native `getMethodParameters()` method for getting the parameter names.
* Do a case-insensitive parameter name compare. While variable names are case-insensitive in PHP, it would still be a really bad idea to bypass the "parameter shadowing super globals" issue by using a lowercase version of the variable name.
* Add unit tests.
@wimg wimg merged commit 8e10da9 into PHPCompatibility:master Aug 17, 2016
@jrfnl jrfnl deleted the feature/improve-shadow-superglobals branch August 18, 2016 03:53
@jrfnl jrfnl added this to the 7.0.3 milestone Apr 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants