Skip to content

Fix Scope detection false positives - #499

Merged
calebdw merged 9 commits into
driftingly:mainfrom
nikspyratos:fix/scope-detection-false-positives
Apr 20, 2026
Merged

Fix Scope detection false positives#499
calebdw merged 9 commits into
driftingly:mainfrom
nikspyratos:fix/scope-detection-false-positives

Conversation

@nikspyratos

@nikspyratos nikspyratos commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

Closes #498

TL;DR expands scope checking rule analyzers to check for more markers of a local scope:

  1. Method name starts with scope + uppercase char (existing)
  2. First parameter is either untyped or typed as Illuminate\Database\Eloquent\Builder(new)
  3. Return type is absent, void, or Builder (new)

Existing behaviour to check for the method name being only scope (#380) is preserved

In draft until I test this in a real repo soon, unless y'all think it's fine with the test coverage


Ah, didn't notice #409 tries to cover the same ground, as there wasn't an issue for it. Mine covers both ScopeNamedClassMethodToScopeAttributedClassMethodRector and MakeModelAttributesAndScopesProtectedRector, take your pick on which approach is preferred. There are some differences though:

Differences
  • Name pattern check: PR 409 uses /^scope.+$/ which matches names like scopedItems. This PR checks for scope + uppercase char at position 5, which is more precise to Laravel's convention (only scopeX where X is uppercase is a valid scope).

  • Type validation strategy: PR 409 checks "if return type OR first param type is missing, assume scope". This branch checks each item independently: an untyped first param is accepted, but a non-Builder/non-void return type still disqualifies the method.

  • Additional fix: This branch also fixes a pre-existing bug where str_replace('scope', '', $name) could remove multiple occurrences of "scope" in a method name (e.g. scopeRescope → re instead of rescope). Replaced with substr($name, 5).

@nikspyratos
nikspyratos marked this pull request as draft April 16, 2026 05:57
Comment thread src/Rector/ClassMethod/MakeModelAttributesAndScopesProtectedRector.php Outdated
Comment thread src/NodeAnalyzer/ScopeAnalyzer.php Outdated
Comment thread src/NodeAnalyzer/ScopeAnalyzer.php Outdated
Comment thread src/NodeAnalyzer/ScopeAnalyzer.php Outdated
@nikspyratos
nikspyratos marked this pull request as ready for review April 19, 2026 08:46
@nikspyratos

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback! Have updated per the suggestions, let me know if that suffices to resolve the comments.

Got around to testing in an affected project and it all worked fine for me, so marking as ready for review properly.

@calebdw
calebdw enabled auto-merge (squash) April 20, 2026 12:33
@calebdw
calebdw merged commit 0defa4d into driftingly:main Apr 20, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Attributed scope methods should check that the method is actually a scope

2 participants