squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.67k stars 1.48k forks source link

Warnings and some errors are not shown when --report=diff #3583

Closed fogush closed 2 years ago

fogush commented 2 years ago

PHP_CodeSniffer can produce warnings, e.g. for too long lines. For the following test.php file:

<?php

echo "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.";

The default report format will show the warning and fail the check:

> vendor/bin/phpcs test.php

FILE: .../test.php
----------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
----------------------------------------------------------------------
 2 | ERROR   | Missing file doc comment
 3 | WARNING | Line exceeds 85 characters; contains 131 characters
----------------------------------------------------------------------

Time: 145ms; Memory: 6MB
> echo $?                                                                                
1

But --report=diff won't show the warnings and won't fail the check

> vendor/bin/phpcs --report=diff scripts/test.php
> echo $?                                                                                
0

Also, you may notice the "Missing file doc comment" error is not thrown with --report=diff just because there is no "diff" to show.

Is this by design?

> vendor/bin/phpcs --version             
PHP_CodeSniffer version 3.6.2 (stable) by Squiz (http://www.squiz.net)
> php -v                               
PHP 8.1.2 (cli) (built: Jan 21 2022 04:34:01) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.2, Copyright (c) Zend Technologies
    with Zend OPcache v8.1.2, Copyright (c), by Zend Technologies
jrfnl commented 2 years ago

Is this by design?

Yes. The diff report basically shows the patch which would be applied when the fixer is run and neither of the above errors you showed are auto-fixable errors. (Fixable errors are indicated in the normal report with an [X] before the error/warning).

I do wonder a little about the error code being 0 being correct, as a phpcs run with a different report would return 1, but then again a run with phpcbf on the file would return 0, so it is consistent with that.

gsherwood commented 2 years ago

I do wonder a little about the error code being 0 being correct, as a phpcs run with a different report would return 1, but then again a run with phpcbf on the file would return 0, so it is consistent with that.

The diff report is different to the others as it only looks at fixable errors (like PHPCBF) and so it doesn't record any errors on the file as none are fixable. So during that specific run of PHPCS, no errors are recorded and you get a 0 exit code. That basically says there is no work to do.

This is less a design decision around the diff report and more a design decision around PHPCBF, which the diff reports hooks into.

If you generated multiple reports (including a diff report) you'll get a non-0 exit code as all errors need to be recorded so the other report types can display them. An example would be using --report=diff,summary.

Hope that explains why this works the way it does. I'll close this to indicate I'm not planning any changes to this behaviour.