Universal/DisallowAlternativeSyntax: bug fix - ignore inline HTML in …

…nested closed scopes ... as the HTML in that case is not necessarily echo-ed out when the control structure is executed, but rather is executed when the function in the nested closed scope is called and executed, so the inline HTML does not "belong" with the control structure. This should also potentially improve performance of the sniff when large chunks of code without inline HTML are wrapped within a control structure. Includes tests.
PHPCSStandards · Nov 14, 2022 · e4cfa4f · e4cfa4f
1 parent 538b90b
commit e4cfa4f
Show file tree

Hide file tree

Showing 4 changed files with 105 additions and 8 deletions.
diff --git a/Universal/Sniffs/ControlStructures/DisallowAlternativeSyntaxSniff.php b/Universal/Sniffs/ControlStructures/DisallowAlternativeSyntaxSniff.php
@@ -37,6 +37,9 @@ final class DisallowAlternativeSyntaxSniff implements Sniff
      * Whether to allow the alternative syntax when it is wrapped around
      * inline HTML, as is often seen in views.
      *
+     * Note: inline HTML within "closed scopes" - like function declarations -,
+     * within the control structure body will not be taken into account.
+     *
      * @since 1.0.0
      *
      * @var bool
@@ -108,19 +111,31 @@ public function process(File $phpcsFile, $stackPtr)
             return;
         }
 
-        $hasInlineHTML = $phpcsFile->findNext(
-            \T_INLINE_HTML,
-            $opener,
-            $closer
-        );
+        $closedScopes         = Collections::closedScopes();
+        $find                 = $closedScopes;
+        $find[\T_INLINE_HTML] = \T_INLINE_HTML;
+        $inlineHTMLPtr        = $opener;
+        $hasInlineHTML        = false;
+
+        do {
+            $inlineHTMLPtr = $phpcsFile->findNext($find, ($inlineHTMLPtr + 1), $closer);
+            if ($tokens[$inlineHTMLPtr]['code'] === \T_INLINE_HTML) {
+                $hasInlineHTML = true;
+                break;
+            }
+
+            if (isset($closedScopes[$tokens[$inlineHTMLPtr]['code']], $tokens[$inlineHTMLPtr]['scope_closer'])) {
+                $inlineHTMLPtr = $tokens[$inlineHTMLPtr]['scope_closer'];
+            }
+        } while ($inlineHTMLPtr !== false && $inlineHTMLPtr < $closer);
 
-        if ($hasInlineHTML !== false) {
+        if ($hasInlineHTML === true) {
             $phpcsFile->recordMetric($stackPtr, self::METRIC_NAME, 'alternative syntax with inline HTML');
         } else {
             $phpcsFile->recordMetric($stackPtr, self::METRIC_NAME, 'alternative syntax');
         }
 
-        if ($hasInlineHTML !== false && $this->allowWithInlineHTML === true) {
+        if ($hasInlineHTML === true && $this->allowWithInlineHTML === true) {
             return;
         }
 
@@ -131,7 +146,7 @@ public function process(File $phpcsFile, $stackPtr)
         $error .= '. Found: %1$s(): ... end%1$s;';
 
         $code = 'Found' . \ucfirst($tokens[$stackPtr]['content']);
-        if ($hasInlineHTML !== false) {
+        if ($hasInlineHTML === true) {
             $code .= 'WithInlineHTML';
         }
 

diff --git a/Universal/Tests/ControlStructures/DisallowAlternativeSyntaxUnitTest.inc b/Universal/Tests/ControlStructures/DisallowAlternativeSyntaxUnitTest.inc
@@ -222,6 +222,45 @@ A is something else
 <?php enddeclare; ?>
 
 <?php
+/*
+ * Tests specifically for the "has inline HTML ?" determination to make sure inline HTML
+ * in nested closed scopes is ignored.
+ */
+if (true):
+    echo 'no inline HTML';
+    ?><?php
+    echo 'between close and open tag';
+endif;
+
+if (!class_exists('Foo')) :
+    class Foo {
+        // Long piece of code
+        function bar() {
+            ?>
+            Inline HTML in closed scopes should be disregarded.
+            <?php
+        }
+    }
+endif;
+
+switch ($foo) :
+    case 1:
+        $closure = function () {
+            ?>
+            Inline HTML in closed scopes should be disregarded.
+            <?php
+        };
+        break;
+endswitch;
+
+declare (ticks = 1):
+    function showTicks() {
+        ?>
+        Inline HTML in closed scopes should be disregarded.
+        <?php
+    }
+enddeclare;
+
 // phpcs:set Universal.ControlStructures.DisallowAlternativeSyntax allowWithInlineHTML false
 
 // Live coding.

diff --git a/Universal/Tests/ControlStructures/DisallowAlternativeSyntaxUnitTest.inc.fixed b/Universal/Tests/ControlStructures/DisallowAlternativeSyntaxUnitTest.inc.fixed
@@ -222,6 +222,45 @@ A is something else
 <?php enddeclare; ?>
 
 <?php
+/*
+ * Tests specifically for the "has inline HTML ?" determination to make sure inline HTML
+ * in nested closed scopes is ignored.
+ */
+if (true){
+    echo 'no inline HTML';
+    ?><?php
+    echo 'between close and open tag';
+}
+
+if (!class_exists('Foo')) {
+    class Foo {
+        // Long piece of code
+        function bar() {
+            ?>
+            Inline HTML in closed scopes should be disregarded.
+            <?php
+        }
+    }
+}
+
+switch ($foo) {
+    case 1:
+        $closure = function () {
+            ?>
+            Inline HTML in closed scopes should be disregarded.
+            <?php
+        };
+        break;
+}
+
+declare (ticks = 1){
+    function showTicks() {
+        ?>
+        Inline HTML in closed scopes should be disregarded.
+        <?php
+    }
+}
+
 // phpcs:set Universal.ControlStructures.DisallowAlternativeSyntax allowWithInlineHTML false
 
 // Live coding.

diff --git a/Universal/Tests/ControlStructures/DisallowAlternativeSyntaxUnitTest.php b/Universal/Tests/ControlStructures/DisallowAlternativeSyntaxUnitTest.php
@@ -54,6 +54,10 @@ public function getErrorList()
             172 => 1,
             176 => 1,
             185 => 1,
+            229 => 1,
+            235 => 1,
+            246 => 1,
+            256 => 1,
         ];
     }