Skip to content

feat: add ForbidEloquentMutationInControllersRule (queue #87)#28

Merged
Goosterhof merged 1 commit into
mainfrom
queue-87-forbid-eloquent-mutation-in-controllers
Jun 1, 2026
Merged

feat: add ForbidEloquentMutationInControllersRule (queue #87)#28
Goosterhof merged 1 commit into
mainfrom
queue-87-forbid-eloquent-mutation-in-controllers

Conversation

@Goosterhof
Copy link
Copy Markdown
Contributor

Summary

New PHPStan rule ForbidEloquentMutationInControllersRule banning Eloquent persistence-API method calls on Illuminate\Database\Eloquent\Model subclasses and Illuminate\Database\Eloquent\Builder chains when the call site is inside an App\Http\Controllers\* class. Reads (find, where, get, first, paginate, pluck, count, exists, query) deliberately permitted.

  • Identifier: forbidEloquentMutationInControllers.eloquentMutationInController
  • Doctrine: ADR-0011 (Action Class Architecture) + ADR-0019 (Explicit Model Hydration)
  • Closes: war-room enforcement queue #87
  • Supersedes: consumer-side string-match Pest arch tests in kendo (backend/tests/Arch/ControllersTest.php), ublgenie + entreezuil (tests/Arch/ControllersTest.php), ISMS bridge subset (backend/tests/Architecture/ControllerCurrentUserTest.php PR CI: add Infection mutation testing gate #10, 2026-05-28). Consumer cascade is a separate dispatch the General owns post-merge; emmie + brick-inventory-orchestrator pick up coverage automatically.

Stacked on #27

Branched off queue-86-enforce-audit-transaction-scope (the branch backing PR #27, EnforceAuditTransactionScopeRule). Both rules add a new file under src/Rules/ and register a new service in extension.neon — the parent's services-block edit is load-bearing inheritance to avoid a merge-conflict race. PR base is main; will rebase onto fresh main once #27 merges. No source-symbol dependency between the two rules — purely a config-block ordering concern.

24-method blocklist (full ADR-0011 + ADR-0019 mutation surface)

save, saveOrFail, saveQuietly, update, updateOrFail, updateQuietly, delete, deleteOrFail, forceDelete, forceDeleteQuietly, destroy, create, createOrFirst, firstOrCreate, updateOrCreate, fill, forceFill, push, pushQuietly, restore, restoreQuietly, restoreOrCreate, touch, touchQuietly.

Algorithm

  1. Namespace gatestr_starts_with($scope->getNamespace(), 'App\Http\Controllers'). Sub-namespaces like kendo's App\Http\Controllers\Central\* pass naturally (regression guard: ViolationKendoCentralSubnamespace.php fixture). Future divergent-namespace consumers lift this into a controllerNamespacePrefixes parameter per the EnforceResourceDataValidatorOptInRule precedent — out of scope here.
  2. Method walkClass_ AST: iterate getMethods(), recursively walk each method body collecting MethodCall + StaticCall nodes (shared walkNodes() helper, mirrors the other four rules — Standing Concern ci: pin symfony/console to ^7 — Infection mutation gate broken by Symfony Console 8 #29 family).
  3. MethodCall — type-aware: ObjectType::isSuperTypeOf() against Model OR Builder matches the receiver type; method name in blocklist fires.
  4. StaticCallScope::resolveName() resolves the literal class Name node to FQCN; if the FQCN is a Model subtype and the method name is in the blocklist, fire. Variable class names ($class::create(...)) are out of scope.

Builder-on-query coverage (order §A7 decision)

Resolution (a) chosen — type-aware Builder coverage works cleanly. ObjectType::isSuperTypeOf() matches Builder<User> as a subtype of the unparameterized Builder cleanly, no brittle generic introspection needed. Verified empirically by ViolationBuilderUpdate.php fixture firing at the ->update([...]) line of a User::query()->where(...)->update([...]) chain. The order's resolution (b) gap (Builder out of scope, document, defer) is NOT needed.

Out of scope

  • Non-App\Http\Controllers\* namespaces — Actions/Services/Jobs/Middleware are allowed to call persistence APIs.
  • Non-Eloquent receivers — $service->save() on a custom class passes the type gate.
  • Dynamic method names ($model->{$var}()) — would need value-flow analysis.
  • Variable class names in static calls ($class::create(...)).
  • Consumer-side Pest test removal — cross-territory cascade, separate dispatches.
  • Parameterized controllerNamespacePrefixes — defer until a consumer territory needs it.

Test plan

  • 14 RuleTestCase methods — 4 compliant (read-only / action delegation / non-controller / non-Model receiver) + 10 violation (instance save/update/delete/forceDelete, static create/destroy/updateOrCreate, multi-violation-one-method, kendo sub-namespace, Builder-on-query)
  • composer test — 80 tests / 126 assertions, all green
  • composer phpstan — self-analysis at level max, clean (10 services)
  • composer format:check — Pint dry-run clean
  • composer test:coverage + composer coverage:check — line coverage 87.98% (new rule 91.01%) ≥83 gate
  • composer mutation — MSI 82%, Covered Code MSI 82% (both ≥75 gate). 7 escaped mutants on the new rule are all in the recognizable Standing Concern ci: pin symfony/console to ^7 — Infection mutation gate broken by Symfony Console 8 #29 family (walkNodes() parity), MBString-equivalent on mb_substr / mb_strrpos, and defensive ?? Model::class / ?? EnforceBuilder::class defaults — same shape as the precedent on the other four rules
  • CI gates re-run on PHP 8.4 and 8.5 matrix legs

CHANGELOG

Added entry under [Unreleased]### Added (first bullet, Keep-a-Changelog order honored). Versioning per ADR-0021: candidate Major bump (the rule will surface new errors in already-clean code where consumers have controllers calling Eloquent persistence APIs directly — especially the type-aware additional surface that the string-match shape missed: Model::create(), Model::destroy(), chained Builder mutations, *Quietly variants). Pre-cascade audit required across ISMS, kendo, emmie, entreezuil, ublgenie, brick-inventory before tagging.

🤖 Generated with Claude Code

@jasperboerhof
Copy link
Copy Markdown
Contributor

PR Reviewer · claimed

@jasperboerhof
Copy link
Copy Markdown
Contributor

PR Reviewer · 9/10 · PASS

Findings

  • none — all reviewers clean

Action

merge-ready

jasperboerhof
jasperboerhof previously approved these changes May 29, 2026
Copy link
Copy Markdown
Contributor

@jasperboerhof jasperboerhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved by /review-open-prs — review verdict is PASS. See the verdict comment for the per-reviewer breakdown.

@Goosterhof Goosterhof added the Agent Review Requested Requesting review of specialized AI review agents. label May 29, 2026
Copy link
Copy Markdown
Contributor Author

@Goosterhof Goosterhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Four findings: zero blockers, two concerns, one nit, one praise.

The rule is correct on its declared scope and the test matrix is genuinely good — type-aware receiver matching, static-call FQCN resolution, Builder-chain coverage, and a kendo sub-namespace regression guard are all exercised with line-anchored expectations. The PR body is thorough and the versioning/cascade discipline is honest. My findings are about edge cases the test matrix does not pin and one namespace-gate sharp edge. None of them block.

Concerns

1. Namespace gate has no separator boundary — App\Http\ControllersFoo false-matches. ForbidEloquentMutationInControllersRule.php:142 gates on str_starts_with($namespace, 'App\Http\Controllers') with no trailing-\ check. A sibling namespace like App\Http\ControllersSupport\* (or App\Http\ControllersTest) would be pulled into scope and start firing on Eloquent mutations that are legitimately outside the controller surface. The kendo sub-namespace case (App\Http\Controllers\Central\*) is the intended-positive and is well-tested (ViolationKendoCentralSubnamespace.php), but it works precisely because Central follows a \. The fix is cheap: match 'App\Http\Controllers' exactly OR 'App\Http\Controllers\' as prefix. This is unlikely to bite any current consumer, hence a concern not a blocker — but it's the kind of latent false-positive that surfaces only when a territory adds an adjacent namespace, and the controllerNamespacePrefixes parameterization the docblock defers (ForbidEloquentMutationInControllersRule.php:60-64) would inherit the same defect. Add a fixture: a App\Http\ControllersSupport\SomeHelper calling $user->save(), asserting zero errors.

2. firstOrCreate / createOrFirst / restoreOrCreate are read-dominant hybrids folded into the mutation blocklist without a fixture pinning intent. MUTATION_METHODS (ForbidEloquentMutationInControllersRule.php:126-129) includes firstOrCreate, createOrFirst, and restoreOrCreate. These are defensible bans — they can write — but they're semantically different from a bare create(): the common controller use is idempotent lookup-or-provision, and a consumer team will reasonably push back when the rule fires on what reads at the call site like a getter. The blocklist is the rule's contract and three of its 24 entries carry that ambiguity unguarded by a test. I'm not asking you to remove them — the ADR-0019 line is "banned application-wide" and that's a fair reading. I'm asking for one violation fixture on firstOrCreate so the decision is pinned and a future "soften this" PR has to argue against an explicit test rather than silently flip a in_array entry. The static-call test matrix only exercises create, destroy, updateOrCreate (ForbidEloquentMutationInControllersRuleTest.php:102-139); the *OrFirst/*OrCreate family is entirely uncovered.

Nit

QueryBuilder (Illuminate\Database\Query\Builder) is not in the type gate, and there's no fixture documenting the miss. matchedReceiverFqcn (ForbidEloquentMutationInControllersRule.php:276-291) matches Eloquent\Model and Eloquent\Builder only. A controller calling DB::table('users')->update([...]) or DB::table('users')->delete() returns a Query\Builder, not an Eloquent\Builder, and passes the gate clean. That's arguably fine — DB::table() raw mutations are a separate doctrine surface (closer to ForbidDatabaseManagerInActionsRule territory) and the docblock's "Out of scope" list could legitimately claim it. But right now it's an undocumented silent miss. One line in the docblock's out-of-scope block naming Illuminate\Database\Query\Builder would make the boundary deliberate rather than incidental.

Praise

The Builder-coverage decision is load-bearing and the PR resolved it correctly. The order's §A7 offered a cheap out — declare Builder out of scope and defer — and the PR took the honest path instead: ObjectType::isSuperTypeOf() matches Builder<User> as a subtype of the unparameterized Builder without unwrapping the generic, verified empirically by ViolationBuilderUpdate.php firing on the ->update([...]) line of a User::query()->where(...)->update(...) chain (ForbidEloquentMutationInControllersRuleTest.php:175-186). That closes the exact gap the consumer-side string-match Pest tests couldn't — ->update($vars) without an inline array literal — which is the whole justification for promoting this to a type-aware rule. The docblock self-fail trap from prior package PRs (backticked @phpstan-ignore examples) is also avoided here — the suppression note at ForbidEloquentMutationInControllersRule.php:93-94 references the identifier in prose, not a backticked annotation example, so self-analysis stays clean.

One process note, not a finding: the diff reads as 2264 additions because it's stacked on #27 and carries EnforceAuditTransactionScopeRule plus its 17 fixtures. The queue-#87 rule itself is ForbidEloquentMutationInControllersRule.php (370 lines), its test (202), and 14 fixtures + stubs. Merge #27 first, then this rebases down to the real footprint as the PR body states. The CHANGELOG, README table, CLAUDE.md table, and extension.neon registration are all consistent with the new rule.

This is a COMMENT-level review — no blockers, ship it once #27 lands and the two concerns get a decision (a fixture each, or an explicit "won't fix" on the namespace boundary). Verdict: solid, type-aware, well-tested; the gaps are at the edges, not the core.

@Goosterhof Goosterhof force-pushed the queue-87-forbid-eloquent-mutation-in-controllers branch from 5d6ed98 to 1a222eb Compare May 29, 2026 14:06
@Goosterhof Goosterhof changed the base branch from main to queue-86-enforce-audit-transaction-scope May 29, 2026 14:06
@Goosterhof
Copy link
Copy Markdown
Contributor Author

Restacked for v0.4.0 — re-review requested. Rebased onto the restacked #27; base retargeted to queue-86-enforce-audit-transaction-scope so this diff shows only the ForbidEloquentMutationInControllersRule (the EnforceAuditTransactionScopeRule belongs to #27 below it). Rule source + tests byte-identical to your prior approval — CHANGELOG repositioned into the fresh v0.4.0 [Unreleased] only. Merge after #27 (order: #27#28#26).

Copy link
Copy Markdown
Contributor Author

@Goosterhof Goosterhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Four findings: zero blockers, two concerns, one nit, one praise.

The rule is correct on its declared scope and the test matrix is genuinely good — type-aware receiver matching, static-call FQCN resolution, Builder-chain coverage, and a kendo sub-namespace regression guard are all exercised with line-anchored expectations. The PR body is honest about versioning and the cross-territory cascade. My findings are about edge cases the matrix does not pin and one namespace-gate sharp edge. None of them block.

Concerns

1. Namespace gate has no separator boundary — App\Http\ControllersFoo false-matches. processNode gates on str_starts_with($namespace, self::CONTROLLER_NAMESPACE_PREFIX) (ForbidEloquentMutationInControllersRule.php:198) where the constant is 'App\Http\Controllers' (:168) with no trailing-\. A sibling namespace — App\Http\ControllersSupport\*, App\Http\ControllersTest — would be pulled into scope and start firing on Eloquent mutations that are legitimately outside the controller surface. The kendo sub-namespace case (App\Http\Controllers\Central\*) is the intended positive and is well-tested (ViolationKendoCentralSubnamespace.php), but it works precisely because Central follows a \. The fix is cheap: match 'App\Http\Controllers' exactly OR 'App\Http\Controllers\' as a prefix. Unlikely to bite a current consumer — hence a concern, not a blocker — but it is the kind of latent false-positive that surfaces only when a territory adds an adjacent namespace, and the controllerNamespacePrefixes parameterization the docblock defers (:118-120) would inherit the same defect. Add a fixture: App\Http\ControllersSupport\SomeHelper calling $user->save(), asserting zero errors.

2. firstOrCreate / createOrFirst / restoreOrCreate are read-dominant hybrids in the blocklist without a fixture pinning intent. MUTATION_METHODS (ForbidEloquentMutationInControllersRule.php:182-185) includes firstOrCreate, createOrFirst, and restoreOrCreate. These are defensible bans — they can write — but they read semantically different from a bare create(): the common controller use is idempotent lookup-or-provision, and a consumer team will reasonably push back when the rule fires on what reads at the call site like a getter. The blocklist is the rule's contract and three of its 24 entries carry that ambiguity unguarded by a test. I am not asking you to remove them — the ADR-0019 line is "banned application-wide" and that is a fair reading. I am asking for one violation fixture on firstOrCreate so the decision is pinned and a future "soften this" PR has to argue against an explicit test rather than silently flip an in_array entry. The static-call matrix only exercises create, destroy, updateOrCreate (ForbidEloquentMutationInControllersRuleTest.php:1039-1075); the *OrFirst / *OrCreate family is otherwise uncovered.

Nit

Illuminate\Database\Query\Builder (DB::table()) is not in the type gate, and no fixture documents the miss. matchedReceiverFqcn (ForbidEloquentMutationInControllersRule.php:332-347) matches Eloquent\Model and Eloquent\Builder only. A controller calling DB::table('users')->update([...]) or ->delete() returns a Query\Builder, not an Eloquent\Builder, and passes the gate clean. Arguably fine — raw DB::table() mutations are a separate doctrine surface — but right now it is an undocumented silent miss. The out-of-scope docblock block (:152-162) names non-Eloquent receivers and dynamic method names; one line there naming Illuminate\Database\Query\Builder would make the boundary deliberate rather than incidental.

Praise

The Builder-coverage decision is load-bearing and the PR resolved it the honest way. The order's §A7 offered a cheap out — declare Builder out of scope and defer — and the PR took the type-aware path instead: ObjectType::isSuperTypeOf() matches Builder<User> as a subtype of the unparameterized Builder without unwrapping the generic, verified empirically by ViolationBuilderUpdate.php firing on the ->update([...]) line of a User::query()->where(...)->update(...) chain (ForbidEloquentMutationInControllersRuleTest.php:1112-1122). That closes the exact gap the consumer-side string-match Pest tests could not — ->update($vars) without an inline array literal — which is the whole justification for promoting this to a type-aware rule. The docblock self-fail trap from prior package PRs (backticked @phpstan-ignore example annotations) is also avoided: the suppression note (ForbidEloquentMutationInControllersRule.php:149-150) references the identifier in prose, not a backticked annotation example, so self-analysis stays clean.

One process note, not a finding: this diff is 993 additions, not the stacked total. It is branched off #27's branch (verified OPEN, unmerged), so the [Unreleased] CHANGELOG section shows both this rule's entry and EnforceAuditTransactionScopeRule's — but the EnforceAuditTransactionScopeRule entry and the extension.neon services-block neighbor are #27-inherited context, not new file changes in this PR. The only new source/test files here are ForbidEloquentMutationInControllersRule.php (370 lines), its test (202), and 14 fixtures plus _stubs.php. Merge #27 first, then this rebases onto fresh main. The CHANGELOG, README table, CLAUDE.md table, and extension.neon registration for the new rule are all consistent.

This is a COMMENT-level review — no blockers, ship it once #27 lands and the two concerns get a decision (a fixture each, or an explicit "won't fix" on the namespace boundary). Verdict: solid, type-aware, well-tested; the gaps are at the edges, not the core.

jasperboerhof
jasperboerhof previously approved these changes Jun 1, 2026
Copy link
Copy Markdown
Contributor

@jasperboerhof jasperboerhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved — reviewed, no blockers.

@Goosterhof Goosterhof force-pushed the queue-87-forbid-eloquent-mutation-in-controllers branch from 1a222eb to 697e558 Compare June 1, 2026 10:44
@Goosterhof Goosterhof force-pushed the queue-86-enforce-audit-transaction-scope branch from f6e3757 to 63acabf Compare June 1, 2026 10:44
@Goosterhof Goosterhof changed the base branch from queue-86-enforce-audit-transaction-scope to main June 1, 2026 11:46
@Goosterhof Goosterhof dismissed jasperboerhof’s stale review June 1, 2026 11:46

The base branch was changed.

New PHPStan rule banning Eloquent persistence-API method calls
(save/update/delete/create/destroy/forceDelete/forceFill/push/restore/touch
and their *OrFail / *Quietly / *OrCreate variants — 24-method blocklist) on
`Illuminate\Database\Eloquent\Model` subclasses and
`Illuminate\Database\Eloquent\Builder` chains when the call site is inside an
`App\Http\Controllers\*` class (including sub-namespaces via `str_starts_with`).
Reads (`find`, `where`, `get`, `first`, `paginate`, `pluck`, `count`, `exists`,
`query`) deliberately permitted — controllers reading Models is necessary for
route-model binding, ResourceData hydration, and policy checks.

Identifier: `forbidEloquentMutationInControllers.eloquentMutationInController`.
Doctrine source: ADR-0011 (Action Class Architecture) + ADR-0019 (Explicit
Model Hydration).

Algorithm:
- Namespace gate (`App\Http\Controllers` prefix)
- Recursively walk every `ClassMethod` body collecting `MethodCall`/`StaticCall`
- For `MethodCall`: type-aware `ObjectType::isSuperTypeOf()` against `Model` OR
  `Builder` matches the receiver; method name in blocklist fires
- For `StaticCall`: `Scope::resolveName()` resolves to Model subclass FQCN;
  method name in blocklist fires

Builder coverage uses resolution (a) per order §A7 — type-aware
`ObjectType::isSuperTypeOf()` handles `Builder<User>` as a subtype of the
unparameterized `Builder` cleanly, no brittle generic introspection needed.
Verified empirically by `ViolationBuilderUpdate.php` fixture firing at the
`->update([...])` line of a `User::query()->where(...)->update([...])` chain.

Supersedes consumer-side string-match Pest arch tests in kendo, ublgenie,
entreezuil, and the ISMS bridge subset from PR #10. Cross-territory cascade is
the General's follow-up dispatch after this lands; emmie + BIO pick up coverage
automatically on next composer update.

14 tests, 14 assertions; PHPStan max self-analysis clean (10 services); Pint
clean; line coverage 87.98% (new rule 91.01%) ≥83 gate; MSI 82%, Covered Code
MSI 82% (both ≥75 gate). The 7 escaped mutants on the new rule are all in the
recognizable Standing Concern #29 walkNodes() parity family + MBString-
equivalent + defensive `??` defaults — same shape as the precedent on the
other four rules.

Closes war-room enforcement queue #87.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Goosterhof Goosterhof force-pushed the queue-87-forbid-eloquent-mutation-in-controllers branch from 697e558 to c8fdb46 Compare June 1, 2026 11:47
@Goosterhof Goosterhof requested a review from jasperboerhof June 1, 2026 11:47
@Goosterhof
Copy link
Copy Markdown
Contributor Author

Retargeted to main now that #27 has merged (queue-86 deleted). Rebased onto fresh main to clear BEHIND — single commit, rule source + tests byte-identical to your prior approval, CHANGELOG [Unreleased] cleanly carries all three rules' ### Added bullets + #29's pin. Re-review requested (force-push dismissed the stale approval on the protected base). #26 follows once this lands.

@Goosterhof Goosterhof enabled auto-merge June 1, 2026 11:47
Copy link
Copy Markdown
Contributor Author

@Goosterhof Goosterhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Four findings: zero blockers, two concerns, one nit, one praise.

The rule is correct on its declared scope and the test matrix is genuinely good — type-aware receiver matching, static-call FQCN resolution, Builder-chain coverage, and a kendo sub-namespace regression guard are all exercised with line-anchored expectations. The PR body is honest about the candidate-Major versioning and the cross-territory cascade. My findings are about edge cases the matrix does not pin and one namespace-gate sharp edge. None of them block.

Concerns

1. Namespace gate has no separator boundary — App\Http\ControllersFoo false-matches. processNode gates on str_starts_with($namespace, self::CONTROLLER_NAMESPACE_PREFIX) (ForbidEloquentMutationInControllersRule.php:198) where the constant is 'App\Http\Controllers' (:168) with no trailing \. A sibling namespace — App\Http\ControllersSupport\*, App\Http\ControllersTest — would be pulled into scope and start firing on Eloquent mutations that are legitimately outside the controller surface. The kendo sub-namespace case (App\Http\Controllers\Central\*) is the intended positive and is well-tested (ViolationKendoCentralSubnamespace.php), but it works precisely because Central follows a \. The fix is cheap: match 'App\Http\Controllers' exactly OR 'App\Http\Controllers\' as a prefix. Unlikely to bite a current consumer — hence a concern, not a blocker — but it is the kind of latent false-positive that surfaces only when a territory adds an adjacent namespace, and the controllerNamespacePrefixes parameterization the docblock defers (:118-120) would inherit the same defect. Add a fixture: App\Http\ControllersSupport\SomeHelper calling $user->save(), asserting zero errors.

2. firstOrCreate / createOrFirst / restoreOrCreate are read-dominant hybrids in the blocklist without a fixture pinning intent. MUTATION_METHODS (ForbidEloquentMutationInControllersRule.php:177-187) includes firstOrCreate, createOrFirst, and restoreOrCreate. These are defensible bans — they can write — but they read semantically different from a bare create(): the common controller use is idempotent lookup-or-provision, and a consumer team will reasonably push back when the rule fires on what reads at the call site like a getter. The blocklist is the rule's contract and three of its 24 entries carry that ambiguity unguarded by a test. I am not asking you to remove them — the ADR-0019 line is "banned application-wide" and that is a fair reading. I am asking for one violation fixture on firstOrCreate so the decision is pinned and a future "soften this" PR has to argue against an explicit test rather than silently flip an in_array entry. The static-call matrix exercises only create, destroy, updateOrCreate (ForbidEloquentMutationInControllersRuleTest.php:1039-1075); the *OrFirst / *OrCreate family is otherwise uncovered.

Nit

Illuminate\Database\Query\Builder (DB::table()) is not in the type gate, and no fixture documents the miss. matchedReceiverFqcn (ForbidEloquentMutationInControllersRule.php:332-347) matches Eloquent\Model and Eloquent\Builder only. A controller calling DB::table('users')->update([...]) or ->delete() returns a Query\Builder, not an Eloquent\Builder, and passes the gate clean. Arguably fine — raw DB::table() mutations are a separate doctrine surface — but right now it is an undocumented silent miss. The out-of-scope docblock block (:152-162) names non-Eloquent receivers and dynamic method names; one line there naming Illuminate\Database\Query\Builder would make the boundary deliberate rather than incidental.

Praise

The Builder-coverage decision is load-bearing and the PR resolved it the honest way. The order's §A7 offered a cheap out — declare Builder out of scope and defer — and the PR took the type-aware path instead: ObjectType::isSuperTypeOf() matches Builder<User> as a subtype of the unparameterized Builder without unwrapping the generic, verified empirically by ViolationBuilderUpdate.php firing on the ->update([...]) line of a User::query()->where(...)->update(...) chain (ForbidEloquentMutationInControllersRuleTest.php:1112-1122). That closes the exact gap the consumer-side string-match Pest tests could not — ->update($vars) without an inline array literal — which is the whole justification for promoting this to a type-aware rule. The docblock self-fail trap from prior package PRs (backticked @phpstan-ignore example annotations) is also avoided: the suppression note (:149-150) references the identifier in prose, not a backticked annotation example, so self-analysis stays clean.

One process note, not a finding: this diff is 993 additions and reads clean against current main. PR #27 (EnforceAuditTransactionScopeRule) is now MERGED, so its rule, CHANGELOG entry, and extension.neon registration are already on main — the [Unreleased] CHANGELOG EnforceAuditTransactionScopeRule line shows in this diff as context (no + marker, CHANGELOG.md:10), not a new addition. The only new bullet is this rule's (CHANGELOG.md:9). The PR body still reads as "stacked on #27 / will rebase once #27 merges" — that framing is now stale; #27 has landed, so this is just a normal feature PR against main. The CHANGELOG, README table, CLAUDE.md table, and extension.neon registration for the new rule are all consistent.

Verdict: COMMENT — no blockers, solid, type-aware, well-tested. The gaps are at the edges, not the core. Ship once the two concerns get a decision (a fixture each, or an explicit "won't fix" on the namespace boundary).

@jasperboerhof
Copy link
Copy Markdown
Contributor

PR Reviewer · claimed

  • Target: phpstan-warroom-rules
  • PR: feat: add ForbidEloquentMutationInControllersRule (queue #87) #28
  • Branch: queue-87-forbid-eloquent-mutation-in-controllers
  • AC anchor: none (board-less target; no plan dir; PR body has Test plan, no Acceptance Criteria section)
  • Worktree: ~/Code/agent-worktrees/phpstan-warroom-rules/review-28
  • Note: re-review (force-push rebase onto fresh main 2026-06-01 post-dated the 2026-05-29 PASS verdict)

@jasperboerhof
Copy link
Copy Markdown
Contributor

PR Reviewer · 9/10 · PASS

Findings

  • MAJOR · extension.neon:33:33 — Contract change (informational, not a defect): registering ForbidEloquentMutationInControllersRule adds a new tightening rule that ripples to every consumer on composer update — kendo, codebook, emmie, entreezuil, ublgenie, brick-inventory, ISMS. Previously-green consumer CI with controllers calling Eloquent persistence APIs (especially type-aware additional surface: Model::create(), chained Builder mutations, *Quietly variants the old string-match Pest tests missed) will newly fail. Correctly handled: CHANGELOG flags it as a candidate Major bump with a mandatory pre-cascade audit list, so this is the expected contract impact, not a flaw.
  • MINOR · tests/Rules/ForbidEloquentMutationInControllersRuleTest.php — Only 7 of the 24 blocklisted methods are exercised by fixtures (save, update, delete, forceDelete, create, destroy, updateOrCreate); the *Quietly / OrFail / restore / firstOrCreate / createOrFirst / fill / forceFill / push / touch entries — the very 'additional surface' the CHANGELOG cites as the Major-bump driver — have no fixture, so a mutation deleting any one of those array entries survives undetected. Not a correctness bug (membership is a uniform in_array check), but the blocklist is a contract and untested entries can silently regress on edit.
  • MINOR · src/Rules/ForbidEloquentMutationInControllersRule.php:142 — Namespace gate uses str_starts_with($namespace, 'App\Http\Controllers') with no trailing-separator boundary, so a sibling namespace like App\Http\ControllersHelpers* would also be gated as a controller and could fire false positives. The prefix-match is a deliberate choice to support sub-namespaces (App\Http\Controllers\Central*, fixture-locked), but the boundary case is unguarded. Low-risk and suppressible; consider matching the prefix plus a '' separator (or exact-equal-or-prefixed-with-separator) if a consumer ever ships such a sibling namespace. Out-of-scope-adjacent; does not block.

Action

merge-ready

Copy link
Copy Markdown
Contributor

@jasperboerhof jasperboerhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved by /review-open-prs — review verdict is PASS. See the verdict comment for the per-reviewer breakdown.

@Goosterhof Goosterhof merged commit b1d7b70 into main Jun 1, 2026
2 checks passed
@Goosterhof Goosterhof deleted the queue-87-forbid-eloquent-mutation-in-controllers branch June 1, 2026 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Agent Review Requested Requesting review of specialized AI review agents.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants