VIP-Coding-Standards
VIP-Coding-Standards copied to clipboard
Automattic
Adding WordPress-VIP-Go ruleset suppresses many warnings
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: https://github.com/Automattic/VIP-Coding-Standards/commit/b8c5b767e96d32bbfa096a2d9e07f7db382737d9
Setting <severity> to 3 results in no output. Setting it to 5 results in the expected output.
Using the example rule of StrictComparisons.LooseComparison mentioned, this is due to how the severity is a 3 on the WordPress-VIP-Go ruleset: https://github.com/Automattic/VIP-Coding-Standards/blob/b3e134c6b952ddb4095777f5a1b682b1a40da5d4/WordPress-VIP-Go/ruleset.xml#L206-L208
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.
Re-opening this and requesting a second pass from @GaryJones upon request via ticket.
@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.
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.
Thanks - we'll bear this in mind when we look into this more.
Related: https://github.com/Automattic/vip-go-ci/issues/64
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.
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.
@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.
@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.
@GaryJones Yeah, that's the part that I was referring to. I thought the ticket was reopened and remains so on account of your comments, such as
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.
and the ensuing discussion, so I was just hoping to politely nudge that it's still an issue :)
I'm a little unclear on how the VIP bot being updated will change things. The severity discrepancy will still exist, right?
You're right in that this is still a bit messy, and catches folks out. Still an open issue. Part of it comes down to intent. If someone is running a WordPressCS ruleset directly, then they may want to be told of the items that are currently marked as lower severity in VIPCS. But VIP isn't as concerned with them, hence the lower severity if someone runs a VIP ruleset.
I'm a little unclear on how the VIP bot being updated will change things. The severity discrepancy will still exist, right?
The bot will use VIPCS 3, which means it also uses WordPressCS 3.0, and through their use of PHPCSUtils there are a lot more support for modern PHP 8.x syntax, meaning few false positives and false negatives, which means a higher percentage of actionable violations.
Yes, the severity discrepancy will still exist. Updating those may be classed as a breaking change, so it would need to be addressed in VIPCS 4. It wasn't tackled in VIPCS 3 because we wanted to focus on getting WordPressCS 3 compatibility so that folks could get the benefit of WordPressCS 3 as soon as possible.
Got it. Thank you both!
Part of it comes down to intent. If someone is running a WordPressCS ruleset directly, then they may want to be told of the items that are currently marked as lower severity in VIPCS.
Yeah, this was my thought process. I'd like to know before opening a PR what the VIP bot will comment on, even if it's not severe.
Perhaps an easy solution (at least for VIP customers) would just be tweaking the WordPress VIP Skeleton Application README to explain this nuance and suggest running it with --severity=1 (and --config-set ignore_warnings_on_exit 1 to print warnings but not fail when running in CI)?