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

[New sniff] Check for disallowed theme related WP constants #23

Closed
4 of 6 tasks
grappler opened this issue Jul 12, 2016 · 9 comments
Closed
4 of 6 tasks

[New sniff] Check for disallowed theme related WP constants #23

grappler opened this issue Jul 12, 2016 · 9 comments

Comments

@grappler
Copy link
Member

grappler commented Jul 12, 2016

Rule:

ERROR : Check for usage of deprecated WP constants relating to themes and discouraged PHP constants e.g __FILE__. We could probably extend an existing sniff of similar ilk for this.

Ref: https://make.wordpress.org/themes/handbook/review/required/#core-functionality-and-features

Theme check file covering this rule:

https://github.com/Otto42/theme-check/pull/162/files

Decision needed:

Confirmation is needed that the current list is correct. If not, input is needed about which other theme related WP constants should (not) be covered by this sniff.

To do:

  • Review the current list of constants being checked for and confirm that this covers the theme related WP constants. If not, please leave a comment in this issue thread with details of the changes needed.
  • Expand and clarify the rule in the Handbook.
  • Create unit tests
  • Create new sniff
  • Open new item for related but distinct PHP constants sniff
  • If needed: Open new item for related but distinct other constants sniff
@emiluzelac
Copy link

Here's my snippet:

Theme needs to properly load files, but also to be consistent as well.

There are several functions available for getting the path:

  • get_template_directory - Filter the current theme directory path.
  • get_template_directory_uri - Filter the current theme directory URI.
  • get_stylesheet_directory - Filter the stylesheet directory path for current theme.
  • get_stylesheet_directory_uri - Filter the stylesheet directory URI.

http://justintadlock.com/archives/2010/11/17/how-to-load-files-within-wordpress-themes

@jrfnl
Copy link
Contributor

jrfnl commented Jul 12, 2016

Is the current list as can be found in https://github.com/Otto42/theme-check/pull/162/files still complete enough as a starting point ?

@jrfnl
Copy link
Contributor

jrfnl commented Jul 12, 2016

Also: looking at the PR - we may want to split this into two three sniffs:

  • one for path related constants
  • one for theme support related constants
  • one for generally deprecated WP constants

I think that would make it easier to maintain and extend.

[Edit] Possibly even more groups should be added. I'd prefer one issue for each sniff to be created, so would like to suggest splitting this sniff in three or more issues.

@jrfnl
Copy link
Contributor

jrfnl commented Jul 12, 2016

Also looking at the rule in the handbook, I'd like to add an action item to expand and clarify the rule as it currently isn't explicit enough and doesn't cover all that is covered by the Theme Check PR IMHO:

Use get_template_directory() rather than TEMPLATEPATH to return the template path.
Use get_stylesheet_directory() rather than STYLESHEETPATH to return the stylesheet path.

@justintadlock
Copy link

justintadlock commented Jul 15, 2016

I'd say no to "discouraged" (not sure who decided this was discouraged) PHP constants like __FILE__. That can be quite useful.

@jrfnl
Copy link
Contributor

jrfnl commented Jul 15, 2016

@justintadlock I guess this was the inspiration: #18

@jrfnl jrfnl changed the title [New sniff] Check for disallowed constants [New sniff] Check for disallowed theme related WP constants Jul 18, 2016
@jrfnl jrfnl self-assigned this Jul 18, 2016
@grappler
Copy link
Member Author

The reason to not support the constants is that they have been deprecated. I think the list is good and we can always extend it if necessary. I am looking to find a way we can best document these requirements.

@jrfnl
Copy link
Contributor

jrfnl commented Jul 19, 2016

The reason to not support the constants is that they have been deprecated

In that case - let me adjust the PR and name the sniff DeprecatedWPConstants instead of Forbidden.

[Edit] Done.

jrfnl added a commit that referenced this issue Jul 19, 2016
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
grappler pushed a commit that referenced this issue Jul 28, 2016
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
@grappler
Copy link
Member Author

grappler pushed a commit that referenced this issue Sep 27, 2016
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 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants