Skip to content

[SKETCH] Size the request-time base-variant fix (#106696) - #2

Draft
CybotTM wants to merge 2 commits into
mainfrom
sketch/106696-proper-fix-sizing
Draft

[SKETCH] Size the request-time base-variant fix (#106696)#2
CybotTM wants to merge 2 commits into
mainfrom
sketch/106696-proper-fix-sizing

Conversation

@CybotTM

@CybotTM CybotTM commented Jul 1, 2026

Copy link
Copy Markdown
Owner

Note

Sizing sketch only — NOT for merge, and not how TYPO3 Core is contributed (that goes through Gerrit). This exists to make the size of a correct request-time base-variant fix for forge #106696 tangible — the "big" alternative to the naive one-liner (CybotTM/typo3#1) and to the docs PR (TYPO3-Documentation/TYPO3CMS-Reference-CoreApi#6651).

Why the naive fix cannot work (recap)

Base variants are resolved in the request-less Site constructor, reached from the SiteResolver middleware before $GLOBALS['TYPO3_REQUEST'] is ever assigned (that happens only in the innermost frontend/Http/RequestHandler::handle()). So ip() silently returns false. A correct fix must make the base request-scoped at read time, which changes the lifecycle of Site and SiteLanguage.

What this branch implements

The new infrastructure a request-time fix needs:

  • CurrentRequestHolder (new, ~50 LOC): a process-wide, request-scoped holder, external to Site (which is serialized into the runtime cache, so a live request must never live on it), with withRequest() save/restore for sub-requests.
  • Services.yaml: registers it (public, shared).
  • SiteResolver middleware: populates the holder with the request — which at that point already carries normalizedParams / the client IP — and restores it via try/finally.

These compile but intentionally do nothing on their own: the holder is populated and never read, because the large consuming surgery is not applied (it is the risky part being sized).

What a correct fix additionally needs (NOT in this branch — the sized surgery)

See SIZING.md for the file-by-file breakdown. In short: Site.php (stop eager resolve, lazy getBase(), ~100–150 LOC), SiteLanguage.php (lazy base, ~40–70 LOC), SiteMatcher canonical-base carve-out so identification stays request-independent while presentation becomes request-aware, backend SiteResolver + adminpanel preview holder population, and real FE-request functional tests (~200–320 LOC).

Size bands

Option Files LOC Backport Status
Naive fix (#1) 2 +45 / −1 broken (silent no-op)
Docs PR (#6651) 1–2 ~30–80 prose 13.4 ✓ recommended
Correct fix (A+C hybrid) ~10–11 ~450–700 (prod ~250–380, tests ~200–320) main-only this sketch sizes it
Reduced (site base only) ~5–6 ~250 main-only partial fix

≈ 10–15× the naive fix's LOC, main-only, with real BC / site-identity / cache-coherence risk across TypoLink, redirects, hreflang and CSP — for a capability the ecosystem already covers via a custom ExpressionLanguageProvider (b13/host-variants).

Verdict

NO-GO on a core code fix as the primary answer. Ship the docs PR (#6651), delegate request/host-dependent bases to the provider pattern, and only build this out if the maintainers explicitly mandate request-dependent base variants in core.

Sizing skeleton only, NOT for merge. Implements the new request-scoped
plumbing tier (CurrentRequestHolder + Services registration + SiteResolver
hook) so the shape and injection point are tangible, and documents in
SIZING.md the large Site/SiteLanguage lazy-resolution surgery that a
correct fix additionally requires (~10-11 files, ~450-700 LOC).

See https://forge.typo3.org/issues/106696

Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Implements the site-level request-aware base:
- keep canonical (request-independent) base eager in the Site constructor,
  resolved with RequestWrapper(null) so ip() no longer throws
- resolve the request-aware base lazily in getBase() against a request
  published by the SiteResolver middleware via CurrentRequestHolder
- site identification (SiteMatcher) uses getCanonicalBase(), so routing
  stays request-independent

Per-language variants, backend/CLI holder population and a SiteInterface
compat path are deliberately out of scope; see SIZING.md.

See https://forge.typo3.org/issues/106696

Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
@CybotTM

CybotTM commented Jul 1, 2026

Copy link
Copy Markdown
Owner Author

Alternative: explicit withRequest() variant → #3

Per @garvinhicking's review feedback (avoid the to-be-phased-out $GLOBALS['TYPO3_REQUEST']; treat Site as a value object), the v15, core-direction-aligned alternative lives in #3: no ambient state, an immutable Site::withRequest(): self clone, request flowing through the existing site request attribute.

Key finding from that buildout: the "purer" explicit approach is actually more invasive than this holder — replacing the site attribute with a clone breaks a core invariant (the attribute is object-identical to the SiteFinder result), so two identity comparisons (ShortcutAndMountPointRedirect, PageLinkBuilder) had to change to compare by identifier. The ambient holder here avoids that entirely.

#3 has since been hardened to core-mergeable shape (multi-reviewer pass): no SiteInterface BC break (getCanonicalBase()/withRequest() are concrete-only), __serialize() drops the bound request, Feature-106696 + Breaking-106696 changelog included, [FEATURE] commit. It is now 9 files (+ the two changelog entries) with the full suite green (SiteHandling 2510). Both variants confirm the mechanism is small; the architecture is the hard part. See #3 for the full comparison and honest limitations.

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.

1 participant