Skip to content

Commit 447b0b5

Browse files
committed
Squiz/StaticThisUsage: always skip nested anonymous scope
As things were, the `Squiz.Scope.StaticThisUsage` sniff already skipped over the body of anonymous classes when found within the body of a static OO method, to prevent false positives related to `$this` when it refers to the anonymous class. However, the sniff did not skip over the body of an anonymous function (closure). This is wrong, as anonymous functions can be bound to an object, in which case the closure has access to that object via `$this`. However, whether the object is actually bound or not, is irrelevant. The closure has its own (closed) scope and doesn't have access to variables from outside the closure, so any usages of `$this` _within_ the closure would not be the "static this usage" this sniff is looking for anyway. With that in mind, the sniff should skip over the body of closures, same as it skips over the body of anonymous classes. Having said that, I also believe that the sniff should explicitly examine `static` closures for the use of `$this`, while it currently doesn't (false negative), but that's another matter. Fixes 1357 Related to previous iterations: * squizlabs/PHP_CodeSniffer 1747 - solved via 8ad3826 * squizlabs/PHP_CodeSniffer 2725
1 parent 2108e8b commit 447b0b5

File tree

3 files changed

+27
-3
lines changed

3 files changed

+27
-3
lines changed

src/Standards/Squiz/Sniffs/Scope/StaticThisUsageSniff.php

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,12 +86,15 @@ private function checkThisUsage(File $phpcsFile, int $next, int $end)
8686
$tokens = $phpcsFile->getTokens();
8787

8888
do {
89-
$next = $phpcsFile->findNext([T_VARIABLE, T_ANON_CLASS], ($next + 1), $end);
89+
$next = $phpcsFile->findNext([T_VARIABLE, T_CLOSURE, T_ANON_CLASS], ($next + 1), $end);
9090
if ($next === false) {
9191
continue;
9292
}
9393

94-
if ($tokens[$next]['code'] === T_ANON_CLASS) {
94+
if (($tokens[$next]['code'] === T_ANON_CLASS
95+
|| $tokens[$next]['code'] === T_CLOSURE)
96+
&& isset($tokens[$next]['scope_opener']) === true
97+
) {
9598
$this->checkThisUsage($phpcsFile, $next, $tokens[$next]['scope_opener']);
9699
$next = $tokens[$next]['scope_closer'];
97100
continue;

src/Standards/Squiz/Tests/Scope/StaticThisUsageUnitTest.inc

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,3 +125,22 @@ enum MyEnum {
125125
$this->doSomething();
126126
}
127127
}
128+
129+
class NestedScopesShouldBeSkipped
130+
{
131+
public static function examineMe() {
132+
$this->shouldBeFlagged();
133+
134+
function nestedFunction() {
135+
$this->nestedFunctionsDoNotHaveAccessToThis();
136+
}
137+
138+
\Closure::bind(
139+
function ($this) { // Cannot use $this as a param, so this should still be flagged.
140+
$this->x = 1; // This is fine. $this related to the Closure class, not to the NestedScopesShouldBeSkipped class.
141+
},
142+
new Target(),
143+
Target::class,
144+
)();
145+
}
146+
}

src/Standards/Squiz/Tests/Scope/StaticThisUsageUnitTest.php

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,14 +37,16 @@ public function getErrorList()
3737
9 => 1,
3838
14 => 1,
3939
20 => 1,
40-
41 => 1,
4140
61 => 1,
4241
69 => 1,
4342
76 => 1,
4443
80 => 1,
4544
84 => 1,
4645
99 => 1,
4746
125 => 1,
47+
132 => 1,
48+
135 => 1,
49+
139 => 1,
4850
];
4951
}
5052

0 commit comments

Comments
 (0)