Skip to content

Commit a8371a2

Browse files
committed
PEAR/FunctionComment: bug fix - handling of blank lines in pre-amble
The `PEAR.Commenting.FunctionComment` sniff intends to flag blank lines between a function docblock and the function 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 HandleBlankLinesBetweenDocblockAndDeclarationWithAttributes { /** * Blank line between docblock and attribute. * * @return mixed */ #[ReturnTypeWillChange] #[ AnotherAttribute ]#[AndAThirdAsWell] public function blankLineDetectionA() { }//end blankLineDetectionA() } ``` There will be only one error and it will read: ``` [x] There must be no blank lines after the function comment (PEAR.Commenting.FunctionComment.SpacingAfter) ``` This is confusing as the blank line may not be after the function comment, but after an attribute instead. Additionally, the sniff also flags blank lines _within_ attributes, while that is outside of the purview of the sniff. (Should be handled by an attribute specific sniff) 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 more accurately reflect what is being flagged. > Note: while in PHPCS this gets confusing, the fixer already fixes this correctly. This commit changes the `SpacingAfter` 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. _Note: while it will still be messy, it may be easier to review this PR while ignoring whitespace changes._
1 parent 71fcf9a commit a8371a2

File tree

4 files changed

+103
-19
lines changed

4 files changed

+103
-19
lines changed

src/Standards/PEAR/Sniffs/Commenting/FunctionCommentSniff.php

+25-14
Original file line numberDiff line numberDiff line change
@@ -119,33 +119,44 @@ public function process(File $phpcsFile, $stackPtr)
119119
return;
120120
}
121121

122+
// Check there are no blank lines in the preamble for the property,
123+
// but ignore blank lines _within_ attributes as that's not the concern of this sniff.
122124
if ($tokens[$commentEnd]['line'] !== ($tokens[$stackPtr]['line'] - 1)) {
123125
for ($i = ($commentEnd + 1); $i < $stackPtr; $i++) {
124-
if ($tokens[$i]['column'] !== 1) {
126+
// Skip over the contents of attributes.
127+
if (isset($tokens[$i]['attribute_closer']) === true) {
128+
$i = $tokens[$i]['attribute_closer'];
125129
continue;
126130
}
127131

128-
if ($tokens[$i]['code'] === T_WHITESPACE
129-
&& $tokens[$i]['line'] !== $tokens[($i + 1)]['line']
132+
if ($tokens[$i]['column'] !== 1
133+
|| $tokens[$i]['code'] !== T_WHITESPACE
134+
|| $tokens[$i]['line'] === $tokens[($i + 1)]['line']
135+
// Do not report blank lines after a PHPCS annotation as removing the blank lines could change the meaning.
136+
|| isset(Tokens::$phpcsCommentTokens[$tokens[($i - 1)]['code']]) === true
130137
) {
131-
$error = 'There must be no blank lines after the function comment';
132-
$fix = $phpcsFile->addFixableError($error, $commentEnd, 'SpacingAfter');
138+
continue;
139+
}
133140

134-
if ($fix === true) {
135-
$phpcsFile->fixer->beginChangeset();
141+
$nextNonWhitespace = $phpcsFile->findNext(T_WHITESPACE, ($i + 1), null, true);
142+
$error = 'There must be no blank lines between the function comment and the declaration';
143+
$fix = $phpcsFile->addFixableError($error, $i, 'SpacingAfter');
136144

137-
while ($i < $stackPtr
138-
&& $tokens[$i]['code'] === T_WHITESPACE
139-
&& $tokens[$i]['line'] !== $tokens[($i + 1)]['line']
140-
) {
141-
$phpcsFile->fixer->replaceToken($i++, '');
145+
if ($fix === true) {
146+
$phpcsFile->fixer->beginChangeset();
147+
148+
for ($j = $i; $j < $nextNonWhitespace; $j++) {
149+
if ($tokens[$j]['line'] === $tokens[$nextNonWhitespace]['line']) {
150+
break;
142151
}
143152

144-
$phpcsFile->fixer->endChangeset();
153+
$phpcsFile->fixer->replaceToken($j, '');
145154
}
146155

147-
break;
156+
$phpcsFile->fixer->endChangeset();
148157
}
158+
159+
$i = $nextNonWhitespace;
149160
}//end for
150161
}//end if
151162

src/Standards/PEAR/Tests/Commenting/FunctionCommentUnitTest.inc

+38
Original file line numberDiff line numberDiff line change
@@ -510,3 +510,41 @@ class SpacingAfter {
510510

511511
public function multipleLinesSomeEmpty() {}
512512
}
513+
514+
class HandleBlankLinesBetweenDocblockAndDeclarationWithAttributes
515+
{
516+
/**
517+
* Blank line between docblock and attribute.
518+
*
519+
* @return mixed
520+
*/
521+
522+
#[ReturnTypeWillChange]
523+
524+
525+
526+
527+
528+
#[
529+
530+
AnotherAttribute
531+
532+
]#[AndAThirdAsWell]
533+
534+
public function blankLineDetectionA()
535+
{
536+
537+
}//end blankLineDetectionA()
538+
539+
/**
540+
* Blank line between attribute and function declaration.
541+
*
542+
* @return mixed
543+
*/
544+
#[ReturnTypeWillChange]
545+
546+
public function blankLineDetectionB()
547+
{
548+
549+
}//end blankLineDetectionB()
550+
}//end class

src/Standards/PEAR/Tests/Commenting/FunctionCommentUnitTest.inc.fixed

+30
Original file line numberDiff line numberDiff line change
@@ -489,3 +489,33 @@ class SpacingAfter {
489489
*/
490490
public function multipleLinesSomeEmpty() {}
491491
}
492+
493+
class HandleBlankLinesBetweenDocblockAndDeclarationWithAttributes
494+
{
495+
/**
496+
* Blank line between docblock and attribute.
497+
*
498+
* @return mixed
499+
*/
500+
#[ReturnTypeWillChange]
501+
#[
502+
503+
AnotherAttribute
504+
505+
]#[AndAThirdAsWell]
506+
public function blankLineDetectionA()
507+
{
508+
509+
}//end blankLineDetectionA()
510+
511+
/**
512+
* Blank line between attribute and function declaration.
513+
*
514+
* @return mixed
515+
*/
516+
#[ReturnTypeWillChange]
517+
public function blankLineDetectionB()
518+
{
519+
520+
}//end blankLineDetectionB()
521+
}//end class

src/Standards/PEAR/Tests/Commenting/FunctionCommentUnitTest.php

+10-5
Original file line numberDiff line numberDiff line change
@@ -75,11 +75,16 @@ public function getErrorList()
7575
364 => 1,
7676
406 => 1,
7777
417 => 1,
78-
455 => 1,
79-
464 => 1,
80-
473 => 1,
81-
485 => 1,
82-
501 => 1,
78+
456 => 1,
79+
466 => 1,
80+
474 => 1,
81+
476 => 1,
82+
486 => 1,
83+
502 => 1,
84+
521 => 1,
85+
523 => 1,
86+
533 => 1,
87+
545 => 1,
8388
];
8489

8590
}//end getErrorList()

0 commit comments

Comments
 (0)