Rulesets: lower severity instead of excluding checks

[jrfnl]

As discussed in #1157 with conclusion written up in #1157 (comment) .

Hard excluding individual PHPCS rules makes it impossible for a custom ruleset to re-enable a rule.
By changing the severity instead, a custom ruleset can up the severity of a check to re-enable it.

Note: whether this will work or not depends on the loading order of rules. No guarantees given.
The fact that it may not work in certain circumstances, is something which should be addressed upstream.

[GaryJones]

So:

<!-- in WordPress-Core/ruleset.xml -->
<rule ref="PSR2">
    <exclude name="PSR2.ControlStructures.SwitchDeclaration.NotLower">
</rule>

and

<!-- in MyPlugin/.phpcs.xml.dist -->
<rule ref="WordPress"/>
<rule ref="PSR2.ControlStructures.SwitchDeclaration.NotLower"/>

wouldn't work (as in, use the whole of PSR2)?

[jrfnl]

@GaryJones Correct. Test it if you like.

Once this PR is merged, the below, however, should start to work:

<!-- in MyPlugin/.phpcs.xml.dist -->
<rule ref="WordPress"/>
<rule ref="PSR2.ControlStructures.SwitchDeclaration.NotLower">
    <severity>5</severity>
</rule>

[GaryJones]

Test it if you like.

Setup

PHPCS 3.2.0

My test file:

<?php

$var = 'string';

switch ($var) {
    case 'value': break;
}

The relevant bit of my .phpcs.xml.dist:

<rule ref="PSR2.ControlStructures">
    <exclude name="PSR2.ControlStructures.SwitchDeclaration.BodyOnNextLineCASE"/>
    <exclude name="PSR2.ControlStructures.SwitchDeclaration.BreakNotNewLine"/>
    <exclude name="Squiz.WhiteSpace.ScopeClosingBrace.ContentBefore"/>
</rule>

Control

With the three excludes, phpcs passes.

Test

Override the first exclusion by adding a change in severity afterwards. Relevant part of .phpcs.xml.dist is now:

<rule ref="PSR2.ControlStructures">
	<exclude name="PSR2.ControlStructures.SwitchDeclaration.BodyOnNextLineCASE"/>
	<exclude name="PSR2.ControlStructures.SwitchDeclaration.BreakNotNewLine"/>
	<exclude name="Squiz.WhiteSpace.ScopeClosingBrace.ContentBefore"/>
</rule>
<rule ref="PSR2.ControlStructures.SwitchDeclaration.BodyOnNextLineCASE">
	<severity>5</severity>
</rule>

The result is that the BodyOnNextLineCASE message appears as expected:

Hypothesis

exclude is the same as setting the severity to 0, at least in PHPCS 3.2.

Evidence

Running phpcs -vv includes the following near the top:

Conclusion

I'm not sure if the exclude being the same as severity 0 goes all the way back to our earliest supported version of PHPCS, but as things stand, I don't see that the changes here make any functional difference to the ability for anyone else consuming the ruleset to be able to be able to un-exclude error codes by changing the severity.

There's a potential argument regarding giving readers of our code a hint as to what they need to do to un-exclude an error code, but equally there's an argument for the exclude being easier to understand for someone who doesn't know (or care) about severity levels.

One suggestion (assuming my conclusion is valid): Keep the excludes, but add a comment along the lines of:

<!-- Excluded error codes can be un-excluded by changing the severity level. See https://... for more information. -->

[jrfnl]

@GaryJones Your conclusion is correct.

Including a sniff with <rule ref="..."/> will not work once the severity has been lowered to 0 (either by excluding the sniff or by changing the severity). Upping the severity will work.

Changing the exclusions to severity changes in the ruleset will make it much more intuitive and discoverable to users of custom rulesets how to overrule the exclude.

[JDGrimes]

So, if I'm understanding this right:

• exclusions set the severity to 0.
• but inclusions do not modify severity.

So if a sniff has been excluded, including it again won't cause it to give an error, because the severity will still be 0.

But by explicitly changing the severity to 0, instead of using the exclude shorthand, we make it more obvious how to re-enable the sniff. Inclusions still won't work, but changing the severity to something else will.

So @GaryJones example that doesn't work with the new change, also doesn't work now.

I see no downside to this change. It might be nice if PHPCS improves this in the future, but until then I still think this change makes sense.