Skip to content

Commit 46fbb5d

Browse files
committed
Squiz/MemberVarSpacing: bug fix / improve parse error handling
The `Squiz.WhiteSpace.MemberVarSpacing` sniff checks the number of blank lines before a property declaration. To determine the number of blank lines before a property, it tries to find the start of the statement by: * First finding the first modifier keyword before the variable (to skip over a potential type declaration); * And then walking over the other modifiers until it finds the first one for the statement; * After that, it checks for potential docblocks and attributes and skips over those. Only after all that it checks the number of blank lines. The first step however leads to problems when, during live coding, a property would be declared without a modifier keyword. In that case, the sniff could walk back much further than it should, potentially misidentifying a modifier keyword for a function for the modifier keyword for the property. While this is an edge-case as it is not customary for properties to be declared _after_ functions, the sniff should still handle this situation correctly. Fixed by changing the logic of the sniff to stop searching earlier. Includes new test case files, both of which demonstrate the bug. Additionally, the test in the `1` file safeguards that the current behaviour of the sniff for multi-property declarations is not aversely affected by the fix.
1 parent 066c378 commit 46fbb5d

6 files changed

+49
-5
lines changed

src/Standards/Squiz/Sniffs/WhiteSpace/MemberVarSpacingSniff.php

+11-5
Original file line numberDiff line numberDiff line change
@@ -45,23 +45,29 @@ protected function processMemberVar(File $phpcsFile, $stackPtr)
4545
{
4646
$tokens = $phpcsFile->getTokens();
4747

48+
$stopPoints = [
49+
T_SEMICOLON,
50+
T_OPEN_CURLY_BRACKET,
51+
T_CLOSE_CURLY_BRACKET,
52+
];
53+
54+
$endOfPreviousStatement = $phpcsFile->findPrevious($stopPoints, ($stackPtr - 1), null, false, null, true);
55+
4856
$validPrefixes = Tokens::$methodPrefixes;
4957
$validPrefixes[] = T_VAR;
5058
$validPrefixes[] = T_READONLY;
5159

52-
$startOfStatement = $phpcsFile->findPrevious($validPrefixes, ($stackPtr - 1), null, false, null, true);
60+
$startOfStatement = $phpcsFile->findNext($validPrefixes, ($endOfPreviousStatement + 1), $stackPtr, false, null, true);
5361
if ($startOfStatement === false) {
62+
// Parse error/live coding - property without modifier. Bow out.
5463
return;
5564
}
5665

5766
$endOfStatement = $phpcsFile->findNext(T_SEMICOLON, ($stackPtr + 1), null, false, null, true);
5867

59-
$ignore = $validPrefixes;
60-
$ignore[T_WHITESPACE] = T_WHITESPACE;
61-
6268
$start = $startOfStatement;
6369
for ($prev = ($startOfStatement - 1); $prev >= 0; $prev--) {
64-
if (isset($ignore[$tokens[$prev]['code']]) === true) {
70+
if ($tokens[$prev]['code'] === T_WHITESPACE) {
6571
continue;
6672
}
6773

src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc

+6
Original file line numberDiff line numberDiff line change
@@ -379,3 +379,9 @@ class SupportReadonlyProperties {
379379
readonly bool $readonlyB;
380380
readonly private bool $readonlyPrivate;
381381
}
382+
383+
class NoPreambleMultilineDeclaration {
384+
public
385+
static
386+
int $prop = 1;
387+
}

src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc.fixed

+7
Original file line numberDiff line numberDiff line change
@@ -368,3 +368,10 @@ class SupportReadonlyProperties {
368368

369369
readonly private bool $readonlyPrivate;
370370
}
371+
372+
class NoPreambleMultilineDeclaration {
373+
374+
public
375+
static
376+
int $prop = 1;
377+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<?php
2+
3+
// Intentional parse error. Property missing modifier.
4+
// This should be the only test in this file.
5+
6+
class PropertyWithoutModifier
7+
{
8+
public static function output() {}
9+
$var = null;
10+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
<?php
2+
3+
// Intentional parse error. Property in nested anonymous class missing modifier.
4+
// This should be the only test in this file.
5+
6+
class NestedAnonClass
7+
{
8+
public function method($var1, $var2)
9+
{
10+
$anon = new class {
11+
$p1 = null;
12+
};
13+
}
14+
}

src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.php

+1
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ public function getErrorList($testFile='')
7575
378 => 1,
7676
379 => 1,
7777
380 => 1,
78+
384 => 1,
7879
];
7980

8081
default:

0 commit comments

Comments
 (0)