Skip to content

Commit b883123

Browse files
jrfnljrfnl
committed
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
1 parent 8b7c477 commit b883123

File tree

4 files changed

+99
-54
lines changed

4 files changed

+99
-54
lines changed

‎src/Standards/Squiz/Sniffs/WhiteSpace/MemberVarSpacingSniff.php‎

Lines changed: 41 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -82,60 +82,51 @@ protected function processMemberVar(File $phpcsFile, $stackPtr)
8282
break;
8383
}
8484

85-
if (isset(Tokens::$commentTokens[$tokens[$prev]['code']]) === true) {
85+
if ($tokens[$prev]['code'] === T_DOC_COMMENT_CLOSE_TAG) {
86+
$start = $prev;
87+
} else if (isset(Tokens::$commentTokens[$tokens[$prev]['code']]) === true) {
8688
// Assume the comment belongs to the member var if it is on a line by itself.
8789
$prevContent = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($prev - 1), null, true);
8890
if ($tokens[$prevContent]['line'] !== $tokens[$prev]['line']) {
89-
// Check the spacing, but then skip it.
90-
$foundLines = ($tokens[$startOfStatement]['line'] - $tokens[$prev]['line'] - 1);
91-
if ($foundLines > 0) {
92-
for ($i = ($prev + 1); $i < $startOfStatement; $i++) {
93-
if ($tokens[$i]['column'] !== 1) {
94-
continue;
95-
}
96-
97-
if ($tokens[$i]['code'] === T_WHITESPACE
98-
&& $tokens[$i]['line'] !== $tokens[($i + 1)]['line']
99-
) {
100-
$error = 'Expected 0 blank lines after member var comment; %s found';
101-
$data = [$foundLines];
102-
$fix = $phpcsFile->addFixableError($error, $prev, 'AfterComment', $data);
103-
if ($fix === true) {
104-
$phpcsFile->fixer->beginChangeset();
105-
// Inline comments have the newline included in the content but
106-
// docblocks do not.
107-
if ($tokens[$prev]['code'] === T_COMMENT) {
108-
$phpcsFile->fixer->replaceToken($prev, rtrim($tokens[$prev]['content']));
109-
}
110-
111-
for ($i = ($prev + 1); $i <= $startOfStatement; $i++) {
112-
if ($tokens[$i]['line'] === $tokens[$startOfStatement]['line']) {
113-
break;
114-
}
115-
116-
// Remove the newline after the docblock, and any entirely
117-
// empty lines before the member var.
118-
if (($tokens[$i]['code'] === T_WHITESPACE
119-
&& $tokens[$i]['line'] === $tokens[$prev]['line'])
120-
|| ($tokens[$i]['column'] === 1
121-
&& $tokens[$i]['line'] !== $tokens[($i + 1)]['line'])
122-
) {
123-
$phpcsFile->fixer->replaceToken($i, '');
124-
}
125-
}
126-
127-
$phpcsFile->fixer->addNewline($prev);
128-
$phpcsFile->fixer->endChangeset();
129-
}//end if
130-
131-
break;
132-
}//end if
133-
}//end for
134-
}//end if
135-
13691
$start = $prev;
137-
}//end if
138-
}//end if
92+
}
93+
}
94+
95+
// Check for blank lines between the docblock/comment and the property declaration.
96+
for ($i = ($start + 1); $i < $startOfStatement; $i++) {
97+
if ($tokens[$i]['column'] !== 1
98+
|| $tokens[$i]['code'] !== T_WHITESPACE
99+
|| $tokens[$i]['line'] === $tokens[($i + 1)]['line']
100+
// Do not report blank lines after a PHPCS annotation as removing the blank lines could change the meaning.
101+
|| isset(Tokens::$phpcsCommentTokens[$tokens[($i - 1)]['code']]) === true
102+
) {
103+
continue;
104+
}
105+
106+
// We found a blank line which should be reported.
107+
$nextNonWhitespace = $phpcsFile->findNext(T_WHITESPACE, ($i + 1), null, true);
108+
$foundLines = ($tokens[$nextNonWhitespace]['line'] - $tokens[$i]['line']);
109+
110+
$error = 'Expected no blank lines between the member var comment and the declaration; %s found';
111+
$data = [$foundLines];
112+
$fix = $phpcsFile->addFixableError($error, $i, 'AfterComment', $data);
113+
114+
if ($fix === true) {
115+
$phpcsFile->fixer->beginChangeset();
116+
117+
for ($j = $i; $j < $nextNonWhitespace; $j++) {
118+
if ($tokens[$j]['line'] === $tokens[$nextNonWhitespace]['line']) {
119+
break;
120+
}
121+
122+
$phpcsFile->fixer->replaceToken($j, '');
123+
}
124+
125+
$phpcsFile->fixer->endChangeset();
126+
}
127+
128+
$i = $nextNonWhitespace;
129+
}//end for
139130

140131
// There needs to be n blank lines before the var, not counting comments.
141132
if ($start === $startOfStatement) {

‎src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -385,3 +385,29 @@ class NoPreambleMultilineDeclaration {
385385
static
386386
int $prop = 1;
387387
}
388+
389+
class MultipleBlankLinesInPreAmble {
390+
391+
/**
392+
* Docblock.
393+
*/
394+
395+
#[MyAttribute]
396+
397+
398+
#[MyOtherAttribute]
399+
400+
public $prop;
401+
}
402+
403+
final class BlankLinesBetweenVsAttributesWithoutCommentIssueSquiz3594
404+
{
405+
406+
// phpcs can fix this but not the next one
407+
#[SingleAttribute]
408+
409+
public $property1;
410+
#[SingleAttribute]
411+
412+
public $property2;
413+
}

‎src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc.fixed‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -375,3 +375,24 @@ class NoPreambleMultilineDeclaration {
375375
static
376376
int $prop = 1;
377377
}
378+
379+
class MultipleBlankLinesInPreAmble {
380+
381+
/**
382+
* Docblock.
383+
*/
384+
#[MyAttribute]
385+
#[MyOtherAttribute]
386+
public $prop;
387+
}
388+
389+
final class BlankLinesBetweenVsAttributesWithoutCommentIssueSquiz3594
390+
{
391+
392+
// phpcs can fix this but not the next one
393+
#[SingleAttribute]
394+
public $property1;
395+
396+
#[SingleAttribute]
397+
public $property2;
398+
}

‎src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.php‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,9 @@ public function getErrorList($testFile='')
4444
50 => 1,
4545
73 => 1,
4646
86 => 1,
47-
106 => 1,
47+
107 => 1,
4848
115 => 1,
49-
150 => 1,
49+
151 => 1,
5050
160 => 1,
5151
165 => 1,
5252
177 => 1,
@@ -66,16 +66,23 @@ public function getErrorList($testFile='')
6666
288 => 1,
6767
292 => 1,
6868
333 => 1,
69-
342 => 1,
69+
343 => 1,
70+
345 => 1,
7071
346 => 1,
71-
353 => 1,
72+
355 => 1,
7273
357 => 1,
7374
366 => 1,
7475
377 => 1,
7576
378 => 1,
7677
379 => 1,
7778
380 => 1,
7879
384 => 1,
80+
394 => 1,
81+
396 => 1,
82+
399 => 1,
83+
408 => 1,
84+
411 => 1,
85+
412 => 1,
7986
];
8087

8188
default:

0 commit comments

Comments
 (0)