Squiz/EmbeddedPhp: bug fix - fixer conflict with itself

jrfnl · jrfnl · commit e9ef421215d5 · 2025-02-18T16:39:33.000+01:00
For multi-line PHP snippets, the `Squiz.PHP.EmbeddedPhp` sniff expects the open and close tag to each be on their own line.
For single-line PHP snippets, it does not.

Now, the sniff already handled a close tag of a previous multi-line snippet and the open tag of a next multi-line snippet being on the same line correctly and prevented a fixer conflict for that, but it did not correctly handle the open tag of a multi-line snippet being on the same line after a single-line PHP snippet.

In that case, it would not recognize that the open tag had to be moved to its own line and it would also calculate the expected indent for both the PHP open tag as well as the first line of the content within the multi-line snippet incorrectly, which in turn would lead to fixer conflicts with the `Generic.WhiteSpace.ScopeIndent` sniff.

I.e. for the new test added:
```php
&lt;?php echo 'Open tag after code'; ?&gt;            &lt;?php
echo $j;
?&gt;
```
... the sniff would previously throw the following error:
```
ERROR | [x] Opening PHP tag indent incorrect; expected no more than 4 spaces but found 48 (Squiz.PHP.EmbeddedPhp.OpenTagIndent)
ERROR | [x] First line of embedded PHP code must be indented 11 spaces; 0 found (Squiz.PHP.EmbeddedPhp.Indent)
```
... and the fixer would conflict and try to add the same indent to the open tag time and time again, but without adding a new line, which meant it was replacing the token content with the existing content, not fixing anything. Hence, the fixer conflict with itself.

Also take note of the incorrect indent expectation for the next line (11 spaces).

This commit fixes both issues by:
1. Improving the "does this line containing a PHP open tag have content on it before" verification for the `ContentBeforeOpen` error and
2. Fixing the "what should the indent be" calculation for both the `ContentBeforeOpen` error (for the indent when the tag is moved to the next line), as well as for the `Indent` error via a new `calculateLineIndent()` method which takes a lot more potential "first token on a line which may contain whitespace" situations into account.

With the fix in place, it will now show the following error:
```
ERROR | [x] Opening PHP tag must be on a line by itself (Squiz.PHP.EmbeddedPhp.ContentBeforeOpen)
```

Includes implementing the use of the new `calculateLineIndent()` method in other places in the sniff to make sure this calculation is consistent throughout the sniff.

Includes tests, also specifically for the new method.

The change does mean that some existing snippets now get two errors instead of one when a close tag + an open tag for multi-line snippets are on the same line.
In my opinion, this should be regarded as a bug fix for the second error previously not showing.
As for the fixer, the end-result for those snippets is the same, so it doesn't result in a new conflict (as the sniff already contains protection against that specific conflict).

For the reviewers: I've verified that the error messages for pre-existing tests involving indent calculations are 100% the same before and after the change.
diff --git a/src/Standards/Squiz/Sniffs/PHP/EmbeddedPhpSniff.php b/src/Standards/Squiz/Sniffs/PHP/EmbeddedPhpSniff.php
@@ -102,8 +102,7 @@ private function validateMultilineEmbeddedPhp($phpcsFile, $stackPtr, $closingTag
             $error = 'Opening PHP tag must be on a line by itself';
             $fix   = $phpcsFile->addFixableError($error, $stackPtr, 'ContentAfterOpen');
             if ($fix === true) {
-                $first   = $phpcsFile->findFirstOnLine(T_WHITESPACE, $stackPtr, true);
-                $padding = (strlen($tokens[$first]['content']) - strlen(ltrim($tokens[$first]['content'])));
+                $padding = $this->calculateLineIndent($phpcsFile, $stackPtr);
 
                 $phpcsFile->fixer->beginChangeset();
                 $phpcsFile->fixer->replaceToken($stackPtr, rtrim($tokens[$stackPtr]['content']));
@@ -147,17 +146,7 @@ private function validateMultilineEmbeddedPhp($phpcsFile, $stackPtr, $closingTag
                     }
                 }//end if
 
-                $indent = 0;
-                $first  = $phpcsFile->findFirstOnLine(T_WHITESPACE, $stackPtr);
-                if ($first === false) {
-                    $first = $phpcsFile->findFirstOnLine(T_INLINE_HTML, $stackPtr);
-                    if ($first !== false) {
-                        $indent = (strlen($tokens[$first]['content']) - strlen(ltrim($tokens[$first]['content'])));
-                    }
-                } else {
-                    $indent = ($tokens[($first + 1)]['column'] - 1);
-                }
-
+                $indent        = $this->calculateLineIndent($phpcsFile, $stackPtr);
                 $contentColumn = ($tokens[$firstContent]['column'] - 1);
                 if ($contentColumn !== $indent) {
                     $error = 'First line of embedded PHP code must be indented %s spaces; %s found';
@@ -180,52 +169,40 @@ private function validateMultilineEmbeddedPhp($phpcsFile, $stackPtr, $closingTag
 
         $lastContentBeforeBlock = $phpcsFile->findPrevious(T_WHITESPACE, ($stackPtr - 1), null, true);
         if ($tokens[$lastContentBeforeBlock]['line'] === $tokens[$stackPtr]['line']
-            && trim($tokens[$lastContentBeforeBlock]['content']) !== ''
+            && (($tokens[$lastContentBeforeBlock]['code'] === T_INLINE_HTML
+            && trim($tokens[$lastContentBeforeBlock]['content']) !== '')
+            || ($tokens[($lastContentBeforeBlock - 1)]['code'] !== T_INLINE_HTML
+            && $tokens[($lastContentBeforeBlock - 1)]['line'] === $tokens[$stackPtr]['line']))
         ) {
             $error = 'Opening PHP tag must be on a line by itself';
             $fix   = $phpcsFile->addFixableError($error, $stackPtr, 'ContentBeforeOpen');
             if ($fix === true) {
-                $padding = 0;
-                $first   = $phpcsFile->findFirstOnLine(T_WHITESPACE, $stackPtr);
-                if ($first === false) {
-                    $first = $phpcsFile->findFirstOnLine(T_INLINE_HTML, $stackPtr);
-                    if ($first !== false) {
-                        $padding = (strlen($tokens[$first]['content']) - strlen(ltrim($tokens[$first]['content'])));
-                    }
-                } else {
-                    $padding = ($tokens[($first + 1)]['column'] - 1);
-                }
+                $padding = $this->calculateLineIndent($phpcsFile, $lastContentBeforeBlock);
 
+                $phpcsFile->fixer->beginChangeset();
                 $phpcsFile->fixer->addContentBefore($stackPtr, $phpcsFile->eolChar.str_repeat(' ', $padding));
-            }
+
+                // Make sure we don't leave trailing whitespace behind.
+                if ($tokens[($stackPtr - 1)]['code'] === T_INLINE_HTML
+                    && trim($tokens[($stackPtr - 1)]['content']) === ''
+                ) {
+                    $phpcsFile->fixer->replaceToken(($stackPtr - 1), '');
+                }
+
+                $phpcsFile->fixer->endChangeset();
+            }//end if
         } else {
             // Find the first token on the first non-empty line we find.
             for ($first = ($lastContentBeforeBlock - 1); $first > 0; $first--) {
                 if ($tokens[$first]['line'] === $tokens[$stackPtr]['line']) {
                     continue;
                 } else if (trim($tokens[$first]['content']) !== '') {
                     $first = $phpcsFile->findFirstOnLine([], $first, true);
-                    if ($tokens[$first]['code'] === T_COMMENT
-                        && $tokens[$first]['content'] !== ltrim($tokens[$first]['content'])
-                    ) {
-                        // This is a subsequent line in a star-slash comment containing leading indent.
-                        // We'll need the first line of the comment to correctly determine the indent.
-                        continue;
-                    }
-
                     break;
                 }
             }
 
-            $expected = 0;
-            if ($tokens[$first]['code'] === T_INLINE_HTML
-                && trim($tokens[$first]['content']) !== ''
-            ) {
-                $expected = (strlen($tokens[$first]['content']) - strlen(ltrim($tokens[$first]['content'])));
-            } else if ($tokens[$first]['code'] === T_WHITESPACE) {
-                $expected = ($tokens[($first + 1)]['column'] - 1);
-            }
-
+            $expected  = $this->calculateLineIndent($phpcsFile, $first);
             $expected += 4;
             $found     = ($tokens[$stackPtr]['column'] - 1);
             if ($found > $expected) {
@@ -261,17 +238,7 @@ private function validateMultilineEmbeddedPhp($phpcsFile, $stackPtr, $closingTag
                 ) {
                     $closerIndent = $indent;
                 } else {
-                    $first = $phpcsFile->findFirstOnLine(T_WHITESPACE, $closingTag, true);
-
-                    while ($tokens[$first]['code'] === T_COMMENT
-                        && $tokens[$first]['content'] !== ltrim($tokens[$first]['content'])
-                    ) {
-                        // This is a subsequent line in a star-slash comment containing leading indent.
-                        // We'll need the first line of the comment to correctly determine the indent.
-                        $first = $phpcsFile->findFirstOnLine(T_WHITESPACE, ($first - 1), true);
-                    }
-
-                    $closerIndent = ($tokens[$first]['column'] - 1);
+                    $closerIndent = $this->calculateLineIndent($phpcsFile, $closingTag);
                 }
 
                 $phpcsFile->fixer->beginChangeset();
@@ -290,10 +257,10 @@ private function validateMultilineEmbeddedPhp($phpcsFile, $stackPtr, $closingTag
             $error = 'Closing PHP tag must be on a line by itself';
             $fix   = $phpcsFile->addFixableError($error, $closingTag, 'ContentAfterEnd');
             if ($fix === true) {
-                $first = $phpcsFile->findFirstOnLine(T_WHITESPACE, $closingTag, true);
+                $indent = $this->calculateLineIndent($phpcsFile, $closingTag);
                 $phpcsFile->fixer->beginChangeset();
                 $phpcsFile->fixer->addNewline($closingTag);
-                $phpcsFile->fixer->addContent($closingTag, str_repeat(' ', ($tokens[$first]['column'] - 1)));
+                $phpcsFile->fixer->addContent($closingTag, str_repeat(' ', $indent));
 
                 if ($tokens[$firstContentAfterBlock]['code'] === T_INLINE_HTML) {
                     $trimmedHtmlContent = ltrim($tokens[$firstContentAfterBlock]['content']);
@@ -513,4 +480,44 @@ private function reportEmptyTagSet(File $phpcsFile, $stackPtr, $closeTag)
     }//end reportEmptyTagSet()
 
 
+    /**
+     * Calculate the indent of the line containing the stackPtr.
+     *
+     * @param \PHP_CodeSniffer\Files\File $phpcsFile The file being scanned.
+     * @param int                         $stackPtr  The position of the current token in the
+     *                                               stack passed in $tokens.
+     *
+     * @return int
+     */
+    private function calculateLineIndent(File $phpcsFile, $stackPtr)
+    {
+        $tokens = $phpcsFile->getTokens();
+
+        for ($firstOnLine = $stackPtr; $tokens[$firstOnLine]['column'] !== 1; $firstOnLine--);
+
+        // Check if this is a subsequent line in a star-slash comment containing leading indent.
+        // In that case, we'll need the first line of the comment to correctly determine the indent.
+        while ($tokens[$firstOnLine]['code'] === T_COMMENT
+            && $tokens[$firstOnLine]['content'] !== ltrim($tokens[$firstOnLine]['content'])
+        ) {
+            for (--$firstOnLine; $tokens[$firstOnLine]['column'] !== 1; $firstOnLine--);
+        }
+
+        $indent = 0;
+        if ($tokens[$firstOnLine]['code'] === T_WHITESPACE) {
+            $indent = ($tokens[($firstOnLine + 1)]['column'] - 1);
+        } else if ($tokens[$firstOnLine]['code'] === T_INLINE_HTML
+            || $tokens[$firstOnLine]['code'] === T_END_HEREDOC
+            || $tokens[$firstOnLine]['code'] === T_END_NOWDOC
+        ) {
+            $indent = (strlen($tokens[$firstOnLine]['content']) - strlen(ltrim($tokens[$firstOnLine]['content'])));
+        } else if ($tokens[$firstOnLine]['code'] === T_DOC_COMMENT_WHITESPACE) {
+            $indent = (strlen($tokens[$firstOnLine]['content']) - strlen(ltrim($tokens[$firstOnLine]['content'])) - 1);
+        }
+
+        return $indent;
+
+    }//end calculateLineIndent()
+
+
 }//end class
diff --git a/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.1.inc b/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.1.inc
@@ -269,6 +269,20 @@ echo 'the PHP tag is correctly indented as an indent less than the previous code
 
 <?PHP  echo 'Extra space after uppercase long open tag '; ?>
 
+<?php echo 'Open tag after code'; ?>            <?php
+echo $j;
+?>
+
+    <?php echo 'Open tag after code - indented'; ?>            <?php
+    echo $j;
+    /* comment
+    */ /*comment*/ ?>            <?php
+    /**
+     * Docblock.
+     */   ?>            <?php
+    echo $j;
+    ?>
+
 <?php
 // This test case file MUST always end with an unclosed long open PHP tag (with this comment) to prevent
 // the tests running into the "last PHP closing tag excepted" condition breaking tests.
diff --git a/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.1.inc.fixed b/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.1.inc.fixed
@@ -281,6 +281,26 @@ echo 'the PHP tag is correctly indented as an indent less than the previous code
 
 <?PHP echo 'Extra space after uppercase long open tag '; ?>
 
+<?php echo 'Open tag after code'; ?>
+<?php
+echo $j;
+?>
+
+    <?php echo 'Open tag after code - indented'; ?>
+    <?php
+    echo $j;
+    /* comment
+    */ /*comment*/
+    ?>
+    <?php
+    /**
+     * Docblock.
+     */
+    ?>
+    <?php
+    echo $j;
+    ?>
+
 <?php
 // This test case file MUST always end with an unclosed long open PHP tag (with this comment) to prevent
 // the tests running into the "last PHP closing tag excepted" condition breaking tests.
diff --git a/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.25.inc b/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.25.inc
@@ -0,0 +1,21 @@
+<?php
+
+// This test case file MUST always start with a open PHP tag set (with this comment) to prevent
+// the tests running into the "first PHP open tag excepted" condition breaking the tests.
+// Tests related to that "first PHP open tag excepted" condition should go in separate files.
+
+// Tests indent calculation in combination with PHP 7.3+ flexible heredoc/nowdocs.
+?>
+
+    <?php
+    echo <<<HEREDOC
+    HEREDOC; ?>            <?php
+    echo <<<'NOWDOC'
+    NOWDOC; ?>            <?php
+    echo $j;
+    ?>
+
+<?php
+// This test case file MUST always end with an unclosed open PHP tag (with this comment) to prevent
+// the tests running into the "last PHP closing tag excepted" condition breaking tests.
+// Tests related to that "last PHP closing tag excepted" condition should go in separate files.
diff --git a/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.25.inc.fixed b/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.25.inc.fixed
@@ -0,0 +1,25 @@
+<?php
+
+// This test case file MUST always start with a open PHP tag set (with this comment) to prevent
+// the tests running into the "first PHP open tag excepted" condition breaking the tests.
+// Tests related to that "first PHP open tag excepted" condition should go in separate files.
+
+// Tests indent calculation in combination with PHP 7.3+ flexible heredoc/nowdocs.
+?>
+
+    <?php
+    echo <<<HEREDOC
+    HEREDOC;
+    ?>
+    <?php
+    echo <<<'NOWDOC'
+    NOWDOC;
+    ?>
+    <?php
+    echo $j;
+    ?>
+
+<?php
+// This test case file MUST always end with an unclosed open PHP tag (with this comment) to prevent
+// the tests running into the "last PHP closing tag excepted" condition breaking tests.
+// Tests related to that "last PHP closing tag excepted" condition should go in separate files.
diff --git a/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.php b/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.php
@@ -80,7 +80,7 @@ public function getErrorList($testFile='')
                 169 => 1,
                 175 => 1,
                 176 => 2,
-                178 => 1,
+                178 => 2,
                 179 => 1,
                 180 => 2,
                 181 => 1,
@@ -101,6 +101,10 @@ public function getErrorList($testFile='')
                 263 => 1,
                 264 => 1,
                 270 => 1,
+                272 => 1,
+                276 => 1,
+                279 => 2,
+                282 => 2,
             ];
 
         case 'EmbeddedPhpUnitTest.2.inc':
@@ -148,7 +152,7 @@ public function getErrorList($testFile='')
                 105 => 1,
                 111 => 1,
                 112 => 2,
-                114 => 1,
+                114 => 2,
                 115 => 1,
                 116 => 2,
                 117 => 1,
@@ -188,7 +192,7 @@ public function getErrorList($testFile='')
         case 'EmbeddedPhpUnitTest.22.inc':
             return [
                 14 => 1,
-                22 => 2,
+                22 => 1,
             ];
 
         case 'EmbeddedPhpUnitTest.24.inc':
@@ -201,6 +205,17 @@ public function getErrorList($testFile='')
             }
             return [];
 
+        case 'EmbeddedPhpUnitTest.25.inc':
+            if (PHP_VERSION_ID >= 70300) {
+                return [
+                    12 => 2,
+                    14 => 2,
+                ];
+            }
+
+            // PHP 7.2 or lower: PHP version which doesn't support flexible heredocs/nowdocs yet.
+            return [];
+
         default:
             return [];
         }//end switch
