Skip to content

Commit 540c3bc

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 77cee16 commit 540c3bc

File tree

4 files changed

+30
-2
lines changed

4 files changed

+30
-2
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ The file documents changes to the PHP_CodeSniffer project.
5959
- A descriptive error message is provided to allow users to fix their ruleset
6060
- Sniff properties set for complete standards/complete sniff categories will now only be set on sniffs which explicitly support the property
6161
- The property will be silently ignored for those sniffs which do not support the property
62+
- Invalid sniff properties set for sniffs via inline annotations will result in an informative `Internal.PropertyDoesNotExist` errror on line 1 of the scanned file, but will not halt the execution of PHPCS
6263
- For sniff developers, it is strongly recommended for sniffs to explicitly declare any user-adjustable public properties
6364
- If dynamic properties need to be supported for a sniff, either declare the magic __set()/__get()/__isset()/__unset() methods on the sniff or let the sniff extend stdClass
6465
- Note: The #[\AllowDynamicProperties] attribute will have no effect for properties which are being set in rulesets.

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)