Skip to content

Commit

Permalink
Don't stop scan on invalid inline property annotation
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jrfnl committed Oct 31, 2023
1 parent 321d252 commit 38924fb
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 2 deletions.
18 changes: 17 additions & 1 deletion src/Files/File.php
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,7 @@ public function process()
$listenerIgnoreTo = [];
$inTests = defined('PHP_CODESNIFFER_IN_TESTS');
$checkAnnotations = $this->config->annotations;
$annotationErrors = [];

// Foreach of the listeners that have registered to listen for this
// token, get them to process it.
Expand Down Expand Up @@ -411,7 +412,15 @@ public function process()
'scope' => 'sniff',
];
$listenerClass = $this->ruleset->sniffCodes[$listenerCode];
$this->ruleset->setSniffProperty($listenerClass, $propertyCode, $settings);
try {
$this->ruleset->setSniffProperty($listenerClass, $propertyCode, $settings);
} catch (RuntimeException $e) {
// Non-existant property being set via an inline annotation.
// This is typically a PHPCS test case file, but we can't throw an error on the annotation
// line as it would get ignored. We also don't want this error to block
// the scan of the current file, so collect these and throw later.
$annotationErrors[] = 'Line '.$token['line'].': '.str_replace('Ruleset invalid. ', '', $e->getMessage());
}
}
}
}//end if
Expand Down Expand Up @@ -536,6 +545,13 @@ public function process()
}
}

if ($annotationErrors !== []) {
$error = 'Encountered invalid inline phpcs:set annotations. Found:'.PHP_EOL;
$error .= implode(PHP_EOL, $annotationErrors);

$this->addWarning($error, null, 'Internal.PropertyDoesNotExist');
}

if (PHP_CODESNIFFER_VERBOSITY > 2) {
echo "\t*** END TOKEN PROCESSING ***".PHP_EOL;
echo "\t*** START SNIFF PROCESSING REPORT ***".PHP_EOL;
Expand Down
7 changes: 7 additions & 0 deletions src/Standards/Generic/Tests/PHP/BacktickOperatorUnitTest.inc
Original file line number Diff line number Diff line change
@@ -1,2 +1,9 @@
<?php
$output = `ls -al`;

// Testing an invalid phpcs:set annotations.
// This test is unrelated to this sniff, but the issue needs a sniff to be tested.
// phpcs:set Generic.PHP.BacktickOperator unknown 10

// Make sure errors after an incorrect annotation are still being thrown.
$output = `ls -al`;
6 changes: 5 additions & 1 deletion src/Standards/Generic/Tests/PHP/BacktickOperatorUnitTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,10 @@ class BacktickOperatorUnitTest extends AbstractSniffUnitTest
*/
public function getErrorList()
{
return [2 => 2];
return [
2 => 2,
9 => 2,
];

}//end getErrorList()

Expand All @@ -40,6 +43,7 @@ public function getErrorList()
*/
public function getWarningList()
{
// Warning about incorrect annotation will be shown on line 1 once PR #3915 would be merged.
return [];

}//end getWarningList()
Expand Down

0 comments on commit 38924fb

Please sign in to comment.