Feature/prefix globals warning #208

dingo-d · 2019-04-01T09:40:02Z

Since we've released the Theme Sniffer plugin, more and more authors and reviewers are complaining about the prefix all globals sniff.

This PR downgrades it to a warning instead of an error.

dingo-d · 2019-04-01T09:50:36Z

I just saw your comment @jrfnl in #189.

I think that rewording of the error message could help, but we would still get a lot of false positives.

jrfnl

This is not a good idea. Let me reiterate what I've said before:

I would strongly recommend against that. If anything, you may just want to downgrade the specific message about non-prefixed variables which is causing all the strive, definitely not the whole sniff.

Also note: The sniff won't be active unless a prefix is provided.

jrfnl · 2019-04-01T09:58:55Z

I think that rewording of the error message could help, but we would still get a lot of false positives.

Correct me if I'm wrong, but from what I've seen from the discussion, you're not getting false positives, but you want to allow non-prefixed variables in a certain context.

You should still want to forbid non-prefixed classes, global functions etc.

If that's the case, there are two options:

Either downgrade a specific errorcode to a warning, not the whole sniff.
Or (better) extend the sniff and overload the relevant method to bow out early in case of theme template files, but still trigger an error in all other cases.

The fact that people are getting annoyed with the message, is not a fault of the sniff, but a case of educating people to write better code.

dingo-d · 2019-04-01T10:15:28Z

Yeah, the issue (from what I've seen) is that non prefixed variables in the template files trigger this sniff. And since there is no required way of loading the templates (like having them all in a specific folder in a theme called templates), it's not possible for the sniffer to know what is a template file and what isn't.

I'll try to get a hold of a few themes and see where the faulty code is, and then try to implement your suggestions. I'll work in this PR so it can stay open.

Thanks for the suggestions and advice 👍

jrfnl · 2019-04-01T10:56:12Z

it's not possible for the sniffer to know what is a template file and what isn't

I had a little think about that and I think we can probably detect whether something is a template file with a reasonably high degree of certainty (providing the customized sniff is only run on themes).

I'm thinking along the following lines:

File isn't called functions.php.
File contains inline HTML.
File does not contain declarations for classes, functions and such.

Does that help ?

DannyCooper · 2019-04-01T11:28:41Z

I might be underthinking this, but wouldn't it be possible to have a list of 'template-files' that would be right 90%+ of the time?

/template-parts/*
/templates/*
/partials/*
template-*
taxonomy-*
category-*
tag-*
page-*
archive-*
single-*
sidebar-*
front-page.ph
header.php
footer.php
index.php
comments.php
search.php
searchform.php
404.php

Sure there would be edge cases, but if it eliminated 90% of the 'false-positives', would it be worth it?

joyously · 2019-04-01T14:45:34Z

I don't think a whitelist is a good idea. I agree with detecting that it's a template. In fact, my theme would not pass because my template files are output as widgets, so would not match the whitelist at all.
And the other half of the problem is that those template files still should not be modifying WordPress globals, so they can't just be skipped. (or is that two different sniffs?)

dingo-d · 2019-04-01T15:00:45Z

I think that the globals sniff would just have specific error codes downgraded to warnings, but a separate sniff would be used for the templates and checking if globals are present there.

jrfnl · 2019-04-01T16:51:34Z

wouldn't it be possible to have a list of 'template-files' that would be right 90%+ of the time?

I don't think a whitelist is a good idea. I agree with detecting that it's a template. In fact, my theme would not pass because my template files are output as widgets, so would not match the whitelist at all.

@DannyCooper @joyously Both approaches could be combined, a file whitelist first, for files not on that list: check the contents of the file.
If I remember correctly, the WPCS Filename sniff already has a template file name regex which could possibly be used or if not used directly, used as inspiration.

And the other half of the problem is that those template files still should not be modifying WordPress globals, so they can't just be skipped. (or is that two different sniffs?)

Those are two different sniffs, so that shouldn't be a problem and the above would only be applied to the one sniff, the files would not be skipped for any other sniffs.

dingo-d · 2019-04-13T15:02:10Z

@jrfnl I'd like to work on this so that I can brush up on writing the sniffs, but I'd need some help 🙂

You've mentioned downgrading certain errorcode to a warning. That would be done in phpcs.xml.dist or?

About sniff extension, I've seen WordPress.WP.GlobalVariablesOverride.OverrideProhibited triggered in template files, so the steps to override this would be:

Create a new sniff, say GlobalVariablesOverrideTemplatesExclude that extends the GlobalVariablesOverrideSniff

use WordPressCS\WordPress\Sniffs\WP\GlobalVariablesOverrideSniff;

class GlobalVariablesOverrideTemplatesExcludeSniff extends GlobalVariablesOverrideSniff {
 /* [...] */
}

In the sniff add two regexes for template files and folders check. I took the one from FileNameSniff and modified it slightly (based on the comment by Danny Cooper)

const THEME_EXCEPTIONS_REGEX = '`
		^                    # Anchor to the beginning of the string.
		(?:
							 # Template prefixes which can have exceptions.
			(?:archive|category|content|embed|page|single|tag|taxonomy|template|tag|sidebar|front-page|header|footer|index|comments|search|searchform|404)
			-[^\.]+          # These need to be followed by a dash and some chars.
		)\.(?:php|inc)$      # End in .php (or .inc for the test files) and anchor to the end of the string.
	`Dx';

and for templates (could probably be improved, I'm not a regex expert, but when I test this with regex101 it seems like it makes too much passes)

const THEME_TEMPLATE_FOLDERS_REGEX = '`(?:template-parts/|templates/|partials/)`Dx';

Copy the entire process_token() method, but add two regex checks for the file path and file name at the beginning

$file = $this->strip_quotes( $this->phpcsFile->getFileName() ); // File path that can contain one of the folders from regex.
$fileName = basename( $file ); // File name to check with the first regex.

// If the regex matches skip checking for globals override.
if ( preg_match( self::THEME_EXCEPTIONS_REGEX, $fileName ) === 1 ) {
  return;
}

if ( preg_match( self::THEME_TEMPLATE_FOLDERS_REGEX, $file ) === 1 ) {
  return;
}

Am I on the right track?

dingo-d · 2019-04-23T07:20:25Z

@jrfnl Just pinging to check up on this 🙂

jrfnl · 2019-04-24T21:08:40Z

Am I on the right track?

@dingo-d Sorry, but no, not really. Might be easier if we discuss this over Slack ? Ping me if you like with a day/time.

dingo-d · 2019-05-01T14:30:16Z

I've updated the code after our call @jrfnl

The current sniff overloads the PrefixAllGlobals sniff from WPCS. It looks if the file path contains certain folder names (template-parts, templates and partials). The folders array is set as public property so it can be extended if needed.

Also, the file name is checked against the regex in the FileNameSniff.

The tests are not passing so I'd need some help to see if I'm on the right track here, and what am I doing wrong.

And I've added

<!-- Exclude PrefixAllGlobals Sniff from WPCS and use the one from this ruleset -->
<rule ref="WordPress.NamingConventions.PrefixAllGlobals">
  <severity>0</severity>
</rule>

in the ruleset.xml

Because

<rule ref="WordPress">
  <exclude name="WordPress.NamingConventions.PrefixAllGlobals"/>
</rule>

Would load all sniffs from the WordPress standard, and we don't want that.

I'm not 100% sure this is ok, and if this will silence errors when we don't want them to be silenced in the case of WordPress.NamingConventions.PrefixAllGlobals sniff.

jrfnl

@dingo-d I've had a look, this is much closer to what is needed than before 👍

I've left quite some remarks inline as we're definitely not there yet (sorry).

Additionally, you've now branched off master, while we work on develop. Could you please rebase the PR on the develop branch ?

jrfnl · 2019-05-04T09:59:17Z

WPThemeReview/ruleset.xml

@@ -150,4 +150,10 @@
 	<!-- Themes should never touch the timezone. -->
 	<rule ref="WordPress.WP.TimezoneChange"/>

+	<!-- Exclude PrefixAllGlobals Sniff from WPCS and use the one from this ruleset -->


This whole block needs to be removed.

The WPCS sniff is included on line 131. That inclusion should be removed instead of adding this block.
To keep the ruleset documentation to indicate that the rule is checked, I would propose changing the block there to:

Makes much more sense. I wasn't sure if I need this because I want to have its functionality, but it will be included via the new sniff (only slightly changed for themes). 👍

jrfnl · 2019-05-04T10:12:50Z

WPThemeReview/Sniffs/CoreFunctionality/PrefixAllGlobalsSniff.php

+use WordPressCS\WordPress\Sniffs\Files\FileNameSniff;
+
+/**
+ * Extend the PrefixAllGlobalsSniff and silence the errors if coming from template files or folders


Suggested change

* Extend the PrefixAllGlobalsSniff and silence the errors if coming from template files or folders

* Verify that everything defined in the global namespace is prefixed with a theme specific prefix.

*

* This sniff extends the upstream WPCS PrefixAllGlobalsSniff. The differences are:

* - For non-prefixed global variables, an error will only be thrown when the variable is created outside of a theme template file.

jrfnl · 2019-05-04T10:14:19Z

WPThemeReview/Sniffs/CoreFunctionality/PrefixAllGlobalsSniff.php

+	 *
+	 * @var array
+	 */
+	public $allowed_folders = [


This should be either protected or private.

public sniff properties in PHPCS can be adjusted/overruled from a custom ruleset. (I don't believe that was your intention or was it ?)

In fact it was. If I want to create my own ruleset based on this rule, I can maybe add some additional folders (not all people may name the template folders that way).

That was the rationale behind making it public 🙂 I can change it to protected if you think that the better way would be, when working on your own standard, to extend this sniff and just overload this property.

If non of those make sense than I can make it private and if we see some extra folder names popping up in the reviews, we can easily add those in the array.

It's fine to leave it as public if that's the intended behaviour.

In that case, you may want to adjust the property description to make that clear.
We may also need to have a think about whether the default value should in that case be an empty array instead ?

Note: setting this from a ruleset will overwrite the current value, unless someone explicitly uses the PHPCS 3.4.0 extends="true" attribute in the ruleset.
See: https://github.com/squizlabs/PHP_CodeSniffer/releases/tag/3.4.0

Would something like this be ok?

/** * The list of allowed folders to check the file path against. * * This array can be extended in the custom ruleset. * If you are using PHPCS prior to the version 3.4.0 you would overwrite the current value by setting: * * <property name="allowed_folders" type="array" values="template-parts,templates,partials,my-template-folder"> * * If you are using PHPCS 3.4.0 or greater you can extend this array by setting extends="true" attribute: * * <property name="allowed_folders" type="array" extends="true" values="my-template-folder"> * * @var array */

If we set empty array as a default value we would define the folders inside the ruleset.xml? I'm for it 🙂

Couple of things:

The example XML code is old-style PHPCS. You can use more readable arrays since 3.3.0.

You don't need to explain how custom rulesets work here, that may be more a thing for a wiki page or for the Readme.
Mentioning that the property is intended to be set from within a custom ruleset is all that's really needed.

If you want to allow people to extend the array, you need to set the initial values in the ruleset instead of in the sniff. Extending only works based on properties set in rulesets, so yes, that would make the default value for the property in the sniff an empty array.
If you're going that direction, you may want to mention in the documentation that the WPTRCS ruleset contains a base set for the property.
See: Feature request: Array property extending doesn't work when property has default value squizlabs/PHP_CodeSniffer#2228

jrfnl · 2019-05-04T10:14:30Z

WPThemeReview/Sniffs/CoreFunctionality/PrefixAllGlobalsSniff.php

+class PrefixAllGlobalsSniff extends WPCSPrefixAllGlobalsSniff {
+
+	/**
+	 * List of allowed folders to check the file path against


Suggested change

* List of allowed folders to check the file path against

* List of allowed folders to check the file path against.

jrfnl · 2019-05-04T10:18:19Z

WPThemeReview/Sniffs/CoreFunctionality/PrefixAllGlobalsSniff.php

+	];
+
+	/**
+	 * Check that defined global variables are prefixed, except if coming from template files.


Suggested change

* Check that defined global variables are prefixed, except if coming from template files.

* Check that defined global variables are prefixed.

*

* This overloads the parent method to allow for non-prefixed variables to be declared

* in template files as those are included from within a function and therefore would be

* local to that function.

jrfnl · 2019-05-04T11:05:56Z

WPThemeReview/Tests/CoreFunctionality/PrefixAllGlobalsUnitTest.php

+	 *
+	 * @var array
+	 */
+	private $expected_results = array(


This array can be dropped.

Instead of testing that we are receiving errors, we are testing that errors will not be thrown.

jrfnl · 2019-05-04T11:08:02Z

WPThemeReview/Tests/CoreFunctionality/PrefixAllGlobalsUnitTest.php

+	 */
+	public function getErrorList( $testFile = '' ) {
+
+		if ( isset( $this->expected_results[ $testFile ] ) ) {


See my remarks above. Only the test for the non-template file should trigger an error, none of the others should.

So I should remove this. But how do I test for the specific .inc files if they are triggering errors? Like footer_widgets.inc and say social-share.inc (outside of partials folder) that should also trigger an error.

Should I manually include the test files, like here: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/develop/WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.php?

The WPCS file name sniff is the only one to include a list like that in a property as all the errors would be on the same line and it improved the documentation of why and what.

Normally for sniffs which need to be tested using several files, the test method would use a unit test file name based switch which also allows more easily to the differentiate between the lines on which errors/warnings are being thrown between files.
The default case would be an empty array (explicitly expect no errors), so you would only need to list the non-template test case files in the switch.

Like in the file you linked above as well as in, for instance, this one: https://github.com/WPTRT/WPThemeReview/blob/develop/WPThemeReview/Tests/ThouShallNotUse/NoAutoGenerateUnitTest.php

Should I manually include the test files,

'The switch doesn't include the test files. Test case files with a SniffNameTest.1.inc (numbered) file name will be included for testing automatically by PHPCS, it's only for non-standard test case file names (which you need for this sniff) that you have to tell PHPCS which files to use in the test and you already do so via the getTestFiles() method.

jrfnl · 2019-05-04T11:27:42Z

WPThemeReview/Tests/CoreFunctionality/ThemeTemplatesException/footer_widgets.inc

@@ -0,0 +1,4 @@
+<!-- Annotation has to be on the second line as errors are thrown on line 1 and errors on annotation lines are ignored. -->
+phpcs:set WordPress.Files.FileName is_theme true


Property is for the wrong sniff.

Try:

// phpcs:set WPThemeReview.CoreFunctionality.PrefixAllGlobals prefixes[] my_theme

jrfnl · 2019-05-04T11:28:21Z

WPThemeReview/Tests/CoreFunctionality/ThemeTemplatesException/footer_widgets.inc

+<!-- Annotation has to be on the second line as errors are thrown on line 1 and errors on annotation lines are ignored. -->
+phpcs:set WordPress.Files.FileName is_theme true
+<?php
+/* phpcs:set WordPress.Files.FileName is_theme false */


And to clear it:

// phpcs:set WPThemeReview.CoreFunctionality.PrefixAllGlobals prefixes[]

jrfnl · 2019-05-04T11:30:21Z

WPThemeReview/Tests/CoreFunctionality/ThemeTemplatesException/footer_widgets.inc

@@ -0,0 +1,4 @@
+<!-- Annotation has to be on the second line as errors are thrown on line 1 and errors on annotation lines are ignored. -->
+phpcs:set WordPress.Files.FileName is_theme true
+<?php


You need to add some code to trigger the sniff so it will actually be tested.
Try something like:

$my_theme_var = 123; // OK, prefixed. $var = // OK, template file.

Although this should be in the header.inc as that is considered a template file, while footer_widgets.inc isn't, right?

It should be in all the test files for this sniff, but the OK should be changed to Bad for non-template files.

If the code is not in non-template files, the sniff wouldn't be triggered and we still wouldn't be testing if the "fall through to the parent sniff if the file is not a template file" works correctly.

dingo-d · 2019-05-05T08:10:25Z

One thing that I kinda realised now is that THEME_EXCEPTIONS_REGEX from the FileNameSniff doesn't check for all the cases mentioned in the comment by Danny Cooper.

Because of that the header.inc file is flagged as a non template file and I get an error in the tests.

In fact, when running tests, I get 0 for every file in preg_match( FileNameSniff::THEME_EXCEPTIONS_REGEX, $fileName ), so I'll always fall to the parent::process_variable_assignment( $stackPtr );.

I remember you mentioning that this change should be done upstream, but we'd have to wait for this, and for the new version of WPCS, and I'd like to fix this asap so that Theme Sniffer development can continue (we have quite some work to be done there).

Would it be better to define the regex in the sniff here, and once this is fixed upstream, we should just replace it with FileNameSniff::THEME_EXCEPTIONS_REGEX?

jrfnl

@dingo-d Went through it yet again and left some more remarks & replies.

One thing that I kinda realised now is that THEME_EXCEPTIONS_REGEX from the FileNameSniff doesn't check for all the cases mentioned in the comment by Danny Cooper.

Because of that the header.inc file is flagged as a non template file and I get an error in the tests.

The regex in the FileNameSniff is based on the template hierarchy in the theme handbook - see: https://developer.wordpress.org/themes/basics/template-hierarchy/

Has the hierarchy changed ? or did @DannyCooper accidentally list some "partials" ?

I remember you mentioning that this change should be done upstream, but we'd have to wait for this, and for the new version of WPCS, and I'd like to fix this asap so that Theme Sniffer development can continue (we have quite some work to be done there).

Would it be better to define the regex in the sniff here, and once this is fixed upstream, we should just replace it with FileNameSniff::THEME_EXCEPTIONS_REGEX?

If a PR to update the regex (with justification for the update) is submitted to WPCS, I'd be quite happy to release a new patch version of WPCS within a week or so. There have been some more small changes recently which would be good to get out in the world.

jrfnl · 2019-05-04T19:58:13Z

CHANGELOG.md

@@ -8,6 +8,10 @@ This projects adheres to [Semantic Versioning](https://semver.org/) and [Keep a

 _No documentation available about unreleased changes as of yet._

+## [0.2.0] - 2019-04-13


Still here, there's still a merge commit in the list of commits which brings it back. You may just want to squash all the commits into one and then get rid of it.

jrfnl · 2019-05-09T01:55:50Z

WPThemeReview/Sniffs/CoreFunctionality/PrefixAllGlobalsSniff.php

+	 *
+	 * @var array
+	 */
+	public $allowed_folders = [


Couple of things:

The example XML code is old-style PHPCS. You can use more readable arrays since 3.3.0.

You don't need to explain how custom rulesets work here, that may be more a thing for a wiki page or for the Readme.
Mentioning that the property is intended to be set from within a custom ruleset is all that's really needed.

If you want to allow people to extend the array, you need to set the initial values in the ruleset instead of in the sniff. Extending only works based on properties set in rulesets, so yes, that would make the default value for the property in the sniff an empty array.
If you're going that direction, you may want to mention in the documentation that the WPTRCS ruleset contains a base set for the property.
See: Feature request: Array property extending doesn't work when property has default value squizlabs/PHP_CodeSniffer#2228

jrfnl · 2019-05-09T01:57:54Z

WPThemeReview/Sniffs/CoreFunctionality/PrefixAllGlobalsSniff.php

+		$fileName = basename( $file );
+
+		// Don't process in case of a template file or folder.
+		if ( preg_match( FileNameSniff::THEME_EXCEPTIONS_REGEX, $fileName ) === 1 && $this->is_from_allowed_folder( $file ) ) {


I think this may solve the unit test issue ;-)

Suggested change

if ( preg_match( FileNameSniff::THEME_EXCEPTIONS_REGEX, $fileName ) === 1 && $this->is_from_allowed_folder( $file ) ) {

if ( preg_match( FileNameSniff::THEME_EXCEPTIONS_REGEX, $fileName ) === 1 || $this->is_from_allowed_folder( $file ) ) {

WPThemeReview/Sniffs/CoreFunctionality/PrefixAllGlobalsSniff.php

jrfnl · 2019-05-09T02:01:32Z

WPThemeReview/Tests/CoreFunctionality/PrefixAllGlobalsUnitTest.php

+					6 => 1,
+				);
+			case 'header.inc':
+				// Template file - all OK, fall through to the default case.


This comment seems incorrect.

WPThemeReview/Tests/CoreFunctionality/PrefixAllGlobalsUnitTest.php

jrfnl · 2019-05-09T02:04:43Z

WPThemeReview/Tests/CoreFunctionality/PrefixAllGlobalsUnitTest.php

+		}
+	}
+
+	/**


As a convention, this method should be above the getErrorList()/getWarningList() methods (as it will be called before either of them).

Yeah, this makes more sense 🙂

jrfnl · 2019-05-09T02:06:34Z

WPThemeReview/Tests/CoreFunctionality/ThemeTemplatesException/header.inc

+<!-- Annotation has to be on the second line as errors are thrown on line 1 and errors on annotation lines are ignored. -->
+// phpcs:set WPThemeReview.CoreFunctionality.PrefixAllGlobals prefixes[] my_theme
+<?php
+


Why is the $my_theme_var = 123; // OK, prefixed. snippet missing here ?

Forgot to add it, my bad.

jrfnl · 2019-05-09T02:09:07Z

WPThemeReview/Tests/CoreFunctionality/ThemeTemplatesException/footer_widgets.inc

@@ -0,0 +1,7 @@
+<!-- Annotation has to be on the second line as errors are thrown on line 1 and errors on annotation lines are ignored. -->


The above comment can be removed from all test files for this sniff. For this sniff, it's fine to have the annotation on line 1.

The "errors are thrown on line 1" comes from the FileName sniff, where that is true, but that is not the case for this sniff (errors are thrown on the line where the variable is declared).

jrfnl · 2019-05-09T02:13:49Z

composer.json

@@ -38,7 +38,7 @@
 	],
 	"require"    : {
 		"php"                      : ">=5.4",
-		"squizlabs/php_codesniffer": "^3.3.1",
+		"squizlabs/php_codesniffer": "^3.4.1",


Any particular reason to change the minimum requirement ? And if so, why to this particular version ?

I think this was the latest version when I started to work on it initially. I can up this to ^3.4.2.

But why change the minimum version at all at this time ? Is there anything in the newer PHPCS which this PR needs ? which TRTCS needs ? If not,why not keep it as is, in line with the WPCS minimum requirements ?

Ok, makes sense 🙂It's just a habit of mine to keep the libraries up to date (watching for BC breaks ofc).

Not a bad habit at all, but that update would be unrelated to this PR, so it ought to be a separate PR in that case (which would also update the readme + the travis script to match the changed dependency requirements)

dingo-d · 2019-05-09T08:29:26Z

I've fixed the issues, the tests should be passing now. If all is ok (I really hope it is 😂), you can squash commits and merge them to avoid the commit clutter 😄

jrfnl · 2019-05-09T10:15:10Z

I've fixed the issues, the tests should be passing now. If all is ok (I really hope it is)

Nearly there. Just the question about the PHPCS dependency change + the defaults for the $allowed_folders still being in the sniff remaining.

dingo-d · 2019-05-09T10:50:57Z

I think this should be it 🙂

jrfnl · 2019-05-09T22:51:48Z

@dingo-d Ok, had yet another critical look. You're so close now.
There were some very minor tidying up things, not worth the up-and-back, so I've just added a new commit to the PR to sort that out.

Now the only thing left is the unit tests.

You have five unit test files - none of which AFAICS will match the regex. so the regex part is untested.
The attachment.inc file says inline OK, template file., but is listed in the getErrorList() method in the group of files to expect an error, while at the same time, it has a comment within that method as if it shouldn't.
For attachment.inc to match the regex would need to be updated in the FileNameSniff.

A filename like page-recent-news.php would already match and could make for a valid test case.

The other three files in the main directory all test the same thing: the fall through, so only one of those would suffice.

Another test case which could be added would be to have a /ThemeTemplatesException/template-parts/subset/my-file-name.inc file - this would test non-template test file names in subdirectories of one of the allowed folders are correctly excluded (which AFAICS they currently are).

If you could have a think about that, I'll have a look at the template hierarchy and see what has changed/been updated since the regex was created and see about updating the regex in WPCS.

joyously · 2019-05-09T23:32:06Z

Themes aren't supposed to use page template names like page-anything.php. Is the one in the test case supposed to be allowed or we're testing a different thing here?

jrfnl · 2019-05-09T23:45:50Z

Themes aren't supposed to use page template names like page-anything.php.

It's an example which literally comes from the theme handbook: https://developer.wordpress.org/themes/basics/template-hierarchy/#single-page - second item in the list.

Is the one in the test case supposed to be allowed or we're testing a different thing here?

Yes, it is supposed to be allowed.

joyously · 2019-05-09T23:49:19Z

Right, the theme handbook is for themes in general, and not specifically about themes in the repository.
A file like page-xxx.php should be recognized as a template file, but should not be in a theme in the repository.

jrfnl · 2019-05-09T23:54:48Z

A file like page-xxx.php should be recognized as a template file, but should not be in a theme in the repository.

In that case, for the purposes of this sniff, I'd say the sniff should be agnostic on whether or the theme would be allowed in the repository.

If it would be so desired that the sniffer would check for a list of allowed template file names and throw warnings for page-xxx.php type template files, that should be in another (new) sniff.
If you think a sniff like that should be added, let's see if there's an open issue for it and if not, open one.

jrfnl · 2019-05-10T01:14:36Z

I'll have a look at the template hierarchy and see what has changed/been updated since the regex was created and see about updating the regex in WPCS.

Ok, looking more closely at the regex in WPCS, it only looks for the theme template files which are allowed to break the file name rule of "no underscore", i.e. page-post_type.php and the likes, so adjusting that regex is not really an option.

In other words, we need to solve it here.

dingo-d · 2019-05-10T06:44:20Z

@jrfnl I'm happy if you can take this sniff over, it will go a lot quickly than going back and forth with the comments 🙂

I'm puzzled about the comment about the page-xxx.php not being allowed in the theme, and I am failing to see the rationale behind it...

ernilambar · 2019-05-10T08:17:43Z

I'm puzzled about the comment about the page-xxx.php not being allowed in the theme, and I am failing to see the rationale behind it...

See https://developer.wordpress.org/themes/template-files-section/page-template-files/#creating-custom-page-templates-for-global-use

dingo-d · 2019-05-10T08:41:09Z

This looks good to me, feel free to merge it 👍 Thanks for the help with this, I do appreciate it.

This new sniff replaces the `WordPress.NamingConventions.PrefixAllGlobals` sniff which it extends. The WPCS sniff checks whether everything - i.e. namespaces, classes, functions, constants, hooks, variables - declared in the global namespace is prefixed with a theme/plugin specific prefix. For variables declared in theme template files, this leads to a lot of errors being thrown. Theme template files are, when using the normal WP flow, included from within a function and are therefore _local_ to that function. With this in mind, for the purposes of the ThemeReview checker, variables declared within theme template files can be ignored. The new WPTRCS native version of the sniff overloads the prefix check for variables only and will bow out if the file being scanned has a typical theme template file name. For all other files, it will fall through to the WPCS native sniff. For all other constructs in theme template files, i.e. namespaces, classes, functions etc, the checks from the WPCS native sniff will still be run. Notes: * The new sniff adds a public `$allowed_folders` property to whitelist files in specific folders of a theme as template files. The `ruleset.xml` file sets this property to a limited set of folders whitelisted by default. The default property value can be overruled and/or extended from a custom ruleset. * Similar to the WPCS FileNameSniff, this sniff does not currently allow for mimetype sublevel only theme template file names, such as `plain.php`. Includes dedicated unit tests.

jrfnl · 2019-05-10T16:10:55Z

FYI: As discussed with @dingo-d on Slack, I have rebased the PR and squashed all the commits into one commit and added a commit message explaining the change in more detail.

Once the build passes for this new commit, this PR will be merged.

dingo-d added the enhancement label Apr 1, 2019

dingo-d self-assigned this Apr 1, 2019

dingo-d requested a review from jrfnl April 1, 2019 09:40

jrfnl requested changes Apr 1, 2019

View reviewed changes

jrfnl requested changes May 4, 2019

View reviewed changes

dingo-d requested a review from jrfnl May 4, 2019 15:48

jrfnl requested changes May 9, 2019

View reviewed changes

jrfnl force-pushed the feature/prefix-globals-warning branch from 5d985ab to 5983bd9 Compare May 9, 2019 22:39

jrfnl force-pushed the feature/prefix-globals-warning branch from 5983bd9 to a577759 Compare May 9, 2019 23:29

jrfnl force-pushed the feature/prefix-globals-warning branch 3 times, most recently from 4d78b7d to bae3401 Compare May 10, 2019 01:46

jrfnl added this to the 0.2.0 milestone May 10, 2019

jrfnl force-pushed the feature/prefix-globals-warning branch from 9bc89de to d7460d4 Compare May 10, 2019 16:08

jrfnl approved these changes May 10, 2019

View reviewed changes

jrfnl merged commit 8398440 into develop May 10, 2019

delete-merged-branch bot deleted the feature/prefix-globals-warning branch May 10, 2019 17:51

dingo-d mentioned this pull request May 18, 2019

🍾 Next release planning [0.2.0] #189

Closed

jrfnl mentioned this pull request Jul 23, 2019

Prefix global variables in themes WordPress/WordPress-Coding-Standards#1043

Closed

- * Extend the PrefixAllGlobalsSniff and silence the errors if coming from template files or folders
+ * Verify that everything defined in the global namespace is prefixed with a theme specific prefix.
+ *
+ * This sniff extends the upstream WPCS PrefixAllGlobalsSniff. The differences are:
+ * - For non-prefixed global variables, an error will only be thrown when the variable is created outside of a theme template file.

	* List of allowed folders to check the file path against
	* List of allowed folders to check the file path against.

-	 * Check that defined global variables are prefixed, except if coming from template files.
+	 * Check that defined global variables are prefixed.
+	 *
+	 * This overloads the parent method to allow for non-prefixed variables to be declared
+	 * in template files as those are included from within a function and therefore would be
+	 * local to that function.

		@@ -0,0 +1,4 @@
		<!-- Annotation has to be on the second line as errors are thrown on line 1 and errors on annotation lines are ignored. -->
		phpcs:set WordPress.Files.FileName is_theme true

		@@ -8,6 +8,10 @@ This projects adheres to [Semantic Versioning](https://semver.org/) and [Keep a

		_No documentation available about unreleased changes as of yet._

		## [0.2.0] - 2019-04-13

	if ( preg_match( FileNameSniff::THEME_EXCEPTIONS_REGEX, $fileName ) === 1 && $this->is_from_allowed_folder( $file ) ) {
	if ( preg_match( FileNameSniff::THEME_EXCEPTIONS_REGEX, $fileName ) === 1 \|\| $this->is_from_allowed_folder( $file ) ) {

+              		}
+              	}
+              	/**

		@@ -0,0 +1,7 @@
		<!-- Annotation has to be on the second line as errors are thrown on line 1 and errors on annotation lines are ignored. -->

Feature/prefix globals warning #208

Feature/prefix globals warning #208

Conversation

dingo-d commented Apr 1, 2019

dingo-d commented Apr 1, 2019

jrfnl left a comment

Choose a reason for hiding this comment

jrfnl commented Apr 1, 2019 • edited Loading

dingo-d commented Apr 1, 2019

jrfnl commented Apr 1, 2019

DannyCooper commented Apr 1, 2019 • edited Loading

joyously commented Apr 1, 2019

dingo-d commented Apr 1, 2019

jrfnl commented Apr 1, 2019 • edited Loading

dingo-d commented Apr 13, 2019 • edited Loading

dingo-d commented Apr 23, 2019

jrfnl commented Apr 24, 2019

dingo-d commented May 1, 2019

jrfnl left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dingo-d May 4, 2019 • edited Loading

Choose a reason for hiding this comment

jrfnl May 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dingo-d May 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl May 4, 2019 • edited Loading

Choose a reason for hiding this comment

dingo-d commented May 5, 2019 • edited Loading

jrfnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl May 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dingo-d commented May 9, 2019

jrfnl commented May 9, 2019

dingo-d commented May 9, 2019

jrfnl commented May 9, 2019

joyously commented May 9, 2019

jrfnl commented May 9, 2019

joyously commented May 9, 2019

jrfnl commented May 9, 2019

jrfnl commented May 10, 2019

dingo-d commented May 10, 2019

ernilambar commented May 10, 2019

dingo-d commented May 10, 2019

jrfnl commented May 10, 2019

jrfnl commented Apr 1, 2019 •

edited

Loading

DannyCooper commented Apr 1, 2019 •

edited

Loading

jrfnl commented Apr 1, 2019 •

edited

Loading

dingo-d commented Apr 13, 2019 •

edited

Loading

jrfnl left a comment •

edited

Loading

dingo-d May 4, 2019 •

edited

Loading

jrfnl May 4, 2019 •

edited

Loading

dingo-d May 4, 2019 •

edited

Loading

jrfnl May 4, 2019 •

edited

Loading

dingo-d commented May 5, 2019 •

edited

Loading

jrfnl May 9, 2019 •

edited

Loading