Add sniff for deprecated Theme related WP Constants. #30
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
jrfnl
force-pushed
the
WPTRT/feature/issue-23-forbidden-constants
branch
2 times, most recently
from
61ba2fd to
20dfc70
Compare
Covers the sniff + unit test for issue #23. This PR covers the currently known list as can be found in: https://github.com/Otto42/theme-check/pull/162/files
jrfnl
force-pushed
the
WPTRT/feature/issue-23-forbidden-constants
branch
from
20dfc70 to
0f08cff
Compare
jrfnl
changed the title
|
Updated to read 'Deprecated' not 'Forbidden' |
| */ | ||
|
|
||
| /** | ||
| * WordPress_Tests_Theme_DeprecatedWPConstants |
There was a problem hiding this comment.
Missing a full stop at the end here.
There was a problem hiding this comment.
Working on improving the standard documentation in WPCS and will port back to any code in the theme review branch if/when my changes are accepted in WPCS. Ok?
72 tasks
|
@grappler |
|
@ernilambar That's correct behaviour. |
This type of code and similar should be caught by the sniff I believe. This code wont give |
|
In that case, another sniff should be created (or additional check added to this sniff) to check for If you think that TRTCS should also check for constants being defined, please open a new issue in this repo so this can be properly discussed. |
jrfnl
added a commit
to WordPress/WordPress-Coding-Standards
that referenced
this pull request
Aug 5, 2017
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: * 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 WPTT/WPThemeReview/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. Fixes 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.
jrfnl
added a commit
to WordPress/WordPress-Coding-Standards
that referenced
this pull request
Aug 5, 2017
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: * 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 WPTT/WPThemeReview/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 WPTT/WPThemeReview/issues/110 once the original sniff gets removed from the TRT fork and this sniff is added to the TRT ruleset instead.
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.
Covers the sniff + unit test for issue #23.
This PR covers the currently known list as can be found in: https://github.com/Otto42/theme-check/pull/162/files