Skip to content

Commit 51f7282

Browse files
jrfnljrfnl
committed
Squiz/FunctionSpacing: iterate on previous fix for attributes
Turns out the "silence the "before" error if the thing before was another function" functionality still wasn't working correctly. And if there is a blank line between the first thing before the first "preamble" for the function and the declaration itself, the sniff would also disregard the "preamble" and insert the blank lines straight above the function declaration. Fixed now. Includes tests proving the bug. Includes one minor change in the existing tests, where the recognition of "the thing before is also a function" no longer works. This is a specific edge case when there are two functions declared on the same line and, in my opinion, this tiny regression is a valid trade-off for correctly handling functions with attributes, which is much more commonly found in code.
1 parent 6504c29 commit 51f7282

File tree

4 files changed

+126
-68
lines changed

4 files changed

+126
-68
lines changed

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

+49-67
Original file line numberDiff line numberDiff line change
@@ -116,23 +116,36 @@ public function process(File $phpcsFile, $stackPtr)
116116

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

119-
// Skip past function docblocks and attributes.
120-
for ($prev; $prev > 0; $prev--) {
121-
if ($tokens[$prev]['code'] === T_WHITESPACE) {
119+
$startOfDeclarationLine = $phpcsFile->findNext(T_WHITESPACE, ($prev + 1), null, true);
120+
for ($i = $startOfDeclarationLine; $i >= 0; $i--) {
121+
if ($tokens[$i]['line'] === $tokens[$startOfDeclarationLine]['line']) {
122+
$startOfDeclarationLine = $i;
122123
continue;
123124
}
124125

125-
if ($tokens[$prev]['code'] === T_DOC_COMMENT_CLOSE_TAG) {
126-
$prev = $tokens[$prev]['comment_opener'];
127-
continue;
128-
}
126+
break;
127+
}
129128

130-
if ($tokens[$prev]['code'] === T_ATTRIBUTE_END) {
131-
$prev = $tokens[$prev]['attribute_opener'];
132-
continue;
133-
}
129+
// Skip past function docblocks and attributes.
130+
$prev = $startOfDeclarationLine;
131+
if ($startOfDeclarationLine > 0) {
132+
for ($prev = ($startOfDeclarationLine - 1); $prev > 0; $prev--) {
133+
if ($tokens[$prev]['code'] === T_WHITESPACE) {
134+
continue;
135+
}
134136

135-
break;
137+
if ($tokens[$prev]['code'] === T_DOC_COMMENT_CLOSE_TAG) {
138+
$prev = $tokens[$prev]['comment_opener'];
139+
continue;
140+
}
141+
142+
if ($tokens[$prev]['code'] === T_ATTRIBUTE_END) {
143+
$prev = $tokens[$prev]['attribute_opener'];
144+
continue;
145+
}
146+
147+
break;
148+
}
136149
}
137150

138151
if ($tokens[$prev]['code'] === T_OPEN_CURLY_BRACKET) {
@@ -233,9 +246,11 @@ public function process(File $phpcsFile, $stackPtr)
233246
before the function.
234247
*/
235248

249+
$startOfPreamble = $phpcsFile->findNext(T_WHITESPACE, ($prev + 1), null, true);
250+
236251
$prevLineToken = null;
237-
for ($i = $stackPtr; $i >= 0; $i--) {
238-
if ($tokens[$i]['line'] === $tokens[$stackPtr]['line']) {
252+
for ($i = $startOfPreamble; $i >= 0; $i--) {
253+
if ($tokens[$i]['line'] === $tokens[$startOfPreamble]['line']) {
239254
continue;
240255
}
241256

@@ -250,43 +265,15 @@ public function process(File $phpcsFile, $stackPtr)
250265
$prevContent = 0;
251266
$prevLineToken = 0;
252267
} else {
253-
$currentLine = $tokens[$stackPtr]['line'];
254-
255-
$prevContent = $phpcsFile->findPrevious(T_WHITESPACE, $prevLineToken, null, true);
256-
257-
if ($tokens[$prevContent]['code'] === T_COMMENT
258-
|| isset(Tokens::$phpcsCommentTokens[$tokens[$prevContent]['code']]) === true
268+
$firstBefore = $phpcsFile->findPrevious(T_WHITESPACE, ($startOfDeclarationLine - 1), null, true);
269+
if ($tokens[$firstBefore]['code'] === T_COMMENT
270+
|| isset(Tokens::$phpcsCommentTokens[$tokens[$firstBefore]['code']]) === true
259271
) {
260272
// Ignore comments as they can have different spacing rules, and this
261273
// isn't a proper function comment anyway.
262274
return;
263275
}
264276

265-
// Skip past function docblocks and attributes.
266-
for ($prevContent; $prevContent > 0; $prevContent--) {
267-
if ($tokens[$prevContent]['code'] === T_WHITESPACE) {
268-
continue;
269-
}
270-
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
289-
290277
// Before we throw an error, check that we are not throwing an error
291278
// for another function. We don't want to error for no blank lines after
292279
// the previous function and no blank lines before this one as well.
@@ -297,38 +284,33 @@ public function process(File $phpcsFile, $stackPtr)
297284
$stopAt = array_pop($conditions);
298285
}
299286

300-
$prevLineToken = $prevContent;
301-
$prevLine = ($tokens[$prevContent]['line'] - 1);
302-
$i = ($stackPtr - 1);
303-
$foundLines = 0;
287+
$currentLine = $tokens[$startOfPreamble]['line'];
288+
$prevContent = $prev;
289+
$prevLine = ($tokens[$prevContent]['line'] - 1);
290+
$foundLines = ($currentLine - $tokens[$prevContent]['line'] - 1);
291+
292+
for ($i = $prevContent; $i > $stopAt; $i--) {
293+
if ($tokens[$i]['code'] === T_CLOSE_CURLY_BRACKET) {
294+
if (isset($tokens[$i]['scope_condition']) === true
295+
&& $tokens[$tokens[$i]['scope_condition']]['code'] === T_FUNCTION
296+
) {
297+
// Found a previous function.
298+
return;
299+
} else {
300+
break;
301+
}
302+
}
304303

305-
while ($currentLine !== $prevLine && $currentLine > 1 && $i > $stopAt) {
306304
if ($tokens[$i]['code'] === T_FUNCTION) {
307305
// Found another interface or abstract function.
308306
return;
309307
}
310308

311-
if ($tokens[$i]['code'] === T_CLOSE_CURLY_BRACKET
312-
&& $tokens[$tokens[$i]['scope_condition']]['code'] === T_FUNCTION
313-
) {
314-
// Found a previous function.
315-
return;
316-
}
317-
318309
$currentLine = $tokens[$i]['line'];
319310
if ($currentLine === $prevLine) {
320311
break;
321312
}
322-
323-
if ($tokens[($i - 1)]['line'] < $currentLine && $tokens[($i + 1)]['line'] > $currentLine) {
324-
// This token is on a line by itself. If it is whitespace, the line is empty.
325-
if ($tokens[$i]['code'] === T_WHITESPACE) {
326-
$foundLines++;
327-
}
328-
}
329-
330-
$i--;
331-
}//end while
313+
}//end for
332314
}//end if
333315

334316
$requiredSpacing = $this->spacing;

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

+34
Original file line numberDiff line numberDiff line change
@@ -692,3 +692,37 @@ class DocblockPrecededByAttributesTooLittleSpacing {
692692
// Reset properties to their default value.
693693
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingBeforeFirst 2
694694
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingAfterLast 2
695+
696+
class SilenceBeforeErrorIfPreviousThingWasAFunctionBug
697+
{
698+
/**
699+
* Docblock.
700+
*/
701+
702+
#[ReturnTypeWillChange]
703+
704+
705+
706+
707+
708+
#[
709+
710+
AnotherAttribute
711+
712+
]#[AndAThirdAsWell]
713+
714+
public function blankLineDetectionA()
715+
{
716+
717+
}//end blankLineDetectionA()
718+
719+
/**
720+
* Docblock.
721+
*/
722+
#[ReturnTypeWillChange]
723+
724+
public function blankLineDetectionB()
725+
{
726+
727+
}//end blankLineDetectionB()
728+
}//end class

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

+39
Original file line numberDiff line numberDiff line change
@@ -783,3 +783,42 @@ class DocblockPrecededByAttributesTooLittleSpacing {
783783
// Reset properties to their default value.
784784
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingBeforeFirst 2
785785
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingAfterLast 2
786+
787+
class SilenceBeforeErrorIfPreviousThingWasAFunctionBug
788+
{
789+
790+
791+
/**
792+
* Docblock.
793+
*/
794+
795+
#[ReturnTypeWillChange]
796+
797+
798+
799+
800+
801+
#[
802+
803+
AnotherAttribute
804+
805+
]#[AndAThirdAsWell]
806+
807+
public function blankLineDetectionA()
808+
{
809+
810+
}//end blankLineDetectionA()
811+
812+
813+
/**
814+
* Docblock.
815+
*/
816+
#[ReturnTypeWillChange]
817+
818+
public function blankLineDetectionB()
819+
{
820+
821+
}//end blankLineDetectionB()
822+
823+
824+
}//end class

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

+4-1
Original file line numberDiff line numberDiff line change
@@ -101,12 +101,15 @@ public function getErrorList($testFile='')
101101
560 => 1,
102102
566 => 1,
103103
580 => 2,
104-
583 => 3,
104+
583 => 4,
105105
591 => 1,
106106
627 => 1,
107107
641 => 1,
108108
672 => 1,
109109
686 => 1,
110+
714 => 1,
111+
717 => 1,
112+
727 => 1,
110113
];
111114

112115
case 'FunctionSpacingUnitTest.2.inc':

0 commit comments

Comments
 (0)