WordPress != WordPress-Core,WordPress-Docs,WordPress-Extra

[GaryJones]

After refreshing https://github.com/GaryJones/pwa-wp/tree/phpcs-refresh, no violations are reported when running phpcs. All good.

If I change:

<rule ref="WordPress-Core"/>
<rule ref="WordPress-Docs"/>
<rule ref="WordPress-Extra"/>

to:

<rule ref="WordPress"/>

Then new violations are reported:

FILE: wp-includes/class-wp-service-workers.php
-----------------------------------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------------------------------
 117 | ERROR | Missing wp_unslash() before sanitization. (WordPress.Security.ValidatedSanitizedInput.MissingUnslash)
 117 | ERROR | Detected usage of a non-sanitized input variable: $_SERVER
     |       | (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)
-----------------------------------------------------------------------------------------------------------------------------------------

Is that supposed to happen?

cc @westonruter

[jrfnl]

This is a known (non-)issue.

When we moved/renamed the original badge of sniffs as discussed in #1157, most were added to the Extra ruleset in #1261, save three for which more discussion/bug fixes were needed. The sniff you refer to is one of those three.

This was annotated in the PR at the time.

Note: There are three more candidates for this:

• DB.DirectDatabaseQuery
• DB.SlowDBQuery
• Security.ValidatedSanitizedInput

I've not included the first two (yet) as those may need some more discussion first. The third one is not included as it throws a lot of noise messages and has quite some open issues surrounding it, which should be addressed first IMO.

See #1261

[GaryJones]

Sorry @jrfnl, I don't follow.

Why does this violation appear when checking against WordPress, but not when checking against the three sub-standards together?

[jrfnl]

Why does this violation appear when checking against WordPress, but not when checking against the three sub-standards together?

Because the above mentioned three sniffs are not included in any of the three sub-standards.

The WordPress ruleset included the three sub-standards + every single sniff in the Sniffs directory, which is why these three will be included when you run against the WordPress standard.

The "every single sniff in the Sniffs directory" bit is default behaviour of PHPCS and is part of the difference between a custom ruleset and a standard.

• A standard can have a Sniffs sub-directory and any sniffs in that sub-directory will be automatically registered for the standard.
• A custom ruleset can include sniffs and configuration, but cannot add sniffs. The WordPress-... rulesets are basically custom rulesets (no Sniffs sub-directory), but as they are shipped within the WPCS package, they are registered as standards.

This is the same principle of why we were having so many issues with the deprecation warnings being thrown, even though the VIP standard was not being included.

[GaryJones]

So, at least while those three sniffs need discussion and are not included in Extra, it's best to stick with:

<rule ref="WordPress-Core"/>
<rule ref="WordPress-Docs"/>
<rule ref="WordPress-Extra"/>

in a project's ruleset?

(Or:

<rule ref="WordPress-Extra"/><!-- includes WordPress-Core -->
<rule ref="WordPress-Docs"/>

)

[jrfnl]

@GaryJones That depends on your intention with the custom ruleset.

The simple answer is that with regards to these three sniffs, the behaviour of the WordPress ruleset and the WordPress-Extra ruleset has not changed in 1.0.0. The sniffs were previously already included in the WordPress ruleset and have never been included in the WordPress-Extra ruleset. I.e.: no change.

So if you want the "same" behaviour as before (aside from all the other changes in 1.0.0), leave the ruleset as is, whether that is WordPress or a combination of sub-standards.

[GaryJones]

Understood, thank you.

Worth adding something to the readme, regarding explaining the above?

[jrfnl]

@GaryJones Did you still want to add a note about this to the Readme ? If so, please do. Otherwise, let's close this.

[GaryJones]

Is there any way of easily identifying which sniffs from the Sniffs directory are NOT part of one of the three rulesets?

[jrfnl]

@GaryJones Well, there's the list above. Other than that, I think it would be a case of doing a (manual) comparison between the output of these commands:

phpcs -e --standard=WordPress-Extra
phpcs -e --standard=WordPress

... where the focus would be on what's listed under the "WordPress" heading.

I mean, we know that there are no WPCS native sniffs in the Docs ruleset, so we don't need to take it into account. And the WordPress-Extra ruleset includes WordPress-Core, so comparing the output of the above two commands should give a conclusive answer.

I've just done so and the outcome confirms what we already knew:

  WordPress.DB.DirectDatabaseQuery
  WordPress.DB.SlowDBQuery
  WordPress.Security.ValidatedSanitizedInput
  WordPress.Utils.I18nTextDomainFixer

Note: WordPress.Utils.I18nTextDomainFixer is a sniff which shouldn't normally run anyway as it is a utility to switch textdomains, so that being there is correct and expected.

[GaryJones]

Are you intending to include the top three within a standard before 3.0.0?

[jrfnl]

They are included in the WordPress standard and AFAIC, that's as it should be as per https://github.com/WordPress/WordPress-Coding-Standards/issues/1440#issuecomment-408690186.

The decision not to include them in WordPress-Extra was previously taken to prevent a BC-break, even though the related changes were made at a major release.

I'm open to hearing a case being made for changing that now, but not something I'm terribly concerned about.