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?