Feature/prefix globals warning

[dingo-d]

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]

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]

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:

1. Either downgrade a specific errorcode to a warning, not the whole sniff.
2. 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]

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 :+1:

[jrfnl]

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]

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]

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]

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]

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]

@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:

1. Create a new sniff, say GlobalVariablesOverrideTemplatesExclude that extends the GlobalVariablesOverrideSniff

use WordPressCS\WordPress\Sniffs\WP\GlobalVariablesOverrideSniff;

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

2. 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';

3. 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]

@jrfnl Just pinging to check up on this 🙂

[jrfnl]

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]

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.

[dingo-d]

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?

[dingo-d]

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]

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]

I think this should be it 🙂

[jrfnl]

@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]

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]

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]

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]

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]

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]

@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]

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]

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

[jrfnl]

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.