From 38924fb99523f1dc20c9d0e931e0827c7a61dcc6 Mon Sep 17 00:00:00 2001
From: jrfnl <jrfnl@users.noreply.github.com>
Date: Tue, 31 Oct 2023 20:39:47 +0100
Subject: [PATCH] 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.
---
 src/Files/File.php                             | 18 +++++++++++++++++-
 .../Tests/PHP/BacktickOperatorUnitTest.inc     |  7 +++++++
 .../Tests/PHP/BacktickOperatorUnitTest.php     |  6 +++++-
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/src/Files/File.php b/src/Files/File.php
index 21cbeb2158..ab9c2ce164 100644
--- a/src/Files/File.php
+++ b/src/Files/File.php
@@ -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.
@@ -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
@@ -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;
diff --git a/src/Standards/Generic/Tests/PHP/BacktickOperatorUnitTest.inc b/src/Standards/Generic/Tests/PHP/BacktickOperatorUnitTest.inc
index 3355c2d32c..e2ca72d1f1 100644
--- a/src/Standards/Generic/Tests/PHP/BacktickOperatorUnitTest.inc
+++ b/src/Standards/Generic/Tests/PHP/BacktickOperatorUnitTest.inc
@@ -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`;
diff --git a/src/Standards/Generic/Tests/PHP/BacktickOperatorUnitTest.php b/src/Standards/Generic/Tests/PHP/BacktickOperatorUnitTest.php
index a2541851d7..1582ab2d2d 100644
--- a/src/Standards/Generic/Tests/PHP/BacktickOperatorUnitTest.php
+++ b/src/Standards/Generic/Tests/PHP/BacktickOperatorUnitTest.php
@@ -25,7 +25,10 @@ class BacktickOperatorUnitTest extends AbstractSniffUnitTest
      */
     public function getErrorList()
     {
-        return [2 => 2];
+        return [
+            2 => 2,
+            9 => 2,
+        ];
 
     }//end getErrorList()
 
@@ -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()