fix(controller): register super<name> for app-level controller and view overrides - #3357
Conversation
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR — This PR fixes the super<name> asymmetry between Controller.cfc and Model.cfc (#3325): overriding a controller/view helper like linkTo() and delegating via superLinkTo() used to 500 because the super<name> alias was never registered outside the plugin/package-override path. The core change is correct, minimal, and — I verified — makes Controller.cfc::$integrateFunctions() byte-for-byte identical to Model.cfc::$integrateFunctions() (vendor/wheels/Model.cfc:598-620), so this is true parity rather than a lookalike. Tests are red-first, cover both the happy path and the zero-cost property, and a changelog fragment is present. My verdict is comment: no correctness / cross-engine / security / TDD blockers, but the PR sweeps in 7 unrelated generated MockBox stub files that should be dropped before merge.
Conventions
- 7 stray MockBox stub artifacts unrelated to this change. The diff adds
public/testbox/system/stubs/{139184C0…, 16CA3942…, 30942F4D…, 754BEF48…, B7681C49…, C383511C…, CDCB73D6…}.cfm(436 insertions). These are runtime-generated TestBox/MockBox stubs for SSE/polling specs (closeSSEStream,sendSSEComment,sendSSEEvent,initSSEStream,poll,checkError) — nothing to do withsuper<name>parity. They were almost certainly swept in from a local full-suite run. Recommendgit rmthem from the PR. There is mixed precedent (one such stub,F952D54F…, is already tracked ondevelop), and.gitignorehas notestbox/stubsentry — so a small follow-up addingpublic/testbox/system/stubs/to.gitignorewould stop this recurring. Not merge-blocking, but it is noise in a bug-fix PR.
Correctness
No issues. Walked the new else branch (vendor/wheels/Controller.cfc:415-419): it only ever writes super<name>, never touches the original name, so an app override is never clobbered; when a name is both pre-existing and in overrideSet, both branches write the same ref idempotently. The zero-extra-keys-when-overriding-nothing claim is empirically pinned by the fourth spec (SuperOverrideSpec.cfc, test controller → 0 super* keys) and the suite is green (+4), confirming no framework mixin name collides with an inherited/declared Controller/Global method.
Tests
Good coverage. vendor/wheels/tests/specs/controller/SuperOverrideSpec.cfc asserts registration (StructKeyExists(c, "superLinkTo")), real delegation (toStartWith("wrapped:"), toInclude("<a"), and crucially notToInclude("wrapped:wrapped:") proving the framework original ran rather than the override recursing), model-side parity, and the zero-cost property. BDD, extends wheels.WheelsTest. Fixtures on both sides deliberately share a name to prove symmetry.
Cross-engine
No concerns. The change is pure mixin machinery mirroring the long-standing Model.cfc path (invariant 7 territory), and the PR body notes the compat matrix was dispatched. Left(key, 5) in the spec key scan is standard clamping behavior and the lucee7 suite ran green.
Commits
fix(controller): register super<name> for app-level controller and view overrides — valid type/scope, subject not ALL-CAPS, header <=100 chars, DCO sign-off present, body explains the why. Conforms to commitlint.config.js.
Docs
Changelog fragment changelog.d/3325-super-override-parity.fixed.md is present and correctly named (<slug>.fixed.md). The user-facing "Overriding Core Methods" guide port is explicitly deferred as a follow-up (v4 tree has no such page yet) — reasonable to land after this, since only now is the general claim true.
Wheels Test Results 32 files 9 800 suites 21m 3s ⏱️ For more details on these failures and errors, see this check. Results for commit f612fab. |
c8f3213 to
2072eb3
Compare
…ew overrides Following the "Overriding Core Methods" guide — override `linkTo()`, delegate with `superLinkTo()` — returned a 500, because `superLinkTo` never existed in controller or view context. The convention was implemented asymmetrically. `Model.cfc`'s $integrateFunctions() aliases the framework original to `super<name>` whenever the mixin's name is ALREADY present on the target, which is precisely the app-override case. `Controller.cfc`'s had no such branch: it aliased only names a registered plugin/package mixin overrode. So model overrides got `superFindAll()` and controller/view overrides got nothing, with no indication why. Adding the same else-branch is the whole fix. Before writing it I measured the cost, because `Controller` mixes in a much larger surface than `Model` and this runs on every request: a controller that overrides nothing has 445 keys and gains ZERO `super*` entries, before and after. No two framework mixins contribute the same name, so the branch fires only on a genuine app override — where the extra reference is the point. `Mapper.cfc` has its own $integrateFunctions() variant and the issue flagged it as worth checking. It is not affected: it assigns unconditionally with no existence check and has no `super<name>` concept at all, so there is no asymmetry to correct. Left alone. Red-first, against the pristine Controller.cfc: the two controller specs fail — `Expected [false] to be true` and `No matching function [SUPERLINKTO] found` — while the model-side spec and the no-override spec pass. That split is the bug, stated as a test. 4 specs, using two new fixtures of the same name on each side: a controller overriding `linkTo()` and a model overriding `columnNames()`, both delegating through `super<name>`. The controller spec asserts a real anchor comes back and that the prefix appears exactly once, so it proves the framework original ran rather than the override recursing. The model spec pins behaviour that already worked, so the parity cannot regress from either direction. The fourth asserts the zero-extra-keys property above. Docs are NOT included here. The v3 guide presents the convention as general and the v4-0-0 tree has no "Overriding Core Methods" page at all; porting and correcting it is a separate change against the guides site. Verification, lucee7 + sqlite, full core suite: develop ab901cf 4732 pass / 0 fail / 0 error this branch 4736 pass / 0 fail / 0 error Exactly +4, the new specs. Closes #3325 Signed-off-by: Peter Amiri <peter@alurium.com>
2072eb3 to
47aeb9f
Compare
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR — This PR fixes the super<name> asymmetry between Controller.cfc and Model.cfc: overriding a controller or view helper (e.g. linkTo()) and delegating via superLinkTo() — exactly as the "Overriding Core Methods" guide documents — returned a 500 because the super<name> alias was never registered for app-level overrides. The fix adds the same else-branch Model.cfc has always carried. The diff is minimal, well-tested, and cross-engine-safe. Verdict: approve.
Correctness
The new branch in vendor/wheels/Controller.cfc:418-422
} else {
local.superName = "super" & local.name;
variables[local.superName] = local.ref;
this[local.superName] = local.ref;
}is byte-for-byte identical to the proven branch in vendor/wheels/Model.cfc:608-612, restoring true parity. The else fires only when the name already exists on the target (the app-override case) or via an earlier-integrated mixin; the subsequent overrideSet block still handles plugin/package overrides. The "no two framework mixins contribute the same name" claim is not just asserted — it is pinned by the adds no super<name> keys to a controller that overrides nothing spec against the test fixture, which I confirmed overrides no framework helper.
Cross-engine
No new idioms introduced. Left(key, 5) in the spec is safe on Lucee 7 (length 5 > 0, so the invariant-8 Left(str,0) crash does not apply). No struct member .map(), no inline-closure-as-constructor-arg (invariant 5), no application-scope function members. Exposing this[superName] mirrors both the pre-existing plugin-override branch and Model.cfc, so it adds no novel engine risk. This is mixin machinery on the per-request hot path (invariant 7) and the author reports the compat matrix was dispatched.
Tests
vendor/wheels/tests/specs/controller/SuperOverrideSpec.cfc covers the registration, the delegation happy path (asserting wrapped: appears exactly once via notToInclude("wrapped:wrapped:"), which proves the framework original ran rather than the override recursing), the zero-extra-keys no-override case, and the model-side parity. Fixtures verified: c_o_r_e_posts exists in vendor/wheels/tests/populate.cfm, and columnNames is a real framework model helper (vendor/wheels/model/miscellaneous.cfc), so superColumnNames is genuinely exercised. All matchers (toStartWith, notToInclude) are used elsewhere in the suite.
Docs
The PR body honestly defers the docs half — the v3 "Overriding Core Methods" guide presents the convention as general (only now true) and the v4-0-0 tree has no such page. This is a legitimate follow-up against the guides site, not a blocker for this change. Please do open that follow-up so users can rely on the corrected general claim.
Commits
fix(controller): register super<name> for app-level controller and view overrides — valid conventional-commit type/scope, header under 100 chars, not ALL-CAPS. DCO Signed-off-by: Peter Amiri present. Changelog fragment changelog.d/3325-super-override-parity.fixed.md uses a valid fixed type — correct fragment-based approach, no direct CHANGELOG.md edit.
Nice work — the red-first framing and the cost measurement on the hot path are exactly the right diligence for mixin machinery.
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR — This PR fixes the super<name> asymmetry between Controller.cfc and Model.cfc (#3325, from discussion #3323): overriding a controller or view helper (e.g. linkTo()) and delegating via superLinkTo() — exactly as the "Overriding Core Methods" guide documents — returned a 500, because the super<name> alias was only registered for plugin/package mixin overrides, never for app-level overrides. The fix adds the same else branch Model.cfc has always carried. The diff is minimal, correct, well-tested, and cross-engine-safe — and the 7 stray MockBox stub artifacts flagged on the earlier commit have been dropped, so the head is clean. Verdict: approve.
Correctness
The new branch in vendor/wheels/Controller.cfc:418-422
} else {
local.superName = "super" & local.name;
variables[local.superName] = local.ref;
this[local.superName] = local.ref;
}is byte-for-byte identical to the long-standing branch in vendor/wheels/Model.cfc:606-611, restoring true parity rather than a lookalike (I diffed both $integrateFunctions() bodies — they now match exactly). The else fires only when the name already exists on the target (the app-override case, plus any inherited/earlier-integrated name), and the subsequent overrideSet block still handles plugin/package overrides idempotently — a name that is both pre-existing and in overrideSet writes the same ref twice, never clobbering the app override. The "no two framework mixins contribute the same name" claim is not merely asserted: the fourth spec pins the test controller at 0 super* keys, so the zero-cost-in-the-common-case property is empirically green on the +4 suite.
Cross-engine
No new idioms. Left(key, 5) in the key scan (SuperOverrideSpec.cfc:51) is safe on Lucee 7 — StructKeyArray keys are never empty so length >= 1, and invariant 8 only bites Left(str, 0); counts exceeding string length are already exercised across the full CI matrix (e.g. sseSpec.cfc:157 Left(Trim(local.line), 6), HtmlReportFunctionDeclarationGuardSpec.cfc:58 Left(trimmed, 2)), so no engine throws. This is pure mixin machinery on the per-request hot path mirroring Model.cfc (invariant 7 territory), and the PR body notes the compat matrix was dispatched. No struct member .map(), no inline-closure-as-constructor-arg (invariant 5), no application-scope function members.
Tests
vendor/wheels/tests/specs/controller/SuperOverrideSpec.cfc (BDD, extends wheels.WheelsTest) covers registration (StructKeyExists(c, "superLinkTo")), real delegation (toStartWith("wrapped:"), toInclude("<a"), and crucially notToInclude("wrapped:wrapped:") proving the framework original ran rather than the override recursing), the model-side parity, and the zero-extra-keys no-override case. Fixtures verified real: c_o_r_e_posts exists in vendor/wheels/tests/populate.cfm, columnNames() is a genuine model helper (vendor/wheels/model/miscellaneous.cfc:157), the test controller fixture (Test.cfc) exists, and notToInclude is used elsewhere in the suite (sseSpec.cfc).
Docs
Changelog fragment changelog.d/3325-super-override-parity.fixed.md is present and correctly named (<slug>.fixed.md) — no direct CHANGELOG.md edit. The PR body honestly defers the user-facing "Overriding Core Methods" guide port (the v4-0-0 tree has no such page and the general claim is only now true) as a follow-up — a reasonable split, not a blocker. Please do open that follow-up so users can rely on the corrected convention.
Commits
fix(controller): register super<name> for app-level controller and view overrides — valid conventional-commit type/scope, header <= 100 chars, not ALL-CAPS, DCO Signed-off-by: Peter Amiri present. Conforms to commitlint.config.js. The body explains the why (the asymmetry) rather than the what.
Nice work — the red-first framing and measuring the hot-path key cost before adding the branch are exactly the right diligence for mixin machinery.
Ports the v3-0-0 page into the v4-0-0 tree, corrected for 4.0.x: the super<name> convention now applies to models AND controllers/view helpers (parity landed with the #3325 fix, PR #3357). Documents placement conventions (single controller, app/controllers/Controller.cfc, app/views/helpers.cfm), the superLinkTo(argumentCollection=arguments) delegation pattern pinned by SuperOverrideSpec.cfc, the dataConfirm data-* pass-through alternative for the jsconfirm use case, and a version callout with the CreateObject workaround for 4.0.x builds predating the fix. Explicitly notes mapper internals are not an override surface. Registers the page in the v4-0-0 sidebar and section index, and adds a version-scope note to the v3-0-0 page (model-only in 3.x). Fixes #3343 Signed-off-by: Peter Amiri <petera@pai.com> Co-authored-by: Peter Amiri <petera@pai.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Closes #3325 (from discussion #3323).
The bug
Following the Overriding Core Methods guide — override
linkTo(), delegate withsuperLinkTo()— returns a 500, becausesuperLinkTonever exists in controller or view context.The convention is implemented asymmetrically:
super<name>registered when…Model.cfcController.cfcSo model overrides got
superFindAll(), and controller/view overrides got nothing, with no indication why.The fix
Adding the same
elsebranch toController.cfc::$integrateFunctions().I measured the cost before adding it
Controllermixes in a much larger surface thanModel, and this runs on every request, so "just add the branch" deserved a number rather than a shrug:super*keysNo two framework mixins contribute the same name, so the branch fires only on a genuine app override — where the extra reference is the entire point. Zero cost in the common case.
Mapper.cfc— checked, not affectedThe issue flagged
Mapper.cfc's own$integrateFunctions()variant as worth checking. It assigns unconditionally with no existence check and has nosuper<name>concept at all, so there is no asymmetry to correct. Left alone.Red-first
Against the pristine
Controller.cfc:…while the model-side spec and the no-override spec pass. That split is the bug, stated as a test.
Specs
4 specs, with two new fixtures deliberately sharing a name — a controller overriding
linkTo()and a model overridingcolumnNames(), both delegating throughsuper<name>:superLinkTo<a>comes back, and the fixture'swrapped:prefix appears exactly once, proving the framework original ran rather than the override recursing into itselfsuperColumnNames— pinning behaviour that already worked, so the parity cannot regress from either directionsuper*keysVerification
lucee7 + sqlite, full core suite:
ab901cff7Exactly +4 — the new specs, nothing else moved.
Compat matrix dispatched: this is mixin machinery on a per-request hot path, and mixin integration is exactly where engines diverge (cross-engine invariant 7).
Not included: the docs half
The issue's second point stands and I have not done it. The v3 guide presents the convention as general — it was model-only until this PR — and the v4-0-0 guide tree has no "Overriding Core Methods" page at all. Porting and correcting it is a separate change against the guides site, and it should land after this, since only now is the general claim true. Happy to pick it up next if you want it.
🤖 Generated with Claude Code