Skip to content

docs(web/guides): fix auth-patterns format check and hasStrategy rationale - #3119

Closed
wheels-bot[bot] wants to merge 1 commit into
developfrom
docs/bot-3116-docs-web-guides-authentication-patterns-combined-s
Closed

docs(web/guides): fix auth-patterns format check and hasStrategy rationale#3119
wheels-bot[bot] wants to merge 1 commit into
developfrom
docs/bot-3116-docs-web-guides-authentication-patterns-combined-s

Conversation

@wheels-bot

@wheels-bot wheels-bot Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Two corrections to web/sites/guides/src/content/docs/v4-0-0/digging-deeper/authentication-patterns.mdx, surfaced by the 2026-06 guide behavioral audit.

Fix 1 — Elvis/equality precedence crash (line 343). The combined-strategy base controller had:

if (request.format ?: "html" == "json") {

CFML parses this as request.format ?: ("html" == "json"). With request.format set, the ?: short-circuits and returns the string "json", which if() cannot cast to boolean — it throws at runtime. With request.format absent, the ?: evaluates the right-hand side (false) and the JSON branch is unreachable. The fix parenthesizes the operands:

if ((request.format ?: "html") == "json") {

Fix 2 — Wrong hasStrategy rationale (line 64). The prose claimed the hasStrategy guard "keeps a second reload from stacking duplicates." In source, Authenticator.registerStrategy() already removes any existing entry with the same name before appending, so duplicates cannot stack. The guard merely skips rebuilding the strategy object on warm reloads. The sentence is reworded accordingly.

Fixes #3116

Screenshots needed

None.

…onale

Parenthesize the combined-strategy format check at line 343 so
`(request.format ?: "html") == "json"` evaluates correctly — CFML
parses the unparenthesized form as `request.format ?: ("html" == "json")`,
which crashes at runtime when request.format is set and always falls
through to the HTML branch otherwise.

Also corrects the hasStrategy rationale: registerStrategy() replaces any
existing entry with the same name in place, so duplicate registrations
cannot stack. The guard skips rebuilding the strategy object on warm reloads.

Fixes #3116

Co-authored-by: wheels-bot[bot] <wheels-bot[bot]@users.noreply.github.com>
Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Wheels Bot — Reviewer

TL;DR: This PR makes two corrections to the authentication-patterns guide: parenthesizing the combined-strategy format check ((request.format ?: "html") == "json") and rewording the hasStrategy rationale. Both fixes are verified correct against the framework source and CFML operator-precedence semantics. Verdict: comment — no blocking findings, but two docs nits worth noting, one of which (the "in place" wording) introduces a new subtle inaccuracy about registerStrategy() ordering.

Correctness

Both fixes verified — no findings.

  • Fix 1 verified. CFML's Elvis operator binds looser than ==, so the old request.format ?: "html" == "json" parsed as request.format ?: ("html" == "json") — a string that if() can't cast to boolean when request.format is set, and an always-false expression when it isn't. The parenthesized form is the standard correction, and issue #3116 includes a Lucee probe confirming the runtime behavior. A grep of web/sites/guides/src/content/docs/ confirms this was the only occurrence of the unparenthesized ?: … == pattern.
  • Fix 2 verified. vendor/wheels/auth/Authenticator.cfc:181-185registerStrategy() calls removeStrategy() for an existing same-name entry before appending, with the doc comment "If a strategy with this name already exists, it is replaced." Duplicates indeed cannot stack, so the old "keeps a second reload from stacking duplicates" rationale was wrong and the correction is warranted.

Docs

  1. web/sites/guides/src/content/docs/v4-0-0/digging-deeper/authentication-patterns.mdx:64 — "replaces … in place" is subtly inaccurate. The new prose says:

    registerStrategy() replaces any existing entry with the same name in place

    In source it's remove-then-append: removeStrategy() (Authenticator.cfc:200-212) deletes from the ordered variables.strategies array, then ArrayAppend puts the new entry at the end — so re-registration moves a strategy to the back of the try-order, it does not preserve its position. That distinction matters because this same guide states "Order matters." in the combined-strategy section (line ~334), and $buildStrategyOrder() iterates strategies in registration order. The operative claim (duplicates can't stack) is correct, and in the single-strategy context of line 64 ordering is irrelevant — but as a general statement about registerStrategy() it could mislead a reader wiring multiple strategies. The PR body itself describes the mechanism accurately ("removes any existing entry with the same name before appending"). Suggested wording: "registerStrategy() removes any existing entry with the same name before appending, so duplicate registrations cannot stack (note: re-registering moves a strategy to the end of the try-order)." The changelog fragment (changelog.d/3116-auth-patterns-format-check-and-hasstrategy.fixed.md:1) repeats the same "replaces in place" phrasing and would want the same touch-up.

  2. authentication-patterns.mdx:343request.format is never set by the framework (pre-existing, out of scope, follow-up candidate). The parenthesization makes the expression correct CFML, but request.format has zero occurrences in vendor/wheels/** — nothing in the framework populates it, so in a stock app the JSON branch is still unreachable unless the app sets request.format itself. The Wheels convention for format detection is params.format, populated from the route's .[format] segment and consumed by vendor/wheels/controller/rendering.cfc:782. Switching the example to (params.format ?: "html") == "json" would key the branch off the mechanism Wheels actually provides. This predates the PR and exceeds the scope of #3116, so it's non-blocking here — but it may deserve a follow-up issue, since the example's JSON path remains effectively dead code even after this fix.

Commits

No findings. Single commit docs(web/guides): fix auth-patterns format check and hasStrategy rationale — valid type, sensible scope, ≤ 100 chars, matches the change.

Summary

The two scoped fixes are accurate and well-evidenced; the changelog fragment and commit hygiene are in order. The nits above are wording-level and don't block — though nit 1 is worth a quick amend since the PR's whole purpose is correcting an inaccurate rationale, and "in place" trades one subtle inaccuracy for another.

@bpamiri

bpamiri commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Superseded by #3130 (merged 7fae2a3) — develop already contains both substantive fixes (character-identical parenthesized form at authentication-patterns.mdx:343); issue #3116 closed with it.

@bpamiri bpamiri closed this Jun 12, 2026
@bpamiri
bpamiri deleted the docs/bot-3116-docs-web-guides-authentication-patterns-combined-s branch June 12, 2026 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

1 participant