Proposal: treat deprecations/notices/warnings coming from sniffs in a more end-user friendly manner

[jrfnl]

Repost from squizlabs/PHP_CodeSniffer#3844:

Current situation

As things currently are, when a PHP deprecation/notice/warning/error is received during the scanning of a file, PHPCS turns this into an Internal.Exception and kills the scan of that particular file.

Example 1:

• A file containing 200 lines of code contains a parse error on line 100.
• One of the sniffs involved in the scan does not contain enough defensive coding, which leads to a PHP notice for the code on line 100.
• PHPCS will report the notice as an Internal.exception on line 1 of the file.
• PHPCS will show the scan results for line 1 - 99 of the file.
• No errors or warnings are reported for line 100 - 200 of the file.

Example 2:

• PHP makes a change which causes a deprecation notice in a pre-existing sniff.
• One of the files being scanned causes the sniff to hit the deprecation notice.
• PHPCS will report the notice as an Internal.exception on line 1 of the file.
• PHPCS will show the scan results for the file up to the point the code hit the deprecation notice.
• No errors or warnings are reported for the rest of the file.

The problem

While it is important for sniff authors to have access to this information (to allow them to fix the sniff), this is not user-friendly for end-users who cannot do anything with that information, other than report it to the relevant standard and in the mean time, their code is not fully scanned.

So... I'm wondering if we cannot find a way to make this more user-friendly for end-users, while still keeping the information available for sniff authors.

Important caveat: while the scan results for the "rest of the file" will be fine for PHP deprecations/notices/warnings caused by a logic oversight in the sniff or a change in PHP, if the file being scanned contains a parse error, chances are high that the scan results for the "rest of the file" are unreliable (at best) or even complete nonsense (at worst).

Proposal

I haven't thought this through completely yet, but these are some thoughts I have around this which I'm sharing to receive feedback on them.

Rough outline of proposal:

• Leave the existing behaviour in place for fatal errors.
• Collect all deprecations/notices/warnings seen during the scan, but don't abort scanning a file on those.
• Report the number of deprecations/notices/warnings encountered during a scan at the bottom of the output and exit with 1 if any were seen (same as before as previously the Internal.Exception would cause an exit code of 1).
• Report the full details of deprecations/notices/warnings encountered during a scan at the bottom of the output when running with -v and exit with 1 if any were seen (same as before).
• Add a new CLI argument --ignore-php-notices (or something along those lines) to allow the exit code to be 0 if there were deprecations/notices/warnings.
• In PHPCS 4.0.0, reverse the behaviour for the exit code, meaning PHP deprecations/notices/warnings will no longer cause an exit code of 1, but will exit with 0 instead. Replace the --ignore-php-notices CLI argument with a --fail-on-php-notices CLI argument to still get a 1 exit code when there are notices.

We also may need to make some accommodation in the test framework to allow test runs to still always report those deprecations/notices/warnings and fail a test run if any are encountered.

Thoughts ?

---

Issue squizlabs/PHP_CodeSniffer#2871 is related.

[mbomb007]

Relevant comment I posted on another issue: #184 (comment)

Personally, I would like errors and warnings to have separate exit codes, so that a coding standards error returns 1, whereas warnings with no errors returns 2 or something similar. Otherwise you have to grep the output to determine what type of error it is.

This would be helpful for using PHPCS in GitLab pipelines, because exit code can be used to determine whether a pipeline is allowed to fail.

The argument to allow a zero exit code for warnings is nice, but that's not the same functionality, as then there's no distinction between warnings and a success. GitLab allows pipelines to specify essentially a "pass with warning" state, and the pipeline stage shows as yellow (warning) instead of green (success) or red (error). I'd like an easy way to determine that from the exit code, such as 0 (success), 1 (error), 2 (warnings, no errors).

[jrfnl]

@mbomb007 This ticket is not directly related to exit codes. It is about how to handle (some of the) "Internal" errors.. The current handling has a side-effect on the exit code and this ticket is about removing that side-effect.

This ticket is not related to the errors/warnings about issues found by sniffs.

With that in mind, I am inclined to hide your comment as irrelevant for this ticket.