From a8371a27405e317d24c89f447d1fd513987fb46c Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 17 Feb 2025 11:29:50 +0100 Subject: [PATCH] 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._ --- .../Commenting/FunctionCommentSniff.php | 39 ++++++++++++------- .../Commenting/FunctionCommentUnitTest.inc | 38 ++++++++++++++++++ .../FunctionCommentUnitTest.inc.fixed | 30 ++++++++++++++ .../Commenting/FunctionCommentUnitTest.php | 15 ++++--- 4 files changed, 103 insertions(+), 19 deletions(-) diff --git a/src/Standards/PEAR/Sniffs/Commenting/FunctionCommentSniff.php b/src/Standards/PEAR/Sniffs/Commenting/FunctionCommentSniff.php index ee242cf6e1..d6fe2e8253 100644 --- a/src/Standards/PEAR/Sniffs/Commenting/FunctionCommentSniff.php +++ b/src/Standards/PEAR/Sniffs/Commenting/FunctionCommentSniff.php @@ -119,33 +119,44 @@ public function process(File $phpcsFile, $stackPtr) return; } + // Check there are no blank lines in the preamble for the property, + // but ignore blank lines _within_ attributes as that's not the concern of this sniff. if ($tokens[$commentEnd]['line'] !== ($tokens[$stackPtr]['line'] - 1)) { for ($i = ($commentEnd + 1); $i < $stackPtr; $i++) { - if ($tokens[$i]['column'] !== 1) { + // Skip over the contents of attributes. + if (isset($tokens[$i]['attribute_closer']) === true) { + $i = $tokens[$i]['attribute_closer']; continue; } - if ($tokens[$i]['code'] === T_WHITESPACE - && $tokens[$i]['line'] !== $tokens[($i + 1)]['line'] + 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 ) { - $error = 'There must be no blank lines after the function comment'; - $fix = $phpcsFile->addFixableError($error, $commentEnd, 'SpacingAfter'); + continue; + } - if ($fix === true) { - $phpcsFile->fixer->beginChangeset(); + $nextNonWhitespace = $phpcsFile->findNext(T_WHITESPACE, ($i + 1), null, true); + $error = 'There must be no blank lines between the function comment and the declaration'; + $fix = $phpcsFile->addFixableError($error, $i, 'SpacingAfter'); - while ($i < $stackPtr - && $tokens[$i]['code'] === T_WHITESPACE - && $tokens[$i]['line'] !== $tokens[($i + 1)]['line'] - ) { - $phpcsFile->fixer->replaceToken($i++, ''); + if ($fix === true) { + $phpcsFile->fixer->beginChangeset(); + + for ($j = $i; $j < $nextNonWhitespace; $j++) { + if ($tokens[$j]['line'] === $tokens[$nextNonWhitespace]['line']) { + break; } - $phpcsFile->fixer->endChangeset(); + $phpcsFile->fixer->replaceToken($j, ''); } - break; + $phpcsFile->fixer->endChangeset(); } + + $i = $nextNonWhitespace; }//end for }//end if diff --git a/src/Standards/PEAR/Tests/Commenting/FunctionCommentUnitTest.inc b/src/Standards/PEAR/Tests/Commenting/FunctionCommentUnitTest.inc index a20ba3a720..9c1fc014e7 100644 --- a/src/Standards/PEAR/Tests/Commenting/FunctionCommentUnitTest.inc +++ b/src/Standards/PEAR/Tests/Commenting/FunctionCommentUnitTest.inc @@ -510,3 +510,41 @@ class SpacingAfter { public function multipleLinesSomeEmpty() {} } + +class HandleBlankLinesBetweenDocblockAndDeclarationWithAttributes +{ + /** + * Blank line between docblock and attribute. + * + * @return mixed + */ + + #[ReturnTypeWillChange] + + + + + + #[ + + AnotherAttribute + + ]#[AndAThirdAsWell] + + public function blankLineDetectionA() + { + + }//end blankLineDetectionA() + + /** + * Blank line between attribute and function declaration. + * + * @return mixed + */ + #[ReturnTypeWillChange] + + public function blankLineDetectionB() + { + + }//end blankLineDetectionB() +}//end class diff --git a/src/Standards/PEAR/Tests/Commenting/FunctionCommentUnitTest.inc.fixed b/src/Standards/PEAR/Tests/Commenting/FunctionCommentUnitTest.inc.fixed index fc6d4f7e71..f2736348be 100644 --- a/src/Standards/PEAR/Tests/Commenting/FunctionCommentUnitTest.inc.fixed +++ b/src/Standards/PEAR/Tests/Commenting/FunctionCommentUnitTest.inc.fixed @@ -489,3 +489,33 @@ class SpacingAfter { */ public function multipleLinesSomeEmpty() {} } + +class HandleBlankLinesBetweenDocblockAndDeclarationWithAttributes +{ + /** + * Blank line between docblock and attribute. + * + * @return mixed + */ + #[ReturnTypeWillChange] + #[ + + AnotherAttribute + + ]#[AndAThirdAsWell] + public function blankLineDetectionA() + { + + }//end blankLineDetectionA() + + /** + * Blank line between attribute and function declaration. + * + * @return mixed + */ + #[ReturnTypeWillChange] + public function blankLineDetectionB() + { + + }//end blankLineDetectionB() +}//end class diff --git a/src/Standards/PEAR/Tests/Commenting/FunctionCommentUnitTest.php b/src/Standards/PEAR/Tests/Commenting/FunctionCommentUnitTest.php index 62863be625..7dc2e4ae67 100644 --- a/src/Standards/PEAR/Tests/Commenting/FunctionCommentUnitTest.php +++ b/src/Standards/PEAR/Tests/Commenting/FunctionCommentUnitTest.php @@ -75,11 +75,16 @@ public function getErrorList() 364 => 1, 406 => 1, 417 => 1, - 455 => 1, - 464 => 1, - 473 => 1, - 485 => 1, - 501 => 1, + 456 => 1, + 466 => 1, + 474 => 1, + 476 => 1, + 486 => 1, + 502 => 1, + 521 => 1, + 523 => 1, + 533 => 1, + 545 => 1, ]; }//end getErrorList()