Ruleset: improve error handling

[jrfnl]

Description

👉🏻 This PR has been a while in the making. While I'm confident about the principle of it, I've been struggling to write up the implementation choices made, which normally means there is still something not completely "right".
So under the guise of "Perfect is the enemy of good", here goes anyway (as this PR is blocking everything else I'm working on, as well as blocking the 3.12.0 release).

So please consider this a first iteration. The actual implementation details can be iterated on both in this PR, as well as via future PRs.

---

This PR is intended to solve two problems for the Ruleset class and to allow for implementing the same kind of solution in other parts of the codebase at a later point in time.

Current Situation

Up to now, the Ruleset class only allowed for hard errors which would exit the run immediately with a non-zero exit code.

Problem 1: The Ruleset class only allows for hard errors

In issue 689, the need to be able to display deprecation notices when processing rulesets was identified.
These deprecation notices should be displayed in a user-friendly manner and should not affect the exit code of PHPCS, nor stop PHPCS from continuing its run.

Problem 2: The Ruleset class provides errors to the user one by one

If a Ruleset contains multiple errors, running PHPCS would only display one error at a time, which means the user would have to:

• run PHPCS
• see error 1, fix error 1
• run PHPCS again
• find out there is a second error, fix error 2
• run PHPCS again... etc

This pattern of showing errors piecemeal (one-by-one) is not terribly user-friendly, so the intention is to change this behaviour.

What this PR changes

This PR introduces a new MsgCollector ++MessageCollector++ class which is intended to fix both problems. Errors and notices are "written to" the MsgCollector ++MessageCollector++ while processing the Ruleset and only displayed when the Ruleset has finished its processing.

If there are errors amongst the collected messages, PHPCS will still stop and exit with a non-zero exit code at that point.
If only warnings/notices/deprecations were collected, PHPCS will display these and then continue onto the next step in the PHPCS process.

👉 Note: At this time, no changes are being made to the error "level" of the pre-existing errors coming from the Ruleset class.

Also note that the new MsgCollector ++MessageCollector++ class is marked as a PHPCS internal class and has no BC-promise to allow for iterating on the solution.

Future Scope

At a later point in time, this same principle can be applied in more places.

In particular I'm thinking of:

• The Config class (in conjunction with Config: read all CLI args before doing a deep exit #448). This would then also allow for deprecation of CLI args if/when needed.
• The Fixer class to fix issue Runtime exception originating from sniff fixers are not handled squizlabs/PHP_CodeSniffer#2871 (PHP errors encountered during fixing are not reported leading to potentially incomplete results without the user being informed).

Both of these improvements are currently blocked by the lack of tests for those parts of the codebase.

Implementation choices made for this PR

A this time, the Ruleset class throws the following errors:

// Errors in the ruleset.
throw new RuntimeException('ERROR: No sniffs were registered');
throw new RuntimeException("ERROR: Ruleset $rulesetPath is not valid ...[XML error details]");
throw new RuntimeException('ERROR: The specified autoload file "'.$autoload.'" does not exist');
throw new RuntimeException("ERROR: Referenced sniff \"$ref\" does not exist");
throw new RuntimeException("ERROR: Message type \"$type\" is invalid; must be \"error\" or \"warning\"");
throw new RuntimeException("ERROR: Sniff $sniffClass register() method must return an array");
throw new RuntimeException("ERROR: Ruleset invalid. Property \"$propertyName\" does not exist on sniff ...");

// Sniff developer errors (mostly type checks related to the `DeprecatedSniff` interface).
throw new RuntimeException(sprintf($errorTemplate, $className, 'getDeprecationVersion', 'non-empty ', gettype($deprecatedSince)));
throw new RuntimeException(sprintf($errorTemplate, $className, 'getDeprecationVersion', 'non-empty ', '""'));
throw new RuntimeException(sprintf($errorTemplate, $className, 'getRemovalVersion', 'non-empty ', gettype($removedIn)));
throw new RuntimeException(sprintf($errorTemplate, $className, 'getRemovalVersion', 'non-empty ', '""'));
throw new RuntimeException(sprintf($errorTemplate, $className, 'getDeprecationMessage', '', gettype($customMessage)));
trigger_error(__FUNCTION__.': the format of the $settings parameter has changed from (mixed) $value to array(\'scope\' => \'sniff|standard\', \'value\' => $value). Please update your integration code. See PR #3629 for more information.', E_USER_DEPRECATED);

Details about the errors thrown from the `Ruleset` class (with context)

241:         if ($numSniffs === 0) {
242:             throw new RuntimeException('ERROR: No sniffs were registered');
243:         }

377:             // Verify the interface was implemented correctly.
378:             // Unfortunately can't be safeguarded via type declarations yet.
379:             $deprecatedSince = $this->sniffs[$className]->getDeprecationVersion();
380:             if (is_string($deprecatedSince) === false) {
381:                 throw new RuntimeException(
382:                     sprintf($errorTemplate, $className, 'getDeprecationVersion', 'non-empty ', gettype($deprecatedSince))
383:                 );
384:             }
385:
386:             if ($deprecatedSince === '') {
387:                 throw new RuntimeException(
388:                     sprintf($errorTemplate, $className, 'getDeprecationVersion', 'non-empty ', '""')
389:                 );
390:             }
391:
392:             $removedIn = $this->sniffs[$className]->getRemovalVersion();
393:             if (is_string($removedIn) === false) {
394:                 throw new RuntimeException(
395:                     sprintf($errorTemplate, $className, 'getRemovalVersion', 'non-empty ', gettype($removedIn))
396:                 );
397:             }
398:
399:             if ($removedIn === '') {
400:                 throw new RuntimeException(
401:                     sprintf($errorTemplate, $className, 'getRemovalVersion', 'non-empty ', '""')
402:                 );
403:             }
404:
405:             $customMessage = $this->sniffs[$className]->getDeprecationMessage();
406:             if (is_string($customMessage) === false) {
407:                 throw new RuntimeException(
408:                     sprintf($errorTemplate, $className, 'getDeprecationMessage', '', gettype($customMessage))
409:                 );
410:             }
411:

487:         $ruleset = simplexml_load_string(file_get_contents($rulesetPath));
488:         if ($ruleset === false) {
489:             $errorMsg = "ERROR: Ruleset $rulesetPath is not valid".PHP_EOL;
490:             $errors   = libxml_get_errors();
491:             foreach ($errors as $error) {
492:                 $errorMsg .= '- On line '.$error->line.', column '.$error->column.': '.$error->message;
493:             }
494:
495:             libxml_clear_errors();
496:             throw new RuntimeException($errorMsg);
497:         }

532:             } else if (is_file($autoloadPath) === false) {
533:                 throw new RuntimeException('ERROR: The specified autoload file "'.$autoload.'" does not exist');
534:             }

995:             if (is_file($ref) === false) {
996:                 $error = "ERROR: Referenced sniff \"$ref\" does not exist";
997:                 throw new RuntimeException($error);
998:             }

1085:                 if ($type !== 'error' && $type !== 'warning') {
1086:                     throw new RuntimeException("ERROR: Message type \"$type\" is invalid; must be \"error\" or \"warning\"");
1087:                 }

1414:             if (is_array($tokens) === false) {
1415:                 $msg = "ERROR: Sniff $sniffClass register() method must return an array";
1416:                 throw new RuntimeException($msg);
1417:             }

1509:             trigger_error(
1510:                 __FUNCTION__.': the format of the $settings parameter has changed from (mixed) $value to array(\'scope\' => \'sniff|standard\', \'value\' => $value). Please update your integration code. See PR #3629 for more information.',
1511:                 E_USER_DEPRECATED
1512:             );

1525:             if ($settings['scope'] === 'sniff') {
1526:                 $notice  = "ERROR: Ruleset invalid. Property \"$propertyName\" does not exist on sniff ";
1527:                 $notice .= array_search($sniffClass, $this->sniffCodes, true);
1528:                 throw new RuntimeException($notice);
1529:             }

I've categorized these errors in the above list as "ruleset errors" and "sniff developer errors".

For now, I'm going to leave the sniff developer errors alone. An end-user should never run into these and we may want to have a think about how to ensure that even better, but for now, in my opinion, that is outside of the scope of this ticket.

So that leaves us with the following errors:

throw new RuntimeException('ERROR: No sniffs were registered');
throw new RuntimeException("ERROR: Ruleset $rulesetPath is not valid ...[XML error details]");
throw new RuntimeException('ERROR: The specified autoload file "'.$autoload.'" does not exist');
throw new RuntimeException("ERROR: Referenced sniff \"$ref\" does not exist");
throw new RuntimeException("ERROR: Message type \"$type\" is invalid; must be \"error\" or \"warning\"");
throw new RuntimeException("ERROR: Sniff $sniffClass register() method must return an array");
throw new RuntimeException("ERROR: Ruleset invalid. Property \"$propertyName\" does not exist on sniff ...");

While I'd like to collect ALL of these errors, these is one for which an exception MUST be made:

throw new RuntimeException('ERROR: The specified autoload file "'.$autoload.'" does not exist');

A missing autoload file may or may not be problematic in the context of the MsgCollector ++MessageCollector++, depending on whether whichever classes are supposed to be loaded via the custom autoloader are actually used by the sniffs being loaded in the ruleset and when.

Let me explain. There are basically three possible situations:

1. The autoload file is requested by a (sub-)ruleset, but the sniffs in use by the ruleset do not use any classes which need to be loaded by the custom autoload file. In that case, the missing autoload file is not problematic (at all).
2. The autoload file is requested by a (sub-)ruleset and there are sniffs in use by the ruleset which use classes which need to be loaded by the custom autoload file, but these are only used within a process() method and the sniff may bow out before ever reaching the code which calls the class which needs autoloading. In that case, it depends on the "code under scan" whether the missing autoload file would be problematic.
3. The autoload file is requested by a (sub-)ruleset and there are sniffs in use by the ruleset which use classes which need to be loaded by the custom autoload file and these classes are in the class signature of the sniff class, like extends Something or implements Something.
   In that case, loading the sniff file while reading the ruleset (as done in Ruleset::registerSniffs() line 1337 - $className = Autoload::loadFile($file);), would lead to a "Class not found" error and this error can only be caught since PHP 7.3, which means there is no way we can catch this error in a PHP cross-version compatible manner for all currently supported PHP versions.

So, it is this last case which is the blocker. If the "autoload file missing" error would be collected via the MsgCollector ++MessageCollector++, any subsequent "Class not found" errors, encountered while processing the rulesets, would need to be handled and passed off to the MsgCollector ++MessageCollector++ as well, but we can't currently do that for all supported PHP versions, which means we can't "collect" the "autoload file missing" error.

And if we're going to make exceptions anyway, I'm inclined to not "collect" the "ruleset XML parse error" error either.
While we could "collect" it, ignore the problematic (sub-)ruleset and continue processing other rulesets, I kind of consider an XML parse error serious enough to error straight away.
👉 Note: I'm open to being persuaded to "collect" this error anyway.

Alternative to not "collecting" these errors

As an alternative, I considered "collecting" those errors anyway and adding an extra severity level (CRITICAL) to the MsgCollector ++MessageCollector++.
In that case, if a message with severity CRITICAL would be added, the MsgCollector ++MessageCollector++ would display() whatever had been collected straight away.

The upside of this is that any messages collected prior to the CRITICAL error would also be displayed (instead of getting lost/displayed only in the next run after the critical error was fixed).
The downside of this is that the MsgCollector ++MessageCollector++ would be less context agnostic (less "clean") and would contain logic deciding when to display the messages instead of leaving that to the class using the MsgCollector ++MessageCollector++.

For this first iteration, I've decided against this alternative solution, but as the MsgCollector ++MessageCollector++ is marked as "internal" and has no BC promise, this change could still be made at a later time.

Commits

Note: the last five (six) commits should be squashed into a single commit before merging. They are currently separate commits only to make it more straight-forward to review this PR.

✨ New MsgCollector ++MessageCollector++ utility class

This internal-use only PHP_CodeSniffer\Util\MessageCollector class can be used to collect various type of "error" messages, including notices, warnings and deprecations for display at a later point in time.

This class is intended to be used by various steps in the PHPCS process flow to improve the error handling and prevent the "one error hiding another" pattern.

Design choices:

• This class does not have a constructor and has no awareness of the applicable Config or other aspects of a PHPCS run.
  If notices should be displayed conditionally, like only when not in -q quite mode, this should be handled by the class using the MsgCollector ++MessageCollector++.
• While duplicate error messages are not a great user experience, this class allows for them.
  It is the responsibility of the class using the MsgCollector ++MessageCollector++ to ensure that the messages cached are informative for the end-user.
• The class provides a number of (public) class constants to indicate the error level when adding messages.
  It is strongly recommended to only ever use these class constants for the error levels.
  The default error level for messages will be NOTICE.
  Note: yes, these class constants should basically be an Enum, however, enums are a PHP 8.1+ feature, so can't be used at this time.
• The class provides two (public) "orderby" class constants to indicate the display order of the messages.
  It is strongly recommended to only ever use these class constants for the display order.
  By default, messages will be ordered by severity (with "order received" being the secondary ordering for messages of the same severity).
  Note: again, yes, these class constants should basically be an Enum.
• When display of the messages is requested and the message cache includes messages with severity ERROR, the class will throw a RuntimeException, which will cause the PHPCS run to exit with a non-zero exit code (currently 3).
• In all other cases, the messages will be displayed, but the PHPCS run will continue without affecting the exit code.
• It was suggested by @fredden to consider implementing PSR-3: Logger Interface for the MsgCollector ++MessageCollector++.
  While we discussed this, I don't consider this necessary at this time, though as the class has no BC-promise, this can be reconsidered in the future.
  Arguments against implementing PSR-3 at this time:
  • Placeholder handling is required in the PSR-3 implementation to allow for logging to different contexts and using appropriate variable escaping based on the context.
    For PHPCS, messages will only ever be displayed on the command-line, so there is no need to take escaping for different contexts into account.
  • PSR-3 expects handling of eight different severity levels - emergency, alert, critical, error, warning, notice, info, debug -.
    For PHPCS, the emergency, alert and critical levels, as defined in PSR-3, are redundant as these will never occur.
    Along the same line, PHPCS does not log info.
    However, we do want to be able to display deprecation notices, but that is a severity level not supported under PSR-3.

The new functionality is fully covered by tests.

Potential future scope:

• Add a constructor and inject the Config class to allow for colorizing the messages/message prefixes if color support is enabled.
  This would be most effective after issue Config: read all CLI args before doing a deep exit #448 has been addressed first.
• Add a new CRITICAL level for errors which prohibit the current task from finishing. When an error with such level would be received, it would cause the class to display all messages collected up to that point straight away via an exception.
• If the conditional displaying of the messages, like only when not in -q mode, would lead to undue duplicate code, potentially move display conditions handling into the MsgCollector ++MessageCollector++ class.
  This would also need the aforementioned constructor with Config injection.

Ruleset: wire in MsgCollector ++MessageCollector++

This commit adds a mechanism to handle errors encountered while loading a ruleset in a more user-friendly manner.

• Errors and notices aimed at end-users and ruleset maintainers will be collected while processing the ruleset.
• Only once the complete processing of the ruleset is finished, will all errors/notices be displayed.
  This prevents "one error hiding behind another".
• If there are only notices (deprecations/notices/warnings), these will not display when running -e (explain), -q (quiet mode) or --generator=... (documentation).
• Errors will always display and will hard exit out of the run with a non-zero exit code.

This implementation should be seen as an interim - "good enough for now" - solution, which can be iterated on in the future.
I can imagine a more code-base wide solution at some later point in time, but that should not block this initial improvement.
As the current implementation doesn't change the public API (the only new methods are private), it should be feasible to transform the current solution to whatever form a future solution will take without breaking changes.

Includes tests covering the new functionality.

Ruleset: use MsgCollector ++MessageCollector++ for "no sniffs registered" error

Ruleset: use MsgCollector ++MessageCollector++ for "referenced sniff not found" error

Ruleset: use MsgCollector ++MessageCollector++ for "invalid type" message

Includes updating the message to mention the reference for which the type was (incorrectly) being changed.
This should make it more straight forward for ruleset maintainers to find the problem in their ruleset.
It also makes the message more unique, as this message could occur in multiple places in a ruleset and there was no indication of that in the message previously.

👉🏻 This commit will be easiest to review while ignoring whitespace differences.

Potential future scope

It could be considered to downgrade this message from an ERROR to a NOTICE as an invalid type is not blocking for running the sniffs, though this could lead to results not being as expected if, for instance, the -n flag is being used, which is why I've not changed this at this time.

Ruleset: use MsgCollector ++MessageCollector++ for "register() method must return an array" error

Includes some new assertions which won't run until the test suite supports PHPUnit 10+ (PHPCS 4.0). These tests belong with this commit though, so adding them now anyway.

Ruleset: use MsgCollector ++MessageCollector++ for "setting non-existent property" error

Includes minor adjustment to the error message (removal of "Ruleset invalid" and punctuation).

Suggested changelog entry

Whenever possible, the ruleset processing will be allowed to run to its conclusion before displaying all ruleset errors in one go.
Previously an error in a ruleset would cause PHPCS to exit immediately.

Related issues/external references

Related to #689 (comment)

Types of changes

• New feature (non-breaking change which adds functionality)

[fredden]

Do we need to set this "back" to accessible=false? The object ($reflMethod) will be cease to exist immediately after this line of code.

Suggested change

$reflMethod->setAccessible(false);

[jrfnl]

Force of habit just in case ;-)

[rodrigoprimo]

Thanks for working on this, @jrfnl. This will be a great improvement for the user experience.

Overall, it looks good to me, and it works as expected. I left a few comments with questions and minor suggestions.

[rodrigoprimo]

Thanks for checking my suggestions, @jrfnl. Everything looks good to me now.

[jrfnl]

As there have not been any new comments for two days, I'm going to rebase this PR and squash the "fix up" commits into their originating commits. Once the build has passed again, I'll be merging this PR.