Squiz/StaticThisUsage: improve handling of anonymous functions#1377
Open
Squiz/StaticThisUsage: improve handling of anonymous functions#1377
Conversation
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
Open
4 tasks
rodrigoprimo
reviewed
Mar 2, 2026
Contributor
rodrigoprimo
left a comment
There was a problem hiding this comment.
Looks good to me. I just left a comment with a small fix in a docblock.
rodrigoprimo
approved these changes
Mar 3, 2026
As noted in the previous commit, closures can be declared as `static`, in which case they do not have access to `$this`. That case was so far not considered by this sniff. This commit changes the sniff to: 1. Also examine `T_CLOSURE` tokens. 2. Examine these when nested inside OO methods, as well as when found completely outside of OO structures. Note: in a future iteration it should be considered to get rid of the implementation via the `AbstractScopeSniff` as that implementation actually makes the sniff _more complicated_ instead of _less complicated_. Includes unit tests.
... for code in the sniff previously not covered by tests.
bb14224 to
2da3413
Compare
Member
Author
|
Squashed the "fix up" commit from the review into the canonical commit it belongs with. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Squiz/StaticThisUsage: always skip nested anonymous scope
As things were, the
Squiz.Scope.StaticThisUsagesniff already skipped over the body of anonymous classes when found within the body of a static OO method, to prevent false positives related to$thiswhen 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
$thiswithin 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
staticclosures for the use of$this, while it currently doesn't (false negative), but that's another matter.Fixes #1357
Related to previous iterations:
Squiz/StaticThisUsage: examine the body of static closures
As noted in the previous commit, closures can be declared as
static, in which case they do not have access to$this.That case was so far not considered by this sniff.
This commit changes the sniff to:
T_CLOSUREtokens.Note: in a future iteration it should be considered to get rid of the implementation via the
AbstractScopeSniffas that implementation actually makes the sniff more complicated instead of less complicated.Includes unit tests.
Squiz/StaticThisUsage: add extra test
... for code in the sniff previously not covered by tests.
Suggested changelog entry
$thisin non-static closures nested in OO methods.$thisinstaticclosures.