Restrict defining deprecated define variables #110
Labels
Comments
|
I think it makes sense. This code would fail in the current Theme Check too. https://github.com/WordPress/theme-check/blob/master/checks/constants.php |
jrfnl
added a commit
to WordPress/WordPress-Coding-Standards
that referenced
this issue
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 issue
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.
This was referenced Aug 5, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Separated from #30 (comment)
This type of code and similar should be caught by the sniff I believe. This code wont give
Constant already definederror, since this was the recommended method used to override Custom Header before 3.4.The text was updated successfully, but these errors were encountered: