From b88312369020662c5e2ac9202eb4267143d68bd5 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 17 Feb 2025 03:25:47 +0100 Subject: [PATCH 1/2] Squiz/MemberVarSpacing: bug fix - handling of blank lines in pre-amble The `Squiz.WhiteSpace.MemberVarSpacing` sniff intends to flag blank lines between a property docblock and the property declaration. However, as things are, there are - IMO - two bugs in the logic for this: Given a code block which looks like this: ```php class MultipleBlankLinesInPreAmble { /** * Docblock. */ #[MyAttribute] #[MyOtherAttribute] public $prop; } ``` There will be only one error and it will read: ``` [x] Expected 0 blank lines after member var comment; 6 found (Squiz.WhiteSpace.MemberVarSpacing.AfterComment) ``` This is confusing as there are not 6 blank lines between the end of the docblock and the property, but four blank lines and two attribute lines. What I would expect would be for the sniff to: a) Throw a separate error for each (set of) blank lines found. b) For the error message to correctly reflect the number of blank lines found for each error. > Note: while in PHPCS this gets confusing, the fixer already fixes this correctly. This commit changes the `AfterComment` error to comply with the above expectations Includes test, though there are also some pre-existing tests which show this issue and for which the behaviour is changed (error lines: 342 and 353). _Note: while it will still be messy, it may be easier to review this PR while ignoring whitespace changes._ Fixes squizlabs/PHP_CodeSniffer 3594 --- .../WhiteSpace/MemberVarSpacingSniff.php | 91 +++++++++---------- .../WhiteSpace/MemberVarSpacingUnitTest.1.inc | 26 ++++++ .../MemberVarSpacingUnitTest.1.inc.fixed | 21 +++++ .../WhiteSpace/MemberVarSpacingUnitTest.php | 15 ++- 4 files changed, 99 insertions(+), 54 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/WhiteSpace/MemberVarSpacingSniff.php b/src/Standards/Squiz/Sniffs/WhiteSpace/MemberVarSpacingSniff.php index 96cfd7aa95..b364fcace3 100644 --- a/src/Standards/Squiz/Sniffs/WhiteSpace/MemberVarSpacingSniff.php +++ b/src/Standards/Squiz/Sniffs/WhiteSpace/MemberVarSpacingSniff.php @@ -82,60 +82,51 @@ protected function processMemberVar(File $phpcsFile, $stackPtr) break; } - if (isset(Tokens::$commentTokens[$tokens[$prev]['code']]) === true) { + if ($tokens[$prev]['code'] === T_DOC_COMMENT_CLOSE_TAG) { + $start = $prev; + } else if (isset(Tokens::$commentTokens[$tokens[$prev]['code']]) === true) { // Assume the comment belongs to the member var if it is on a line by itself. $prevContent = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($prev - 1), null, true); if ($tokens[$prevContent]['line'] !== $tokens[$prev]['line']) { - // Check the spacing, but then skip it. - $foundLines = ($tokens[$startOfStatement]['line'] - $tokens[$prev]['line'] - 1); - if ($foundLines > 0) { - for ($i = ($prev + 1); $i < $startOfStatement; $i++) { - if ($tokens[$i]['column'] !== 1) { - continue; - } - - if ($tokens[$i]['code'] === T_WHITESPACE - && $tokens[$i]['line'] !== $tokens[($i + 1)]['line'] - ) { - $error = 'Expected 0 blank lines after member var comment; %s found'; - $data = [$foundLines]; - $fix = $phpcsFile->addFixableError($error, $prev, 'AfterComment', $data); - if ($fix === true) { - $phpcsFile->fixer->beginChangeset(); - // Inline comments have the newline included in the content but - // docblocks do not. - if ($tokens[$prev]['code'] === T_COMMENT) { - $phpcsFile->fixer->replaceToken($prev, rtrim($tokens[$prev]['content'])); - } - - for ($i = ($prev + 1); $i <= $startOfStatement; $i++) { - if ($tokens[$i]['line'] === $tokens[$startOfStatement]['line']) { - break; - } - - // Remove the newline after the docblock, and any entirely - // empty lines before the member var. - if (($tokens[$i]['code'] === T_WHITESPACE - && $tokens[$i]['line'] === $tokens[$prev]['line']) - || ($tokens[$i]['column'] === 1 - && $tokens[$i]['line'] !== $tokens[($i + 1)]['line']) - ) { - $phpcsFile->fixer->replaceToken($i, ''); - } - } - - $phpcsFile->fixer->addNewline($prev); - $phpcsFile->fixer->endChangeset(); - }//end if - - break; - }//end if - }//end for - }//end if - $start = $prev; - }//end if - }//end if + } + } + + // Check for blank lines between the docblock/comment and the property declaration. + for ($i = ($start + 1); $i < $startOfStatement; $i++) { + if ($tokens[$i]['column'] !== 1 + || $tokens[$i]['code'] !== T_WHITESPACE + || $tokens[$i]['line'] === $tokens[($i + 1)]['line'] + // Do not report blank lines after a PHPCS annotation as removing the blank lines could change the meaning. + || isset(Tokens::$phpcsCommentTokens[$tokens[($i - 1)]['code']]) === true + ) { + continue; + } + + // We found a blank line which should be reported. + $nextNonWhitespace = $phpcsFile->findNext(T_WHITESPACE, ($i + 1), null, true); + $foundLines = ($tokens[$nextNonWhitespace]['line'] - $tokens[$i]['line']); + + $error = 'Expected no blank lines between the member var comment and the declaration; %s found'; + $data = [$foundLines]; + $fix = $phpcsFile->addFixableError($error, $i, 'AfterComment', $data); + + if ($fix === true) { + $phpcsFile->fixer->beginChangeset(); + + for ($j = $i; $j < $nextNonWhitespace; $j++) { + if ($tokens[$j]['line'] === $tokens[$nextNonWhitespace]['line']) { + break; + } + + $phpcsFile->fixer->replaceToken($j, ''); + } + + $phpcsFile->fixer->endChangeset(); + } + + $i = $nextNonWhitespace; + }//end for // There needs to be n blank lines before the var, not counting comments. if ($start === $startOfStatement) { diff --git a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc index b2e9db886e..361907517f 100644 --- a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc +++ b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc @@ -385,3 +385,29 @@ class NoPreambleMultilineDeclaration { static int $prop = 1; } + +class MultipleBlankLinesInPreAmble { + + /** + * Docblock. + */ + + #[MyAttribute] + + + #[MyOtherAttribute] + + public $prop; +} + +final class BlankLinesBetweenVsAttributesWithoutCommentIssueSquiz3594 +{ + + // phpcs can fix this but not the next one + #[SingleAttribute] + + public $property1; + #[SingleAttribute] + + public $property2; +} diff --git a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc.fixed b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc.fixed index 86835dfc6d..91158290e8 100644 --- a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc.fixed +++ b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc.fixed @@ -375,3 +375,24 @@ class NoPreambleMultilineDeclaration { static int $prop = 1; } + +class MultipleBlankLinesInPreAmble { + + /** + * Docblock. + */ + #[MyAttribute] + #[MyOtherAttribute] + public $prop; +} + +final class BlankLinesBetweenVsAttributesWithoutCommentIssueSquiz3594 +{ + + // phpcs can fix this but not the next one + #[SingleAttribute] + public $property1; + + #[SingleAttribute] + public $property2; +} diff --git a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.php b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.php index 1031df5d2c..3e144412da 100644 --- a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.php +++ b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.php @@ -44,9 +44,9 @@ public function getErrorList($testFile='') 50 => 1, 73 => 1, 86 => 1, - 106 => 1, + 107 => 1, 115 => 1, - 150 => 1, + 151 => 1, 160 => 1, 165 => 1, 177 => 1, @@ -66,9 +66,10 @@ public function getErrorList($testFile='') 288 => 1, 292 => 1, 333 => 1, - 342 => 1, + 343 => 1, + 345 => 1, 346 => 1, - 353 => 1, + 355 => 1, 357 => 1, 366 => 1, 377 => 1, @@ -76,6 +77,12 @@ public function getErrorList($testFile='') 379 => 1, 380 => 1, 384 => 1, + 394 => 1, + 396 => 1, + 399 => 1, + 408 => 1, + 411 => 1, + 412 => 1, ]; default: From 3239bbd2803535276c09b97cef6de31383ac5f50 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 17 Feb 2025 04:30:50 +0100 Subject: [PATCH 2/2] Squiz/MemberVarSpacing: bug fix - don't remove blank attribute lines While blank lines between the docblock and the property declaration should be removed, blank lines _within_ an attribute should be ignored as that's for a sniff dealing with attribute spacing to deal with and is outside of the scope of this sniff. Fixed now. Includes updated test. --- .../Squiz/Sniffs/WhiteSpace/MemberVarSpacingSniff.php | 5 +++++ .../Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc | 6 +++++- .../Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc.fixed | 6 +++++- .../Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.php | 6 +++--- 4 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/WhiteSpace/MemberVarSpacingSniff.php b/src/Standards/Squiz/Sniffs/WhiteSpace/MemberVarSpacingSniff.php index b364fcace3..c5597012e6 100644 --- a/src/Standards/Squiz/Sniffs/WhiteSpace/MemberVarSpacingSniff.php +++ b/src/Standards/Squiz/Sniffs/WhiteSpace/MemberVarSpacingSniff.php @@ -94,6 +94,11 @@ protected function processMemberVar(File $phpcsFile, $stackPtr) // Check for blank lines between the docblock/comment and the property declaration. for ($i = ($start + 1); $i < $startOfStatement; $i++) { + if (isset($tokens[$i]['attribute_closer']) === true) { + $i = $tokens[$i]['attribute_closer']; + continue; + } + if ($tokens[$i]['column'] !== 1 || $tokens[$i]['code'] !== T_WHITESPACE || $tokens[$i]['line'] === $tokens[($i + 1)]['line'] diff --git a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc index 361907517f..33979c2b68 100644 --- a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc +++ b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc @@ -395,7 +395,11 @@ class MultipleBlankLinesInPreAmble { #[MyAttribute] - #[MyOtherAttribute] + #[ + + BlankLinesWithinAnAttributeShouldBeLeftAlone + + ] public $prop; } diff --git a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc.fixed b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc.fixed index 91158290e8..f3ea2c9ca4 100644 --- a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc.fixed +++ b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc.fixed @@ -382,7 +382,11 @@ class MultipleBlankLinesInPreAmble { * Docblock. */ #[MyAttribute] - #[MyOtherAttribute] + #[ + + BlankLinesWithinAnAttributeShouldBeLeftAlone + + ] public $prop; } diff --git a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.php b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.php index 3e144412da..2ecba697aa 100644 --- a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.php +++ b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.php @@ -79,10 +79,10 @@ public function getErrorList($testFile='') 384 => 1, 394 => 1, 396 => 1, - 399 => 1, - 408 => 1, - 411 => 1, + 403 => 1, 412 => 1, + 415 => 1, + 416 => 1, ]; default: