Skip to content

Commit 0e8478f

Browse files
jrfnljrfnl
committed
Squiz/FunctionSpacing: selectively ignore nested functions
The `Squiz.WhiteSpace.FunctionSpacing` sniff enforces a defined number of blank lines before and after a function declaration. However, when a function is declared nested within another function, this may conflict with rules about superfluous blank lines within a function causing a fixer conflict. This commit fixes this conflict by ignoring an error for nested function declarations when the required spacing for before/after in the specific situation being examined would be more than 1 line. Includes tests. Includes some adjustments to pre-existing tests. This doesn't diminish the value of those pre-existing tests. These tests were introduced via c3a7ec5 and 07ccaf2 to address squizlabs/PHP_CodeSniffer 2406, which was about the sniff recognizing nested functions as (potentially) first or last and applying the correct spacing based on `spacingBeforeFirst`/`spacingAfterLast` properties in those cases. The current fix doesn't "undo" that change and the error codes for nested functions are still correctly identifying first/last functions.
1 parent bfdc6ea commit 0e8478f

File tree

4 files changed

+157
-22
lines changed

4 files changed

+157
-22
lines changed

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

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,18 @@ public function process(File $phpcsFile, $stackPtr)
102102
$this->spacingBeforeFirst = (int) $this->spacingBeforeFirst;
103103
$this->spacingAfterLast = (int) $this->spacingAfterLast;
104104

105+
// Check for functions nested within other functions.
106+
// Different rules for allowed number of consecutive blank lines may apply.
107+
$isNested = false;
108+
if (isset($tokens[$stackPtr]['conditions']) === true) {
109+
$conditions = $tokens[$stackPtr]['conditions'];
110+
$lastCondition = end($conditions);
111+
112+
if ($lastCondition === T_FUNCTION || $lastCondition === T_CLOSURE) {
113+
$isNested = true;
114+
}
115+
}
116+
105117
if (isset($tokens[$stackPtr]['scope_closer']) === false) {
106118
// Must be an interface method, so the closer is the semicolon.
107119
$closer = $phpcsFile->findNext(T_SEMICOLON, $stackPtr);
@@ -213,7 +225,9 @@ public function process(File $phpcsFile, $stackPtr)
213225
$phpcsFile->recordMetric($stackPtr, 'Function spacing after', $foundLines);
214226
}
215227

216-
if ($foundLines !== $requiredSpacing) {
228+
if ($foundLines !== $requiredSpacing
229+
&& ($isNested === false || $requiredSpacing < 2)
230+
) {
217231
$error = 'Expected %s blank line';
218232
if ($requiredSpacing !== 1) {
219233
$error .= 's';
@@ -324,7 +338,9 @@ public function process(File $phpcsFile, $stackPtr)
324338
$phpcsFile->recordMetric($stackPtr, 'Function spacing before', $foundLines);
325339
}
326340

327-
if ($foundLines !== $requiredSpacing) {
341+
if ($foundLines !== $requiredSpacing
342+
&& ($isNested === false || $requiredSpacing < 2)
343+
) {
328344
$error = 'Expected %s blank line';
329345
if ($requiredSpacing !== 1) {
330346
$error .= 's';

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

Lines changed: 65 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -452,7 +452,7 @@ $anon_class = new class() {
452452

453453
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacing 2
454454
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingBeforeFirst 1
455-
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingAfterLast 3
455+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingAfterLast 1
456456

457457
class MyClass {
458458
function a(){}
@@ -726,3 +726,67 @@ class SilenceBeforeErrorIfPreviousThingWasAFunctionBug
726726

727727
}//end blankLineDetectionB()
728728
}//end class
729+
730+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacing 1
731+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingBeforeFirst 0
732+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingAfterLast 0
733+
function HandleNestedFunctionDeclarations() {
734+
735+
function ShouldBeFixedTooMuchSpaceBeforeFirst() {
736+
// ...
737+
}
738+
739+
740+
$foo = 10;
741+
function ShouldBeFixedTooLittleSpace() {
742+
// ...
743+
}
744+
$a = true;
745+
746+
$foo = 10;
747+
748+
749+
function ShouldBeFixedTooMuchSpace() {
750+
// ...
751+
}
752+
753+
754+
$a = true;
755+
756+
757+
function ShouldBeFixedTooMuchSpaceAfterLast() {
758+
// ...
759+
}
760+
761+
}
762+
763+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacing 1
764+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingBeforeFirst 2
765+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingAfterLast 2
766+
function SelectivelyIgnoreNestedFunctionDeclarations() {
767+
function BeforeShouldNotBeFixedAsSpacingBeforeFirstMoreThan1() {
768+
// ...
769+
}
770+
771+
function AfterShouldNotBeFixedAsSpacingAfterLastMoreThan1() {
772+
// ...
773+
}
774+
}
775+
776+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacing 2
777+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingBeforeFirst 1
778+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingAfterLast 1
779+
$appliesToFunctionsNestedInClosureToo = function() {
780+
function BeforeShouldBeFixedButNotAfter() {
781+
// ...
782+
}
783+
784+
function AfterShouldBeFixedButNotBefore() {
785+
// ...
786+
}
787+
};
788+
789+
// Reset properties to their default value.
790+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacing 2
791+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingBeforeFirst 2
792+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingAfterLast 2

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

Lines changed: 63 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -503,7 +503,7 @@ $anon_class = new class() {
503503

504504
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacing 2
505505
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingBeforeFirst 1
506-
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingAfterLast 3
506+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingAfterLast 1
507507

508508
class MyClass {
509509

@@ -515,23 +515,15 @@ class MyClass {
515515

516516
function c(){}
517517

518-
519-
520518
}
521519

522520
$bar = function () {
523521
{
524522

525523
function a(){}
526-
527-
528524
function b(){}
529-
530-
531525
function c(){}
532526

533-
534-
535527
}
536528
};
537529
class Foo {
@@ -541,8 +533,6 @@ class Foo {
541533

542534
public function baz() {}
543535

544-
545-
546536
};
547537
}
548538

@@ -551,20 +541,14 @@ class Foo {
551541

552542
function forbidden() {}
553543

554-
555-
556544
}
557545

558-
559-
560546
}
561547

562548
if (function_exists('foo') === false) {
563549

564550
function foo(){}
565551

566-
567-
568552
}
569553

570554
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacing 0
@@ -822,3 +806,65 @@ class SilenceBeforeErrorIfPreviousThingWasAFunctionBug
822806

823807

824808
}//end class
809+
810+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacing 1
811+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingBeforeFirst 0
812+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingAfterLast 0
813+
function HandleNestedFunctionDeclarations() {
814+
function ShouldBeFixedTooMuchSpaceBeforeFirst() {
815+
// ...
816+
}
817+
818+
$foo = 10;
819+
820+
function ShouldBeFixedTooLittleSpace() {
821+
// ...
822+
}
823+
824+
$a = true;
825+
826+
$foo = 10;
827+
828+
function ShouldBeFixedTooMuchSpace() {
829+
// ...
830+
}
831+
832+
$a = true;
833+
834+
function ShouldBeFixedTooMuchSpaceAfterLast() {
835+
// ...
836+
}
837+
}
838+
839+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacing 1
840+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingBeforeFirst 2
841+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingAfterLast 2
842+
function SelectivelyIgnoreNestedFunctionDeclarations() {
843+
function BeforeShouldNotBeFixedAsSpacingBeforeFirstMoreThan1() {
844+
// ...
845+
}
846+
847+
function AfterShouldNotBeFixedAsSpacingAfterLastMoreThan1() {
848+
// ...
849+
}
850+
}
851+
852+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacing 2
853+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingBeforeFirst 1
854+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingAfterLast 1
855+
$appliesToFunctionsNestedInClosureToo = function() {
856+
857+
function BeforeShouldBeFixedButNotAfter() {
858+
// ...
859+
}
860+
861+
function AfterShouldBeFixedButNotBefore() {
862+
// ...
863+
}
864+
865+
};
866+
867+
// Reset properties to their default value.
868+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacing 2
869+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingBeforeFirst 2
870+
// phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingAfterLast 2

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

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,7 @@ public function getErrorList($testFile='')
8383
458 => 2,
8484
459 => 1,
8585
460 => 1,
86-
465 => 2,
87-
466 => 1,
86+
465 => 1,
8887
467 => 1,
8988
471 => 1,
9089
473 => 2,
@@ -110,6 +109,16 @@ public function getErrorList($testFile='')
110109
714 => 1,
111110
717 => 1,
112111
727 => 1,
112+
735 => 1,
113+
737 => 1,
114+
741 => 1,
115+
743 => 1,
116+
749 => 1,
117+
751 => 1,
118+
757 => 1,
119+
759 => 1,
120+
780 => 1,
121+
786 => 1,
113122
];
114123

115124
case 'FunctionSpacingUnitTest.2.inc':

0 commit comments

Comments
 (0)