New WordPress.WP.DiscouragedConstants sniff
#1089
Merged
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
This sniff is based on a sniff previously pulled to the TRT fork of WPCS as `Theme.DeprecatedConstants`.
Related issue: WPTRT/WordPress-Coding-Standards/issues/ 23
Original PR: WPTRT/WordPress-Coding-Standards/pull/ 30
Covers the list of constants found in: Otto42/theme-check/pull/ 162
Differences between that sniff and this one:
* This sniff is a **_lot_** more comprehensive in preventing false positives.
The original sniff was one of the first ones I wrote and it's kind of sweet to look back at that code which I wrote over a year ago and see how much I've learned about writing sniffs in the mean time.
* The original sniff was called `Theme.DeprecatedConstants`, but in reality, most of these constants aren't officially deprecated, though their usage is discouraged and the constants being addressed are not 100% related to themes either. With that in mind, I've renamed the sniff to `WP.DiscouragedConstants` and changed the error level from Error to Warning.
Also see: https://core.trac.wordpress.org/ticket/18298
* This version of the sniff also addresses the concerns raised in WPTRT/WordPress-Coding-Standards/issues/ 110 about themes `define`-ing any of these constants and leverages the `AbstractFunctionParameterSniff` to do so.
Includes quite extensive unit tests. More are definitely welcome, especially to prevent false positives.
I've added the sniff to the `WordPress-Extra` ruleset.
Based on https://vip.wordpress.com/documentation/code-review-what-we-look-for/#using-theme-constants and https://core.trac.wordpress.org/ticket/18298#comment:2, I've also added the sniff to the `WordPress-VIP` ruleset. /cc @sboisvert @david-binda @ntwb
Fixes 97
Will also fix WPTRT/WordPress-Coding-Standards/issues/ 110 once the original sniff gets removed from the TRT fork and this sniff is added to the TRT ruleset instead.
jrfnl
changed the title
WordPress.WP.DiscouragedConstants sniffWordPress.WP.DiscouragedConstants sniff
GaryJones
approved these changes
Aug 5, 2017
JDGrimes
suggested changes
Aug 5, 2017
| class DEF implements STYLESHEETPATH {} | ||
| interface STYLESHEETPATH {} | ||
| trait STYLESHEETPATH {} | ||
| const STYLESHEETPATH = 'something'; |
There was a problem hiding this comment.
Why is this OK, if calling define() is not OK? I'd think that if we sniff for define(), we ought to also sniff for const outside of a class or namespace.
There was a problem hiding this comment.
Good question. I added a lot of code to prevent false positives and only addressed the define() bit after that. Clearly forgot to also change things for const.
I'm going to change this.
…mespace Distinguished between `const` defines in the global namespace and class constants. To this end, two more utility functions I previously write for PHPCompatibility have been ported over.
JDGrimes
approved these changes
Aug 5, 2017
GaryJones
deleted the
feature/issue-97-new-wp-discouraged-constants-sniff
branch
This was referenced Aug 5, 2017
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 sniff is based on a sniff previously pulled to the TRT fork of WPCS as
Theme.DeprecatedConstants.Related issue: WPTT/WPThemeReview/issues/23
Original PR: WPTT/WPThemeReview/pull/30
Covers the list of constants found in: WordPress/theme-check/pull/162
Differences between that sniff and this one:
The original sniff was one of the first ones I wrote and it's kind of sweet to look back at that code which I wrote over a year ago and see how much I've learned about writing sniffs in the mean time.
Theme.DeprecatedConstants, but in reality, most of these constants aren't officially deprecated, though their usage is discouraged and the constants being addressed are not 100% related to themes either. With that in mind, I've renamed the sniff toWP.DiscouragedConstantsand changed the error level from Error to Warning.Also see: https://core.trac.wordpress.org/ticket/18298
define-ing any of these constants and leverages theAbstractFunctionParameterSniffto do so.Includes quite extensive unit tests. More are definitely welcome, especially to prevent false positives.
I've added the sniff to the
WordPress-Extraruleset.Based on https://vip.wordpress.com/documentation/code-review-what-we-look-for/#using-theme-constants and https://core.trac.wordpress.org/ticket/18298#comment:2, I've also added the sniff to the
WordPress-VIPruleset. /cc @sboisvert @david-binda @ntwbFixes #97
Will also fix WPTT/WPThemeReview/issues/110 once the original sniff gets removed from the TRT fork and this sniff is added to the TRT ruleset instead.
Update: This PR also adds two new utility methods to the
WordPress\Sniffclass:is_class_constant()andvalid_direct_scope().