Skip to content

Commit efcb4e4

Browse files
jrfnljrfnl
committed
Squiz/FunctionSpacing: bug fix for attribute handling
The `Squiz.WhiteSpace.FunctionSpacing` sniff is supposed to check the number of blank lines before (and after) a function declaration, while taking docblocks and attributes attached to the function into account. I.e. if there is a docblock and/or attributes, the blank lines _above_ those will be checked. Additionally, the sniff has a separate error code `BeforeFirst` for the number of blank lines before a first function in a class and the expected number of blank lines for the first function in a class may also be configured to be different than the "normal" number of blank lines before a function/between methods. Now, while the sniff does take attributes into account, it does so incorrectly: 1. It doesn't allow for attributes _before_ a docblock, even though this is perfectly valid PHP and the attributes still belong with the function. 2. It doesn't allow for multiple attributes on the same line. Again, this is perfectly valid PHP. The effect of this bug can be seen by checking the new tests without the fix: 1. The "Correct" functions will also be flagged as incorrect as the sniff gets confused by multiple attributes on the same line. 2. The set of tests with the attributes before the docblock will be flagged with the `Before` error code instead of `BeforeFirst` as the functions will not be recognized as the first in class due to the attributes before the docblock. 3. The fixer will make incorrect fixes. Fixed now by removing the presumption about the _order_ of docblocks vs attributes and stabilizing the "current line" determination. Includes tests.
1 parent f117c2b commit efcb4e4

File tree

4 files changed

+240
-20
lines changed

4 files changed

+240
-20
lines changed

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

Lines changed: 39 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -116,14 +116,23 @@ public function process(File $phpcsFile, $stackPtr)
116116

117117
$prev = $phpcsFile->findPrevious($ignore, ($stackPtr - 1), null, true);
118118

119-
while ($tokens[$prev]['code'] === T_ATTRIBUTE_END) {
120-
// Skip past function attributes.
121-
$prev = $phpcsFile->findPrevious($ignore, ($tokens[$prev]['attribute_opener'] - 1), null, true);
122-
}
119+
// Skip past function docblocks and attributes.
120+
for ($prev; $prev > 0; $prev--) {
121+
if ($tokens[$prev]['code'] === T_WHITESPACE) {
122+
continue;
123+
}
124+
125+
if ($tokens[$prev]['code'] === T_DOC_COMMENT_CLOSE_TAG) {
126+
$prev = $tokens[$prev]['comment_opener'];
127+
continue;
128+
}
123129

124-
if ($tokens[$prev]['code'] === T_DOC_COMMENT_CLOSE_TAG) {
125-
// Skip past function docblocks.
126-
$prev = $phpcsFile->findPrevious($ignore, ($tokens[$prev]['comment_opener'] - 1), null, true);
130+
if ($tokens[$prev]['code'] === T_ATTRIBUTE_END) {
131+
$prev = $tokens[$prev]['attribute_opener'];
132+
continue;
133+
}
134+
135+
break;
127136
}
128137

129138
if ($tokens[$prev]['code'] === T_OPEN_CURLY_BRACKET) {
@@ -253,20 +262,30 @@ public function process(File $phpcsFile, $stackPtr)
253262
return;
254263
}
255264

256-
while ($tokens[$prevContent]['code'] === T_ATTRIBUTE_END
257-
&& $tokens[$prevContent]['line'] === ($currentLine - 1)
258-
) {
259-
// Account for function attributes.
260-
$currentLine = $tokens[$tokens[$prevContent]['attribute_opener']]['line'];
261-
$prevContent = $phpcsFile->findPrevious(T_WHITESPACE, ($tokens[$prevContent]['attribute_opener'] - 1), null, true);
262-
}
265+
// Skip past function docblocks and attributes.
266+
for ($prevContent; $prevContent > 0; $prevContent--) {
267+
if ($tokens[$prevContent]['code'] === T_WHITESPACE) {
268+
continue;
269+
}
263270

264-
if ($tokens[$prevContent]['code'] === T_DOC_COMMENT_CLOSE_TAG
265-
&& $tokens[$prevContent]['line'] === ($currentLine - 1)
266-
) {
267-
// Account for function comments.
268-
$prevContent = $phpcsFile->findPrevious(T_WHITESPACE, ($tokens[$prevContent]['comment_opener'] - 1), null, true);
269-
}
271+
if ($tokens[$prevContent]['code'] === T_DOC_COMMENT_CLOSE_TAG
272+
&& $tokens[$prevContent]['line'] >= ($currentLine - 1)
273+
) {
274+
$currentLine = $tokens[$tokens[$prevContent]['comment_opener']]['line'];
275+
$prevContent = $tokens[$prevContent]['comment_opener'];
276+
continue;
277+
}
278+
279+
if ($tokens[$prevContent]['code'] === T_ATTRIBUTE_END
280+
&& $tokens[$prevContent]['line'] >= ($currentLine - 1)
281+
) {
282+
$currentLine = $tokens[$tokens[$prevContent]['attribute_opener']]['line'];
283+
$prevContent = $tokens[$prevContent]['attribute_opener'];
284+
continue;
285+
}
286+
287+
break;
288+
}//end for
270289

271290
// Before we throw an error, check that we are not throwing an error
272291
// for another function. We don't want to error for no blank lines after

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

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -593,3 +593,102 @@ function a() {
593593
*/
594594
function b() {
595595
}
596+
597+
598+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingBeforeFirst 1
599+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingAfterLast 0
600+
601+
class DocblockFollowedByAttributesCorrectSpacing {
602+
603+
/**
604+
* No error.
605+
*/
606+
#[AttributesShouldBeJumpedOver]
607+
#[
608+
ASecondAttributesShouldBeJumpedOverToo
609+
]#[AndAThirdAsWell]
610+
function FirstFunction()
611+
{
612+
// Code
613+
}
614+
}
615+
616+
class DocblockFollowedByAttributesTooMuchSpacing {
617+
618+
619+
620+
/**
621+
* Docblock.
622+
*/
623+
#[AttributesShouldBeJumpedOver]
624+
#[
625+
ASecondAttributesShouldBeJumpedOverToo
626+
]#[AndAThirdAsWell]
627+
function FirstFunction()
628+
{
629+
// Code
630+
}
631+
}
632+
633+
class DocblockFollowedByAttributesTooLittleSpacing {
634+
/**
635+
* Docblock.
636+
*/
637+
#[AttributesShouldBeJumpedOver]
638+
#[
639+
ASecondAttributesShouldBeJumpedOverToo
640+
]#[AndAThirdAsWell]
641+
function FirstFunction()
642+
{
643+
// Code
644+
}
645+
}
646+
647+
class DocblockPrecededByAttributesCorrectSpacing {
648+
649+
#[AttributesShouldBeJumpedOver]
650+
#[
651+
ASecondAttributesShouldBeJumpedOverToo
652+
]#[AndAThirdAsWell]
653+
/**
654+
* No error.
655+
*/
656+
function FirstFunction()
657+
{
658+
// Code
659+
}
660+
}
661+
662+
class DocblockPrecededByAttributesTooMuchSpacing {
663+
664+
665+
#[AttributesShouldBeJumpedOver]
666+
#[
667+
ASecondAttributesShouldBeJumpedOverToo
668+
]#[AndAThirdAsWell]
669+
/**
670+
* Docblock.
671+
*/
672+
function FirstFunction()
673+
{
674+
// Code
675+
}
676+
}
677+
678+
class DocblockPrecededByAttributesTooLittleSpacing {
679+
#[AttributesShouldBeJumpedOver]
680+
#[
681+
ASecondAttributesShouldBeJumpedOverToo
682+
]#[AndAThirdAsWell]
683+
/**
684+
* Docblock.
685+
*/
686+
function FirstFunction()
687+
{
688+
// Code
689+
}
690+
}
691+
692+
// Reset properties to their default value.
693+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingBeforeFirst 2
694+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingAfterLast 2

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

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -685,3 +685,101 @@ function a() {
685685
*/
686686
function b() {
687687
}
688+
689+
690+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingBeforeFirst 1
691+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingAfterLast 0
692+
693+
class DocblockFollowedByAttributesCorrectSpacing {
694+
695+
/**
696+
* No error.
697+
*/
698+
#[AttributesShouldBeJumpedOver]
699+
#[
700+
ASecondAttributesShouldBeJumpedOverToo
701+
]#[AndAThirdAsWell]
702+
function FirstFunction()
703+
{
704+
// Code
705+
}
706+
}
707+
708+
class DocblockFollowedByAttributesTooMuchSpacing {
709+
710+
/**
711+
* Docblock.
712+
*/
713+
#[AttributesShouldBeJumpedOver]
714+
#[
715+
ASecondAttributesShouldBeJumpedOverToo
716+
]#[AndAThirdAsWell]
717+
function FirstFunction()
718+
{
719+
// Code
720+
}
721+
}
722+
723+
class DocblockFollowedByAttributesTooLittleSpacing {
724+
725+
/**
726+
* Docblock.
727+
*/
728+
#[AttributesShouldBeJumpedOver]
729+
#[
730+
ASecondAttributesShouldBeJumpedOverToo
731+
]#[AndAThirdAsWell]
732+
function FirstFunction()
733+
{
734+
// Code
735+
}
736+
}
737+
738+
class DocblockPrecededByAttributesCorrectSpacing {
739+
740+
#[AttributesShouldBeJumpedOver]
741+
#[
742+
ASecondAttributesShouldBeJumpedOverToo
743+
]#[AndAThirdAsWell]
744+
/**
745+
* No error.
746+
*/
747+
function FirstFunction()
748+
{
749+
// Code
750+
}
751+
}
752+
753+
class DocblockPrecededByAttributesTooMuchSpacing {
754+
755+
#[AttributesShouldBeJumpedOver]
756+
#[
757+
ASecondAttributesShouldBeJumpedOverToo
758+
]#[AndAThirdAsWell]
759+
/**
760+
* Docblock.
761+
*/
762+
function FirstFunction()
763+
{
764+
// Code
765+
}
766+
}
767+
768+
class DocblockPrecededByAttributesTooLittleSpacing {
769+
770+
#[AttributesShouldBeJumpedOver]
771+
#[
772+
ASecondAttributesShouldBeJumpedOverToo
773+
]#[AndAThirdAsWell]
774+
/**
775+
* Docblock.
776+
*/
777+
function FirstFunction()
778+
{
779+
// Code
780+
}
781+
}
782+
783+
// Reset properties to their default value.
784+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingBeforeFirst 2
785+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingAfterLast 2

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,10 @@ public function getErrorList($testFile='')
103103
580 => 2,
104104
583 => 3,
105105
591 => 1,
106+
627 => 1,
107+
641 => 1,
108+
672 => 1,
109+
686 => 1,
106110
];
107111

108112
case 'FunctionSpacingUnitTest.2.inc':

0 commit comments

Comments
 (0)