From e9ef421215d5b3ddf99082e8239f33da5101aef3 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 18 Feb 2025 09:45:49 +0100 Subject: [PATCH] Squiz/EmbeddedPhp: bug fix - fixer conflict with itself 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 ``` ... 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. --- .../Squiz/Sniffs/PHP/EmbeddedPhpSniff.php | 117 ++++++++++-------- .../Squiz/Tests/PHP/EmbeddedPhpUnitTest.1.inc | 14 +++ .../Tests/PHP/EmbeddedPhpUnitTest.1.inc.fixed | 20 +++ .../Tests/PHP/EmbeddedPhpUnitTest.25.inc | 21 ++++ .../PHP/EmbeddedPhpUnitTest.25.inc.fixed | 25 ++++ .../Squiz/Tests/PHP/EmbeddedPhpUnitTest.php | 21 +++- 6 files changed, 160 insertions(+), 58 deletions(-) create mode 100644 src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.25.inc create mode 100644 src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.25.inc.fixed diff --git a/src/Standards/Squiz/Sniffs/PHP/EmbeddedPhpSniff.php b/src/Standards/Squiz/Sniffs/PHP/EmbeddedPhpSniff.php index 63a1cdd09a..be07c5a29a 100644 --- 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,24 +169,28 @@ 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--) { @@ -205,27 +198,11 @@ private function validateMultilineEmbeddedPhp($phpcsFile, $stackPtr, $closingTag 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 index 015468357d..983b3f3734 100644 --- 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 + + + + + + + + + + + + + + + + + + + + + + 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