Don't stop scan on invalid inline property annotation #3916
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Follow up on 3629, which was merged for PHPCS 3.8.0. PR 3629 added logic to throw a "Ruleset invalid. Property \"$propertyName\" does not exist on sniff ..." error. This error is intended for the command-line when reading the `phpcs.xml.dist` ruleset file. However, this error could _also_ be encountered if an inline `// phpcs:set ...` annotation would try to set a non-existent property. While the use of `// phpcs:set` is typically reserved for sniff test case files, there is nothing stopping end-users from using the annotation. The net-effect would be: * The `Ruleset::setSniffProperty()` throws a `RuntimeException`. * This exception is then passed to `File::addMessage()` where it is **not** thrown as the line on which the error is being thrown is an annotation line. * The scan of the file stops dead in its tracks as a `RuntimeException` was encountered. * The end-user doesn't know the file does not finish scanning as no `Internal` error is shown for the file. To me, this is counter-intuitive and counter-productive as it may give people a false sense of security (CI is green, while in reality files are not being scanned). To fix this, I propose the following: * Collect all `// phpcs:set` related inline annotations encountered while scanning. * Do **not** stop the file scan for these errors. * Add a warning with information about the incorrect annotations on line 1 once the file has finished scanning. Includes a test via the `Generic.PHP.BacktickOperator` sniff.
1 task
|
Closing as replaced by PHPCSStandards/PHP_CodeSniffer#99 |
jrfnl
deleted the
feature/3629-dont-stop-scan-on-invalid-property-set-annotation
branch
|
FYI: this fix is included in today's PHP_CodeSniffer 3.8.0 release. As per #3932, development on PHP_CodeSniffer will continue in the PHPCSStandards/PHP_CodeSniffer repository. If you want to stay informed, you may want to start "watching" that repo (or watching releases from that repo). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Follow up on #3629, which was merged for PHPCS 3.8.0.
PR #3629 added logic to throw a "Ruleset invalid. Property "$propertyName" does not exist on sniff ..." error.
This error is intended for the command-line when reading the
phpcs.xml.distruleset file.However, this error could also be encountered if an inline
// phpcs:set ...annotation would try to set a non-existent property.While the use of
// phpcs:setis typically reserved for sniff test case files, there is nothing stopping end-users from using the annotation.The net-effect would be:
Ruleset::setSniffProperty()throws aRuntimeException.File::addMessage()where it is not thrown as the line on which the error is being thrown is an annotation line.RuntimeExceptionwas encountered.Internalerror is shown for the file.To me, this is counter-intuitive and counter-productive as it may give people a false sense of security (CI is green, while in reality files are not being scanned).
To fix this, I propose the following:
// phpcs:setrelated inline annotations encountered while scanning.Includes a test via the
Generic.PHP.BacktickOperatorsniff.Suggested changelog entry
I'd suggest updating the existing changelog entry for the change from PR #3629 and adding:
Types of changes