Skip to content

Commit a741e9e

Browse files
committed
Don't stop scan on invalid inline property annotation
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 parent 5ef2b66 commit a741e9e

File tree

3 files changed

+29
-2
lines changed

3 files changed

+29
-2
lines changed

src/Files/File.php

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -343,6 +343,7 @@ public function process()
343343
$listenerIgnoreTo = [];
344344
$inTests = defined('PHP_CODESNIFFER_IN_TESTS');
345345
$checkAnnotations = $this->config->annotations;
346+
$annotationErrors = [];
346347

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

548+
if ($annotationErrors !== []) {
549+
$error = 'Encountered invalid inline phpcs:set annotations. Found:'.PHP_EOL;
550+
$error .= implode(PHP_EOL, $annotationErrors);
551+
552+
$this->addWarning($error, null, 'Internal.PropertyDoesNotExist');
553+
}
554+
539555
if (PHP_CODESNIFFER_VERBOSITY > 2) {
540556
echo "\t*** END TOKEN PROCESSING ***".PHP_EOL;
541557
echo "\t*** START SNIFF PROCESSING REPORT ***".PHP_EOL;
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,9 @@
11
<?php
22
$output = `ls -al`;
3+
4+
// Testing an invalid phpcs:set annotations.
5+
// This test is unrelated to this sniff, but the issue needs a sniff to be tested.
6+
// phpcs:set Generic.PHP.BacktickOperator unknown 10
7+
8+
// Make sure errors after an incorrect annotation are still being thrown.
9+
$output = `ls -al`;

src/Standards/Generic/Tests/PHP/BacktickOperatorUnitTest.php

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,10 @@ class BacktickOperatorUnitTest extends AbstractSniffUnitTest
2525
*/
2626
public function getErrorList()
2727
{
28-
return [2 => 2];
28+
return [
29+
2 => 2,
30+
9 => 2,
31+
];
2932

3033
}//end getErrorList()
3134

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

4549
}//end getWarningList()

0 commit comments

Comments
 (0)