Issue 1157/Proposal 2: rename sniffs #1242

jrfnl · 2017-11-26T05:44:54Z

While there currently isn't consensus yet about proposal 1 of issue #1157, proposal 2 seems widely supported.

To that end, I have prepared the necessary changes.

This is PR 1 in a series of three PRs to address this and by far the largest one.

This PR basically takes care of all the sniff renames, sniff deprecations and error to warning downgrades as proposed, with the exception of the splitting of the PostsPerPage sniff which I will address in a separate PR.

I've elected to do all the renames in one go as they all require changes to the ruleset.xml file for the top-level WordPress ruleset and would otherwise all conflict or have to be pulled one by one, each waiting on the previous one.

All changes have been made in separate commits though for easier review and better traceability in the git history.

The original sniff class still exists, but extends the renamed sniff now. In addition, a deprecation warning will be thrown if this sniff is included directly by a custom ruleset. A deprecation warning will also be thrown if the sniff is included directly just to set custom properties. Note: if the sniff was explicitly **excluded** via a custom ruleset, no deprecation warning will be thrown and the exclusion will have no effect. The custom ruleset will need to be updated to use the new sniff name for the exclusion to be reinstated.

The messages thrown by the sniff have been downgraded to warnings and the message text adjusted to make the sniff more generically usable. For the VIP ruleset, this change is undone via custom ruleset properties.

The original sniff class still exists, but extends the renamed sniff now. In addition, a deprecation warning will be thrown if this sniff is included directly by a custom ruleset. A deprecation warning will also be thrown if the sniff is included directly just to set custom properties. Note: if the sniff was explicitly **excluded** via a custom ruleset, no deprecation warning will be thrown and the exclusion will have no effect. The custom ruleset will need to be updated to use the new sniff name for the exclusion to be reinstated.

The original sniff class still exists, but extends the renamed sniff now. In addition, a deprecation warning will be thrown if this sniff is included directly by a custom ruleset. Note: if the sniff was explicitly **excluded** via a custom ruleset, no deprecation warning will be thrown and the exclusion will have no effect. The custom ruleset will need to be updated to use the new sniff name for the exclusion to be reinstated.

The original sniff class still exists, but extends the renamed sniff now. In addition, a deprecation warning will be thrown if this sniff is included directly by a custom ruleset. A deprecation warning will also be thrown if the sniff is included directly just to set custom properties. Note: if the sniff was explicitly **excluded** via a custom ruleset, no deprecation warning will be thrown and the exclusion will have no effect. The custom ruleset will need to be updated to use the new sniff name for the exclusion to be reinstated.

The original sniff class still exists, but extends the renamed sniff now. Note: if the sniff was explicitly **excluded** via a custom ruleset, no deprecation warning will be thrown and the exclusion will have no effect. The custom ruleset will need to be updated to use the new sniff name for the exclusion to be reinstated.

The message thrown by the sniff has been downgraded to a warning and the message text adjusted to make the sniff more generically usable. For the VIP ruleset, this change is undone via a custom ruleset property.

…egory The original sniff class still exists, but extends the renamed sniff now. In addition, a deprecation warning will be thrown if this sniff is included directly by a custom ruleset. A deprecation warning will also be thrown if the sniff is included directly just to set custom properties. Note: if the sniff was explicitly **excluded** via a custom ruleset, no deprecation warning will be thrown and the exclusion will have no effect. The custom ruleset will need to be updated to use the new sniff name for the exclusion to be reinstated.

The original sniff class still exists, but extends the renamed sniff now. In addition, a deprecation warning will be thrown if this sniff is included directly by a custom ruleset. A deprecation warning will also be thrown if the sniff is included directly just to set custom properties. Note: if the sniff was explicitly **excluded** via a custom ruleset, no deprecation warning will be thrown and the exclusion will have no effect. The custom ruleset will need to be updated to use the new sniff name for the exclusion to be reinstated.

The message thrown by the sniff has been downgraded to a warning and the message text adjusted to make the sniff more generically usable. For the VIP ruleset, this change is undone via a custom ruleset property.

The original sniff class still exists, but extends the renamed sniff now. In addition, a deprecation warning will be thrown if this sniff is included directly by a custom ruleset. A deprecation warning will also be thrown if the sniff is included directly just to set custom properties. Note: if the sniff was explicitly **excluded** via a custom ruleset, no deprecation warning will be thrown and the exclusion will have no effect. The custom ruleset will need to be updated to use the new sniff name for the exclusion to be reinstated.

GaryJones · 2017-11-26T09:54:06Z

Reminder: Check wiki for old references too.

JDGrimes · 2017-11-27T15:53:17Z

This looks good to me, but I wonder if we should do some additional renames while we are at it.

For example, I'm unclear on the difference between PHP, CodeAnalysis, and Functions.

Going down through the list of our different sniff groupings:

Arrays looks OK.
CSRF is going to be deprecated by this PR.
Classes contains only one sniff that seems OK where it is, I guess. Although as with Functions (see below), what should go here as opposed to other places (PHP or CodeAnalysis) may be kind of fuzzy.
CodeAnalysis seems a reasonable category, but how is it different from PHP? Should some sniffs be moved here?
DB is fine.
Files I see no problem with. Though on the other hand, the sniff that is there could probably just as easily be moved to NamingConventions.
Functions contains three sniffs: DontExtract, FunctionCallSignatureNoParams, and FunctionRestrictions (which is deprecated). Why should DontExtract be here, rather than in PHP (with other function related sniffs)? And why should FunctionCallSignatureNoParams be here, rather than in Whitespace? And is this category supposed to be about function declarations, or about function usages? Currently it is about both, but we are inconsistent as to what we place here and what we place elsewhere. Should we just deprecate this category entirely?
NamingConventions looks good.
PHP seems to hold most of our generic function-related sniffs. It also contains some sniffs that could just as easily go under CodeAnalysis, like YodaConditions and StrictComparisons. Those sniffs could probably be used for JS as well as PHP. I'm not sure that we really even include JS sniffing in our vision for WPCS, which makes the PHP category kind of redundant.
VIP I guess is dealt with pretty well by this PR already.
Variables contains only VariableRestrictions (which is deprecated), and GlobalVariables. Since the latter is specifically related to WordPress globals, it might be reasonable to move it to WP and get rid of this category entirely. Other variable-related sniffs seem to fall into other categories already (like NamingConventions).
WP looks good to me, for sniffs specifically related to WordPress-provided functions/classes/etc., that don't fall into other discrete categories like Security or DB (as fixed by this PR).
WhiteSpace looks good.
XSS is dealt with by this PR, and will now be deprecated.

So basically, CodeAnalysis, Functions, PHP, and Variables could probably use some work. Classes and Files might also be ripe for change. Or would it just be unnecessary churn? Thoughts?

I realize that this initial proposal was mainly intended to address ease of including/excluding DB and Security sniffs in particular, and preparing VIP for deprecation. So maybe any other changes should be dealt with separately?

jrfnl · 2017-11-27T18:33:34Z

@JDGrimes As a general rule for both the proposal as well as most sniffs where I've added new categories, I've looked towards PHPCS upstream to see where similar sniffs upstream were placed and tried to use categories which were consistent with upstream.

Classes contains only one sniff that seems OK where it is, I guess. Although as with Functions (see below), what should go here as opposed to other places (PHP or CodeAnalysis) may be kind of fuzzy.

Upstream Classes is used for any class specific (formatting) sniffs. Example sniffs from upstream in this category (various standards): DuplicateClassName, OpeningBraceSameLine, ClassDeclaration, PropertyDeclaration, SelfMemberReference, ValidClassName.

We mainly use upstream sniffs for these things at the moment, except for the ClassInstantiation for which there is no upstream sniff.

CodeAnalysis seems a reasonable category, but how is it different from PHP? Should some sniffs be moved here?

To me, the difference between the CodeAnalysis category and the PHP category is in the kind of things being analysed.

CodeAnalysis - detect sloppy or messy code. Often things which the PHP Mess Detector would detect as well, i.e. EmptyStatement. Both the sniffs we have in that category will be removed in due time in favour of the same sniffs upstream.
- The AssignmentInCondition sniff has already been merged upstream, but our minimum required version will need to go up before we can use the upstream version (also upstream currently does not detect assignments in condition in ternaries yet)
- The EmptyStatement sniff was added to deal with some empty statement detection missing in the upstream sister-sniff, but needed for the core project. I'm not sure by heart whether I've already pulled our additions upstream. If not, they should be.
PHP - detect certain PHP constructs or features on which we have an opinion about whether to use these or not.

With that in mind, PHP.StrictComparisons could be moved to CodeAnalysis. However, it is also still well placed in the PHP category, especially as the StrictInArray sniff is there too, which is very much linked to specific PHP functions and therefore seems correctly placed in PHP.

Side note: both of these sniffs would be good candidates to pull upstream in the foreseeable future.

The only "odd one out" I see in the PHP category would be YodaConditions, but I'm not sure where else that should be placed.
Personally, I'd rather have a discussion whether that rule is still needed at all, now the AssignmentInCondition sniff is available.

Files I see no problem with. Though on the other hand, the sniff that is there could probably just as easily be moved to NamingConventions.

AFAICS, the difference between NamingConventions and Files is:

NamingConventions - naming of code constructs. Looks for a construct declaration and then examines the name.
File - anything file specific. Upstream this includes sniffs like ByteOrderMark, EndFileNewline, LineLength, (No)ClosingTag and OneObjectStructurePerFile.
Generally speaking these sniffs register the T_OPEN_TAG, scan a whole file in one go and prevent PHPCS from triggering the sniff a second time.

With that in mind, the WP sniffs follow the exact same logic.

Functions contains three sniffs: DontExtract, FunctionCallSignatureNoParams, and FunctionRestrictions (which is deprecated). Why should DontExtract be here, rather than in PHP (with other function related sniffs)?

DontExtract is being moved to PHP as part of this PR 😎

And why should FunctionCallSignatureNoParams be here, rather than in Whitespace? And is this category supposed to be about function declarations, or about function usages? Currently it is about both, but we are inconsistent as to what we place here and what we place elsewhere. Should we just deprecate this category entirely?

Upstream, this category is used similar to Classes and contains both sniffs related to function declarations as well as function calls. Just like classes is being used for both class declaration sniffs as well as class instantiation sniffs.

There are a couple of open issues - #1138 comes to mind - related to function call signatures, for which I'm considering writing a WPCS specific sniff which would then be placed in this category.

Basically, WhiteSpace is used upstream (and in WPCS) only for generic whitespace sniffs which are not linked to a language structure which creates scope - with the emphasis on that last bit: creates scope -.

PHP seems to hold most of our generic function-related sniffs. It also contains some sniffs that could just as easily go under CodeAnalysis, like YodaConditions and StrictComparisons.

See my comments about this above.

Those sniffs could probably be used for JS as well as PHP. I'm not sure that we really even include JS sniffing in our vision for WPCS, which makes the PHP category kind of redundant.

If I remember correctly, we do in some of the whitespace and file related sniffs.
Personally I'd be happy for WPCS to also cover JS things, as long as they aren't already covered by JSLint or JSHint.

Variables contains only VariableRestrictions (which is deprecated), and GlobalVariables. Since the latter is specifically related to WordPress globals, it might be reasonable to move it to WP and get rid of this category entirely. Other variable-related sniffs seem to fall into other categories already (like NamingConventions).

Upstream there is no Variables category. I'd be in favour of moving the GlobalVariables sniff to the WP category and maybe even to rename it to GlobalVariablesOverwrite to make it more explicit what the sniff detects.
The category could then be removed in the drop 2.x support/remove all deprecations release in summer 2018.

JDGrimes · 2017-11-27T19:55:03Z

@jrfnl All of that makes a lot of sense.

Upstream there is no Variables category. I'd be in favour of moving the GlobalVariables sniff to the WP category and maybe even to rename it to GlobalVariablesOverwrite to make it more explicit what the sniff detects.
The category could then be removed in the drop 2.x support/remove all deprecations release in summer 2018.

I'd be in favor of this as well.

…d rename The sniff has been renamed from `GlobalVariables` to `GlobalVariablesOverride` to make it easier to understand at one glance what the sniff is about. The original sniff class still exists, but extends the renamed sniff now. In addition, a deprecation warning will be thrown if this sniff is included directly by a custom ruleset. A deprecation warning will also be thrown if the sniff is included directly just to set custom properties. Note: if the sniff was explicitly **excluded** via a custom ruleset, no deprecation warning will be thrown and the exclusion will have no effect. The custom ruleset will need to be updated to use the new sniff name for the exclusion to be reinstated.

jrfnl · 2017-11-28T09:48:05Z

I've added an additional commit to handle the rename of the Variables.GlobalVariables sniff to WP.GlobalVariablesOverride.

Using Override instead of Overwrite for consistency as that's what's been used elsewhere in the sniff.

GaryJones · 2017-12-13T09:59:09Z

@jrfnl Good to merge from me.

jrfnl · 2017-12-14T04:43:05Z

As this has now been merged, I'd like to change the wiki as well. I will annotate that the change in sniff names happened in version 1.0.0 as discussed in #1247 and I will change the milestone name from 0.15.0 to 1.0.0

GaryJones · 2017-12-20T17:10:52Z

I'm getting the following when using the develop branch on a plugin's codebase:

1 | WARNING | [ ] The "WordPress.XSS.EscapeOutput" sniff has been renamed to "WordPress.XSS.EscapeOutput". Please update your custom ruleset. (WordPress.XSS.EscapeOutput.FoundPropertyForDeprecatedSniff)
1 | WARNING | [ ] The "WordPress.CSRF.NonceVerification" sniff has been renamed to "WordPress.Security.NonceVerification". Please update your custom ruleset. (WordPress.CSRF.NonceVerification.FoundPropertyForDeprecatedSniff)

The second one seems sensible (though I'm not sure why it's appearing since my phpcs.xml.dist simply includes WordPress and excludes WordPress-VIP i.e. nothing referenced by name), but the first one seems like a wrong message.

jrfnl · 2017-12-20T18:45:35Z

@GaryJones Good catch! The message text should be fixed via #1263.

As for why the messages are showing... not sure... Could you point me to the ruleset so I can run some tests ?
I would expect a custom property to be defined for that message to show up.

GaryJones · 2017-12-20T19:24:16Z

@jrfnl My PR has just been merged, so see https://github.com/NicktheGeek/genesis-featured-widget-amplified/blob/hotfix/0.9.1/.phpcs.xml.dist

With the corrected messages:

GaryJones · 2017-12-20T20:50:04Z

@jrfnl So, it seems to be this:

XSS.EscapeOutputSniff is loaded.
XSS.EscapeOutput.DeprecatedSniff has severity 0 set in WordPress/ruleset.xml, so the first warning doesn't get reported. All good.
There is then a bunch of checks for to see whether any custom properties have been set - it checks the public custom properties against the protected properties that are used internally after the custom ones have been merged in.
The merging in is done in mergeFunctionLists(), and this is called at the top of the Security.EscapeOutputSniff's (parent) process_token() method.
However, this parent process_token() is not called until after those "public property vs protected property" checks are done i.e. on the first round, the checks are never going to pass.

A quick solution would be to add $this->mergeFunctionLists(); to the top of the process_token() calls in deprecated Sniffs - that way, the custom properties have been merged with the internal protected property. Certainly, it seems to remove the warnings in my case, though I'm not sure if there is any side-effect of running $this->mergeFunctionLists(); twice. If not, then that should be good to go.

tl;dr: A vs B check is fine in itself, but B hasn't been initialised at the time the first check is done.

jrfnl · 2017-12-20T20:59:31Z

@GaryJones I'm been looking into this as well. The defaults for the addedCustomproperties sub-values are null and not array() which is why the compare fails.
I'm currently trying to figure out why I set those to null initially and whether they can safely be changed to array(). If so, that will be the solution.
I'm also trying to figure out why you only get this for these two sniffs and not for, for instance, the ValidatedSanitizedInput sniff which should also be affected.

A quick solution would be to add $this->mergeFunctionLists(); to the top of the process_token() calls in deprecated Sniffs - that way, the custom properties have been merged with the internal protected property.

That would also prevent the deprecation warnings from being thrown when the custom properties have been set using the old sniff names, which is exactly the situation for which we want to throw the messages, so that would not work.

GaryJones · 2017-12-21T08:04:02Z

The defaults for the addedCustomproperties sub-values are null and not array() which is why the compare fails.
I'm currently trying to figure out why I set those to null initially and whether they can safely be changed to array(). If so, that will be the solution.

I also spotted that - and that solution would work for me.

jrfnl · 2018-06-29T20:30:33Z

FYI: I've updated the wiki pages to use the new sniff names.

jrfnl added 13 commits November 26, 2017 04:14

DirectDatabaseQuery: downgrade messages to warning

4c98056

The messages thrown by the sniff have been downgraded to warnings and the message text adjusted to make the sniff more generically usable. For the VIP ruleset, this change is undone via custom ruleset properties.

PluginMenuSlug: downgrade message to warning

4980804

The message thrown by the sniff has been downgraded to a warning and the message text adjusted to make the sniff more generically usable. For the VIP ruleset, this change is undone via a custom ruleset property.

CronInterval: downgrade message to warning

77e9086

The message thrown by the sniff has been downgraded to a warning and the message text adjusted to make the sniff more generically usable. For the VIP ruleset, this change is undone via a custom ruleset property.

jrfnl added the Status: Review ready label Nov 26, 2017

jrfnl added this to the 0.15.0 milestone Nov 26, 2017

jrfnl changed the title ~~Feature/1157 part 2 rename sniffs~~ Issue 1157/Proposal 2: rename sniffs Nov 26, 2017

jrfnl mentioned this pull request Nov 28, 2017

Proposal for restructuring the rulesets & some of the sniffs #1157

Closed

JDGrimes approved these changes Nov 28, 2017

View reviewed changes

JDGrimes merged commit 4de1cd6 into develop Dec 13, 2017

JDGrimes deleted the feature/1157-part-2-rename-sniffs branch December 13, 2017 13:06

jrfnl mentioned this pull request Dec 14, 2017

PostsPerPage: split sniff #1260

Merged

jrfnl mentioned this pull request Dec 20, 2017

Fix two "typo"'s in deprecation messages #1263

Merged

GaryJones mentioned this pull request Dec 20, 2017

Development improvements NicktheGeek/genesis-featured-widget-amplified#7

Merged

jrfnl mentioned this pull request Jan 6, 2018

Fix version numbers in docblock tags #1281

Merged

GaryJones mentioned this pull request Mar 12, 2018

Renamed sniff reported on line 1 #1325

Closed

jrfnl mentioned this pull request Jun 26, 2018

Deprecate VIP ruleset, and remove from WordPress ruleset #1309

Closed

jrfnl mentioned this pull request Jul 3, 2018

Migrate Remaining VIP Sniffs and Deprecate #1409

Closed

GaryJones removed the Status: Review ready label Aug 21, 2018

jrfnl mentioned this pull request Aug 8, 2022

WP/CronInterval: bugfix for fully qualified time constants #2073

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 1157/Proposal 2: rename sniffs #1242

Issue 1157/Proposal 2: rename sniffs #1242

jrfnl commented Nov 26, 2017

GaryJones commented Nov 26, 2017

JDGrimes commented Nov 27, 2017

jrfnl commented Nov 27, 2017 •

edited

Loading

JDGrimes commented Nov 27, 2017

jrfnl commented Nov 28, 2017

GaryJones commented Dec 13, 2017

jrfnl commented Dec 14, 2017

GaryJones commented Dec 20, 2017 •

edited

Loading

jrfnl commented Dec 20, 2017 •

edited

Loading

GaryJones commented Dec 20, 2017 •

edited

Loading

GaryJones commented Dec 20, 2017 •

edited

Loading

jrfnl commented Dec 20, 2017

GaryJones commented Dec 21, 2017

jrfnl commented Jun 29, 2018

Issue 1157/Proposal 2: rename sniffs #1242

Issue 1157/Proposal 2: rename sniffs #1242

Conversation

jrfnl commented Nov 26, 2017

GaryJones commented Nov 26, 2017

JDGrimes commented Nov 27, 2017

jrfnl commented Nov 27, 2017 • edited Loading

JDGrimes commented Nov 27, 2017

jrfnl commented Nov 28, 2017

GaryJones commented Dec 13, 2017

jrfnl commented Dec 14, 2017

GaryJones commented Dec 20, 2017 • edited Loading

jrfnl commented Dec 20, 2017 • edited Loading

GaryJones commented Dec 20, 2017 • edited Loading

GaryJones commented Dec 20, 2017 • edited Loading

jrfnl commented Dec 20, 2017

GaryJones commented Dec 21, 2017

jrfnl commented Jun 29, 2018

jrfnl commented Nov 27, 2017 •

edited

Loading

GaryJones commented Dec 20, 2017 •

edited

Loading

jrfnl commented Dec 20, 2017 •

edited

Loading

GaryJones commented Dec 20, 2017 •

edited

Loading

GaryJones commented Dec 20, 2017 •

edited

Loading