Recategorize sniffs currently in "VIP" folder

[rebeccahum]

Bug Description

It would be nice to re-categorize the "VIP" folder into more relevant folders.

[GaryJones]

This would be a breaking change.

As of right now, VIPCS is less than 1.0.0, but this still needs to be thought out and agreed before changing.

Why?

Any references to WordPressVIPMinimum.VIP.... in <rule> elements in the phpcs.xml.dist files, and similar in // phpcs:ignore or // phpcs:disable comments will be affected. The former will stop PHPCS from running, the latter will reported previously hidden violations.

WPCS made these changes for WPCS 1.0.0, but kept the old Sniff classes present with deprecation notices at runtime, and the old Sniff classes will be removed in 2.0.0. See notes for the 1.0.0 release.

[GaryJones]

@rebeccahum Did you have any suggestions on where you'd like to move the sniffs currently in the VIP category? Ideally, I think it would be good to try and get rid of VIP category completely.

Current VIP category sniffs:

• ErrorControl
• EscapingVoidReturnFunctions
• ExitAfterRedirect
• FetchingRemoteData
• FlushRewriteRules
• MergeConflict
• PHPFilterFunctions
• ProperEscapingFunction
• RegexpCompare
• RemoteRequestTimeout
• RestrictedFunctions
• Robotstxt
• StaticStrreplace
• TaxonomyMetaInOptions
• WPQueryParams

[rebeccahum]

Ideally, I think it would be good to try and get rid of VIP category completely.
Agreed.

Here are some categories I'd like to propose to start off with:

• Security
• Performance
• Themes
• Options
• VIP-Compatibility

[GaryJones]

• Themes

Do we have much that is specific to themes?

• Options

What is there for Options, that isn't already covered by Security or Performance?

• VIP-Compatibility

How about Platform-Compatibility?

[rebeccahum]

Do we have much that is specific to themes?

Eh, I guess not.

What is there for Options, that isn't already covered by Security or Performance?

I was thinking for #259 maybe...but that would be better with the "Compatiblity" category.

Platform-Compatibility

Hmmm, this doesn't read nice to me. Would "Compatibility" suffice?

[GaryJones]

Would "Compatibility" suffice?

Yes.

Go ahead and make a table / list of which sniffs would go into which category, and don't forget to include the ones from #323. It'll be easier to agree on the mapping first, before starting the work.

[rebeccahum]

Security

• EscapingVoidReturnFunctions
• ExitAfterRedirect
• PHPFilterFunctions
• ProperEscapingFunction
• StaticStrreplace

Performance

• FetchingRemoteData
• FlushRewriteRules
• NoPaging
• OrderByRand
• RegexpCompare
• RemoteRequestTimeout
• WPQueryParams
• TaxonomyMetaInOptions

Compatibility

• FileSystemWritesDisallow
• Robotstxt

Functions

• RestrictedFunctions
• SessionFunctionsUsage - I'm thinking we can move this list to the RestrictedFunctionsSniff

Variables

• RestrictedVariables
• SessionVariableUsage - I'm thinking we can move this list to the RestrictedVariablesSniff

VIP (a.k.a ones that need to be assigned a category still...)

• AdminBarRemoval - @GaryJones What are your thoughts on a UserExperience category? Or is that too broad/may not be used enough?
• ErrorControl - Would WPCS benefit from this sniff?
• MergeConflict

Also, what do you think about a Deprecated category?

[GaryJones]

Those all make sense.

AdminBarRemoval - @GaryJones What are your thoughts on a UserExperience category? Or is that too broad/may not be used enough?

If this might otherwise be the only thing left in VIP, then yes, let's move it to a UserExperience` category. None of the other current sniffs indicate they should be in this category, but I guess there might be future sniffs related to amending how Gutenberg works or something.

• ErrorControl - Would WPCS benefit from this sniff?

The only thing that does is check for a T_ASPERAND token, and reports an Error if one is found.

I think we can drop the sniff altogether, and add Generic.PHP.NoSilencedErrors to our ruleset instead. That gives a dynamic violation message (showing where the @ is used), and it defaults to a Warning by default (we could add a custom property if it needed to stay as an Error). I've opened #329.

MergeConflict

Not really VIP-specific, but once we get to PHPCS 3.4+, we can remove it (#330). In the meantime, we could use VersionControl category, the same as upstream does.

[GaryJones]

@rebeccahum Looking at the others:

1. Should Zoninator be moved from Plugins to Compatibility? Or leave it in case we get some other Plugin-related sniffs in the future? (Then again, anything they flag is likely to be with how that plugin is being used on the platform, therefore, Compatibility).
2. Move the three Cache category sniffs into Performance as well?
3. Move Actions.PreGetPosts and Filters.AlwaysReturn and Filters.RestrictedHook into a Hooks category (and pluralise that last one to RestrictedHooks)?
4. Should Constants.ConstantRestrictions be changed to Constants.RestrictedConstants in the same way that we're proposing having Functions.RestrictedFunctions and Variables.RestrictedVariables (and Hooks.RestrictedHooks)?
5. Should the TemplatingEngines/UnescapedOutput*Sniff.php be renamed to remove the UnescapedOutput bit, and move that into the violation code? i.e. WordPressVIPMinimum.TemplatingEngines.UnescapedOutputMustache.OutputNotation would become WordPressVIPMinimum.TemplatingEngines.Mustache.UnescapedOutput, etc.

[GaryJones]

Also, what do you think about a Deprecated category?

What would go in there?

[rebeccahum]

MergeConflict

Not really VIP-specific, but once we get to PHPCS 3.4+, we can remove it (#330). In the meantime, we could use VersionControl category, the same as upstream does.

This sounds good to me.

Should Zoninator be moved from Plugins to Compatibility? Or leave it in case we get some other Plugin-related sniffs in the future? (Then again, anything they flag is likely to be with how that plugin is being used on the platform, therefore, Compatibility).

I like the idea of moving it to Compatibility since that seems to be the only sniff in the Plugins folder currently.

Move the three Cache category sniffs into Performance as well?

Yes.

Move Actions.PreGetPosts and Filters.AlwaysReturn and Filters.RestrictedHook into a Hooks category (and pluralise that last one to RestrictedHooks)?

This makes sense to me instead of having two separate categories for it.

Should the TemplatingEngines/UnescapedOutput*Sniff.php be renamed to remove the UnescapedOutput bit, and move that into the violation code? i.e. WordPressVIPMinimum.TemplatingEngines.UnescapedOutputMustache.OutputNotation would become WordPressVIPMinimum.TemplatingEngines.Mustache.UnescapedOutput, etc.

Yes, I'm also thinking that we don't need a TemplatingEngines category and it can be moved into Security?

As for the Deprecated category, an example would be for things like WPCOMVIP helper functions. Another use case that could be possible would possibly be for flagging anything that was available on WPCOM but deprecated on VIP Go?