Bug #103875
closedConditions are ignored in sys template
Added by Florian Rival 3 months ago. Updated about 1 month ago.
100%
Description
If I have a TypoScript file included with a condition in sys_template , field config , like that:
<INCLUDE_TYPOSCRIPT: source="DIR:EXT:my_ext/Configuration/TypoScript/" condition="[tree.level >= 2]">
or even like this :
[tree.level >= 2] @import 'EXT:my_ext/Configuration/TypoScript/*.typoscript' [END]
the condition is not applied and the TypoScript is always applied (i.e. the file is always included).
If I set the same condition inside the TypoScript file itself then the condition work as expected.
[tree.level >= 2] lib.myVariable = TEXT lib.myVariable.value = IT WORKS [END]
The conditions defined in sys_template field config are not event displayed in module "TypoScript > Active TypoScript > Setup > Conditions".
Updated by Florian Rival 3 months ago
I was wrong, this:
[tree.level >= 2] @import 'EXT:my_ext/Configuration/TypoScript/*.typoscript' [END]
works as expected.
The problem is only for:
<INCLUDE_TYPOSCRIPT: source="DIR:EXT:my_ext/Configuration/TypoScript/" condition="[tree.level >= 2]">
and it's located here:
File core/Classes/TypoScript/Tokenizer/LossyTokenizer.php , method parseImportOld():
while (true) { $nextChar = $importBodyChars[$importBodyCharCount] ?? null; ... if ($nextChar === '>') { if ($importBodyCharCount) { $importBodyToken = new Token(TokenType::T_VALUE, $importBody); $this->lineStream->append((new ImportOldLine())->setValueToken($importBodyToken)); break; } break; } }
because of the line:
if ($nextChar === '>') {
then the process stops at the first ">" and then the condition is truncated.
This will happen with conditions like:
<INCLUDE_TYPOSCRIPT: source="DIR:EXT:my_ext/Configuration/TypoScript/" condition="[tree.level >= 2]">
or like:
<INCLUDE_TYPOSCRIPT: source="DIR:EXT:my_ext/Configuration/TypoScript/" condition="[tree.level > 2]">
Updated by Gerrit Code Review 3 months ago
- Status changed from New to Under Review
Patch set 2 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/84346
Updated by Christian Kuhn 3 months ago
- Related to Feature #97816: New TypoScript parser added
Updated by Gerrit Code Review 3 months ago
Patch set 3 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/84346
Updated by Gerrit Code Review 3 months ago
Patch set 4 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/84346
Updated by Christian Kuhn 3 months ago
· Edited
Hey.
I added some tests to look at this at https://review.typo3.org/c/Packages/TYPO3.CMS/+/84346/4
First, set 4 is a patch for LossyTokenizer at the moment, LosslessTokenizer has still to be done (which is why one test fails).
Apart from that, I'm a bit unsure if the solution is really the path to follow.
Here are my thoughts:
INCLUDE_TYPOSCRIPT is the ugly child since v12 with @import being quick and with @import being allowed to be nested within "true" conditions since v12. v13 is about to actually deprecate INCLUDE_TYPOSCRIPT in favor of @import - this is prepared already, just the final patch is missing. This way we get rid of the ugly INCLUDE_TYPOSCRIPT syntax entirely.
As such, we're dealing with a bug in legacy in the first place, and people should switch to @import anyways.
Still, we may want to tackle this bug since it is nasty, and it's far from trivial for integrators to actually recognize they are affected by this. Since INCLUDE_TYPOSCRIPT is "old" and essentially code to be removed, I'd be open for a hacky solution as well - there is no true need to have a beautiful solution for some code that is slated for removal.
As such, let's first look at a possible "clean" solution, so we at least know how it would look like:
INCLUDE_TYPOSCRIPT already has issues with some chars in this ugly inline condition syntax. Example:
<INCLUDE_TYPOSCRIPT: condition="[applicationContext matches "/^Production/"]"
The above won't work as expected: We have additional " chars within the condition, that destroy the condition="..." scoping. The solution is documented with 'INCLUDE_TYPOSCRIPT FILE EXT file with complex condition restriction 1' from TreeFromLineStreamBuilderTest (and the next test below) - the " within the condition block need to be quoted \" to tell the parser we're not at the end of the condition part, yet.
<INCLUDE_TYPOSCRIPT: condition="[applicationContext matches \"/^Production/\"]"
As far as I can see, this detail isn't even documented ...
However, the solution for the > sign should be essentially the same: If you have a > within a condition="..." part, the > should be quoted as \>, to tell the parser, this is not the end of char of <INCLUDE_TYPOSCRIPT: ... >". The implementation for this is not within TreeFromLineStreamBuilder, but in LossyTokenizer and LosslessTokenizer, but would be essentially the same (we'd still add another test to TreeFromLineStreamBuilder to see if that is actually hacked into correct pieces).
Forcing > within condition="..." to be quoted however is breaking, since that was parsed without issues with v11 and below. This renders this "clean" solution rather unfortunate.
The solution proposed by Florian is to guess if we're within the condition="..." part, to take away the special meaning of > in this case. It uses the " char to do that: If one has been opened and a > char is there, were in such a block. This is however more guesswork than good parsing. And considering the already special meaning of " as outlined above, I'm sure it would be possible to break the solution somehow again. So it's probably risky doing that. I think adapting the solution to LosslessTokenizer as well, should be do-able, though.
I basically see these options to continue here:
1. go with proposed solution and hope for the best
2. do not fix that issue at all and tell people who run into it to switch to @import within a condition directly, which they'll have to do with v14 latest, anyways, when INCLUDE_TYPOSCRIPT is gone
3. implement a solution with quoting and adapt .rst files to tell people they need to use \> to quote-away the termination behavior of >
What do you think?
Updated by Christian Kuhn 3 months ago
First feedback from Stefan: It seems as if hardening the proposed solution and going with it seems to be the best tradeoff.
Updated by Florian Rival 3 months ago
IMHO the second solution is not viable, because it is not easy to automate the transformation from this syntax:
<INCLUDE_TYPOSCRIPT: source="DIR:EXT:my_ext/Configuration/TypoScript/" condition="[tree.level >= 2]">
to this one:
[tree.level >= 2]
@import 'EXT:my_ext/Configuration/TypoScript/*.typoscript'
[END]
As indicated in your last comment, strengthening the proposed solution seems to be the best compromise and as you say in your comment "it's far from trivial for integrators to actually recognize they are affected by this" I think it's better for the integrator that this problem is solved without manual intervention on their part.
Updated by Gerrit Code Review 3 months ago
Patch set 5 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/84346
Updated by Gerrit Code Review 3 months ago
Patch set 6 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/84346
Updated by Gerrit Code Review 3 months ago
Patch set 7 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/84346
Updated by Gerrit Code Review 3 months ago
Patch set 8 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/84346
Updated by Stephan Großberndt 3 months ago
- Subject changed from Condtions are ignored in sys template to Conditions are ignored in sys template
Updated by Gerrit Code Review 3 months ago
Patch set 1 for branch 12.4 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/84436
Updated by Christian Kuhn 3 months ago
- Status changed from Under Review to Resolved
- % Done changed from 0 to 100
Applied in changeset b1797f9d77c69fafd73b42a0d2931e30c06a6921.