[DOCS] Correct reason for request independence in BaseVariants conditions - #6651
[DOCS] Correct reason for request independence in BaseVariants conditions#6651CybotTM wants to merge 5 commits into
Conversation
See https://forge.typo3.org/issues/106696 - conditions must be request independant. Thus, "ip()" and "traverse(request)" examples are removed and it is stated that request dependency must be avoided. Otherwise, custom condition/expression providers must be utilized.
Co-authored-by: Chris Müller <2566282+brotkrueml@users.noreply.github.com>
Co-authored-by: Chris Müller <2566282+brotkrueml@users.noreply.github.com>
Base variant conditions are evaluated when the Site object is built, using an expression language "site" context that receives no request. Therefore request-dependent functions such as ip() cannot be used; the previous "due to caching reasons" wording was inaccurate. - state the real cause and show the resulting exception (#1686745105) - link the API via :php-short: instead of a raw :t3src: file link - reword the intro of the available function list Follow-up to TYPO3-Documentation#5670 See https://forge.typo3.org/issues/106696 Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
3857ce9 to
0a5b4cd
Compare
Add a versionchanged directive explaining that request-dependent functions such as ip() worked in base variant conditions until v12 (they read the client address from the server environment) and were removed in v13.0, which is why they now fail. Helps users upgrading from v12 understand the behaviour change. See https://forge.typo3.org/issues/106696 Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
|
Thanks! I believe the proper place to discuss this would be in either a forge issue, gerrit patch - or maybe even a slack discussion in coredev. My personal feeling is that we should make the SiteEntity request-aware. This is helpful, but it should not rely on the to-be-phased-out TYPO3_REQUEST. The whole entity does too much and is not a good value object. So that would require thought, discussion and architecture decisions. Shortterm fix would be a "withRequest" or "setRequest" method here and pass it down the chain. Having said that, I think documenting the shortcoming for now to "fix" a wrong statement sounds helpful to me. We can always adapt the docs afterwards if the bug gets fixed. Which might even be v15 only due to involved changes. |
|
|
||
| Check whether a feature (":ref:`feature toggle <feature-toggles>`") is | ||
| enabled in TYPO3. | ||
|
|
There was a problem hiding this comment.
I think this should not be removed! Itcan't check at but isn't "request" actually handed in?
At the least we need to keep "traverse" but with a different array?!
There was a problem hiding this comment.
ah, got it, line 157 ff checking, seems like a mistake
There was a problem hiding this comment.
@garvinhicking AFAIU your removal of this is satisfied, as it is not possible to provide a working example, which would be the whole point of listing it here, isn't it? (request is not handed into the base-variant context in current core, that's exactly this issue; traverse() itself is request-independent, but there's no array variable available to demonstrate it with.)
For this discussion I started investigating in CybotTM/typo3#2 and CybotTM/typo3#3.
Maybe later. In any case, I won't open either of them until I'm sure, although you're certainly welcome to. Personally, I avoid the tools you mentioned as much as possible because they put too much mental strain on me for various reasons. Too many hurdles. (And the core team seems well aware of it as it seems to even utilize it as barrier: "Yes, PRs in ttps://github.com/TYPO3/typo3 have been disabled at the beginning of June, because we had issues with people publicly disclosing security issues. riccardodecontardi For the moment, you need to go through the default core contribution workflow (via Gerrit).") |

What & why
Follow-up to #5670 by @garvinhicking — this branch is based on his commits, so his (and @brotkrueml's co-authored) work is preserved in the history.
The documentation for base variant conditions currently claims that all functions from
DefaultFunctionsProviderare available. Request-dependent functions such asip()are not available there and fail with:(reported in forge #106696).
Root cause (verified in core)
Base variant conditions are evaluated in
Site::resolveBaseWithVariants(), which builds the expression language resolver for thesitecontext without a request variable:DefaultProvideronly suppliesapplicationContext,typo3,dateandfeatures— there is norequest, soip()(which requires aRequestWrapper) throws. This happens when theSiteobject is built — not because of caching, which an earlier revision of #5670 stated (thanks @froemken for the correction).What this changes on top of #5670
requestin thesiteexpression language context).#1686745105) so the page is found when searching for the error message.:php-short:instead of a raw:t3src:file link (as requested by @linawolf / @franzholz in the review of [DOCS] Document request independence for BaseVariants conditions #5670).ipand request-basedtraverseexamples stay removed (as in [DOCS] Document request independence for BaseVariants conditions #5670).Rendered before / after
main)Click a thumbnail to open the full-resolution render.
Decision required
Before merging, please decide whether this should be documented (this PR) or fixed in the core instead: there is an unresolved discussion in #5670 whether the
siteexpression language context should receive aRequestWrapper(@froemken's proposal), which would makeip()usable again. As of coremainthe limitation still exists, so this PR documents the current behaviour — if the core starts passing the request, this section should be reverted.Checklist
render-guides … --fail-on-logexits0