Adding WordPress-VIP-Go ruleset suppresses many warnings

[paulschreiber]

Bug Description

Adding the WordPress-VIP-Go ruleset suppresses many warning from WordPress-Core, including StrictComparisons.LooseComparison.

Sample file (test.php)

<?php
if ( 'edit.php' == $pagenow ) {
	echo 'yes';
}

Example ruleset:

<?xml version="1.0"?>
<ruleset name="Custom ruleset">
    <rule ref="WordPress-Extra"/>
    <rule ref="WordPress-VIP-Go" />
</ruleset>

Output:

$ vendor/bin/phpcs test.php 
W 1 / 1 (100%)

Example ruleset:

<?xml version="1.0"?>
<ruleset name="Custom ruleset">
    <rule ref="WordPress-Extra"/>
</ruleset>

Output:

$ vendor/bin/phpcs test.php 
W 1 / 1 (100%)

FILE: test.php
-----------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------
 2 | WARNING | Found: ==. Use strict comparisons (=== or !==).
   |         | (WordPress.PHP.StrictComparisons.LooseComparison)
-----------------------------------------------------------------------------------------------------------------

Time: 268ms; Memory: 8MB

Environment

Question	Answer
PHP version	7.3.3
PHP_CodeSniffer version	3.4.2
VIPCS version	2.0.0

Additional Context (optional)

The problem seems to be this commit: b8c5b76

Setting <severity> to 3 results in no output. Setting it to 5 results in the expected output.

[rebeccahum]

Using the example rule of StrictComparisons.LooseComparison mentioned, this is due to how the severity is a 3 on the WordPress-VIP-Go ruleset:

VIP-Coding-Standards/WordPress-VIP-Go/ruleset.xml

Lines 206 to 208 in b3e134c

	<rule ref="WordPress.PHP.StrictComparisons.LooseComparison">
	<severity>3</severity>
	</rule>

Per https://github.com/squizlabs/PHP_CodeSniffer/wiki/Configuration-Options#changing-the-default-severity-levels:

By default, PHP_CodeSniffer will show all errors and warnings with a severity level of 5 or greater.

So, anything below 5 isn't picked up by default and therefore, you'll need to pass in --severity=1 into the command in order to see all levels like so:

vendor/bin/phpcs test.php --severity=1

This will display everything that is of level 1 and above. Alternatively, you could configure your phpcs to always show everything that is 1 and above:

vendor/bin/phpcs --config-set severity 1

For more information on this configuration process, I recommend checking out the aforementioned documentation.

[maevelander]

Re-opening this and requesting a second pass from @GaryJones upon request via ticket.

[GaryJones]

@paulschreiber

(Aside: Your first "Output:" block indicates that the file gave a warning, but my testing, and your description suggests this isn't the case - I'm assuming this is copy-pasta.)

@rebeccahum's explanation of the current situation here is correct. The warnings aren't completely suppressed, but lowered in severity.

Somewhat repeating you'd found...

Warning when just WordPress-Extra is used:

➜  phpcs --standard=WordPress-Extra -sp phpcs-test.php
W 1 / 1 (100%)

FILE: phpcs-test.php
-----------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------
 2 | WARNING | Found: ==. Use strict comparisons (=== or !==).
   |         | (WordPress.PHP.StrictComparisons.LooseComparison)
-----------------------------------------------------------------------------------------------------------------

No warning when WordPress-VIP-Go added, at the default severity:

➜  phpcs --standard=WordPress-Extra,WordPress-VIP-Go -sp phpcs-test.php
. 1 / 1 (100%)

Warning reappears when severity is lowered:

➜  phpcs --standard=WordPress-Extra,WordPress-VIP-Go -sp phpcs-test.php --severity=3
W 1 / 1 (100%)

FILE: phpcs-test.php
-----------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------
 2 | WARNING | Found: ==. Use strict comparisons (=== or !==).
   |         | (WordPress.PHP.StrictComparisons.LooseComparison)
-----------------------------------------------------------------------------------------------------------------

The original intent, as I see it, was to consider a different use case to how you might be using it.

If a VIP client is just using the VIPCS standard, then it appears that the intent was to ensure that loose comparisons are flagged, albeit at a lower severity (this would show up for the VIP Go CI bot during PR reviews which uses --severity=1, but not for local scans).

The issue here is when someone is also using WPCS, without changing the --severity, and with all the above said, I think there you have a valid point here; if VIPCS is going to pull in Sniffs from WPCS, it shouldn't be lowering the standard - the checks in that sniff are either important for code on VIP (generalised as security, performance, platform compatibility), or it's not. In this case, loose comparisons might be a source of logic bugs, but it's not specifically related to the concerns of VIPCS, so I'm happy to review this loose comparisons entry.

Before we do, are there any other entries you, Paul, think fall into the same category i.e. WordPress.x.y[.z] sniffs / violations that have a severity less than 5 that should be considered as well? I think the choice would come from this code.

[paulschreiber]

It's unreasonable to expect every VIP client to (a) be aware of this (b) write a custom configuration file or pass in --severity=1.

The expected behaviour is that including the ruleset prints all warnings and errors (at the default severity level). Every rule that currently has a severity of < 5 should be increased to at least 5.

[GaryJones]

Thanks - we'll bear this in mind when we look into this more.

Related: Automattic/vip-go-ci#64

[paulschreiber]

@GaryJones Any update on this?

[GaryJones]

Not on this item specifically, but we've got a task underway that should allow someone time to focus on making improvements like this across all of VIPCS.

[Scotchester]

FWIW, I just stumbled on this after upgrading to VIPCS 3.0. No warnings were reported when running locally, but when I opened the PR, the VIP bot hit me with a WordPress.PHP.StrictComparisons.LooseComparison warning.

[jrfnl]

@Scotchester The reason for what you are seeing is that you are locally running on VIPCS 3.0, while the bot is still on VIPCS 2.3.4. The particular sniff was swopped to another sniff in VIPCS 3.0, so that explains why you are seeing inconsistent results between a local run and a bot run.
Your issue should be fixed when the bot switched to VIPCS 3.0 towards the end of the month.

[GaryJones]

@Scotchester The other difference is that the VIP Code Analysis bot runs with severity 1 by default. Running locally, I'm guessing you're not including a --severity= parameter, so you're running at severity 5. Some items in the WordPress-VIP-Go ruleset are set at severity 3, which means, with defaults, they will show up with the bot, but won't show up locally.

The VIP Code Analysis Bot is due to be updated on September 25th.