feat: add ForbidEloquentMutationInControllersRule (queue #87)

[Goosterhof]

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 gate — str_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 walk — Class_ 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. StaticCall — Scope::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]

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

[jasperboerhof]

PR Reviewer · 9/10 · PASS

• PR: feat: add ForbidEloquentMutationInControllersRule (queue #87) #28 · phpstan-warroom-rules
• AC anchor: none
• Reviewers run: Surface, Silent failures skipped — no triggers, Efficiency skipped — no triggers
• Surface: PASS · 9/10

Findings

• none — all reviewers clean

Action

merge-ready

[jasperboerhof]

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

[Goosterhof]

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]

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).