Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: View helper result only available in child tags since 7.0.0 #1897

Closed
fishbone1 opened this issue Feb 6, 2024 · 4 comments
Closed

BUG: View helper result only available in child tags since 7.0.0 #1897

fishbone1 opened this issue Feb 6, 2024 · 4 comments

Comments

@fishbone1
Copy link

I have checked that the bug exists in the dev-development branch
No.
(There are no relevant commits since 7.0.2)

I have checked that there are no already open issues or recently closed issues about this bug
Yes.

Describe the bug
As of version 7.0.0 several view helpers which store the result in a variable given by parameter as don't work anymore like before.

For example, our Textpic template modifies an array of CSS classes depending on its content element settings. Finally, it implodes the array to get the CSS class string:

(...)
    <f:comment><!-- below, right, right center --></f:comment>
    <v:condition.iterator.contains needle="{data.imageorient}" haystack="{0:8,1:25,2:125}">
        <v:iterator.push subject="{imgColClasses}" add="order-last" as="imgColClasses" />
    </v:condition.iterator.contains>

    <f:comment><!-- left center, right center --></f:comment>
    <v:condition.iterator.contains needle="{data.imageorient}" haystack="{0:125,1:126}">
        <v:iterator.push subject="{imgColClasses}" add="d-flex flex-column justify-content-center" as="imgColClasses" />
        <v:iterator.push subject="{textColClasses}" add="d-flex flex-column justify-content-center" as="textColClasses" />
    </v:condition.iterator.contains>

    <v:iterator.implode content="{imgColClasses}" glue=" " as="imgColClasses" />
    <v:iterator.implode content="{textColClasses}" glue=" " as="textColClasses" />
(...)

This works fine until 6.1.3. Since 7.0.0 behavior changed. The result of iterator.implode and iterator.push operations will only be available in the tag body:

    <v:iterator.implode content="{imgColClasses}" glue=" " as="imgColClasses">
        <!-- 'col-md-auto' -->
        <f:debug>{imgColClasses}</f:debug>
    </v:iterator.implode>

    <!-- NULL -->
    <f:debug>{imgColClasses}</f:debug>

Expected behavior
NULL output above should contain a string with CSS classes like in the view helper tag body.

Additional context
Looking at the code, it seems that "it's not a bug but a feature". It rather has been a bug in the past, because I couldn't see any relevant change at that place, but the view helper calls a method that creates a backup of the variable context before and restores it after rendering the tag. That method's name "renderChildrenWithVariableOrReturnInputStatic" gives a hint about doing so.

However, I wonder what the use of such behavior could be. If there are multiple variables (imgColClasses and textColClasses) and if they are modified multiple times like in my example, the tag nesting required would be really ugly. Inline notation is also hard to write and read (imho).

Also, I couldn't find any hint in the docs that the variable is only available for child elements, so I'd be surprised.

If this is really the intended it would be great if all of those view helpers had an additional parameter to switch between 6.* and 7.* behavior.

Because with version 6.* we programmed everything and as everything worked like expected we also assumed having understood the docs correctly. Changing all the code probably will be lots of work.

@NamelessCoder
Copy link
Member

@fishbone1 If ViewHelpers before did not remove the variable after assigning it through the as argument, then that is considered a bug ("leaking" variables). The only ViewHelpers which are allowed to leak variables is f:variable (core) and v:variable.set (which exists for legacy reasons only). These ViewHelpers are designed to assign or overwrite new variables. With version 7 a rather significant bug was fixed: before this, the method in question would "restore" backed up variables but would use the overridden value instead of the original value. So I'm sorry to say but the strategy you used earlier unfortunately depended on a bug that was fixed and won't be reintroduced.

This fixed bug affects every ViewHelper which uses the as argument.

In the future you would also start running into more and more problems with variable leaking, since Fluid itself is starting to make widespread use of a "local variables" concept: TYPO3/Fluid@cdd96af (I don't agree with this decision and it will probably interfere with or even break the way f:variable is intended to work within a range of ViewHelpers - but it's out of my hands).

it would be great if all of those view helpers had an additional parameter to switch between 6.* and 7.* behavior.

Although this would allow you to solve your issue with less effort, This won't be done - leaking variables is absolutely considered a bug except for the very specific cases of variable-assigning ViewHelpers.

For your case: unless you specifically require the CSS classes to be an array - for example if you need to also remove values, run iterator.unique on them, or count the number of values - I would recommend simply using string concatenation:

<f:variable name="cssClasses">{cssClasses} added-class-1 added-class-2</f:variable>

It's shorter, doesn't depend on a bug, does not require iterator.implode and is relatively easy to reach from your current starting point - about as easy as it would be to add a new argument to all of the involved ViewHelpers.

For the record: f:variable can also be used in tag mode to re-assign variables modified by the mentioned ViewHelpers:

<f:variable name="imgColClasses"><v:iterator.implode content="{imgColClasses}" glue=" " /></f:variable>

Although I would very much recommend inline syntax for something like that especially if you need to perform more than one or two operations on the same variable:

{imgColClasses -> v:iterator.implode(glue: ' ') -> f:variable(name: 'imgColClasses')}

@fishbone1
Copy link
Author

@NamelessCoder Thank you very much for the detailed answer. I was afraid of that...
You say that only f:variable is allowed to leak variables. Is that a Fluid rule? If true, I wonder why they didn't just limit custom view helpers to work this way. Or is it a rule defined by vhs? Is it documented?

@NamelessCoder
Copy link
Member

Is that a Fluid rule?

Since any ViewHelper can decide to leak variables: no, it's a convention but not a strict rule. The convention is in place to make sure that any ViewHelper which uses the as argument will handle the assignment the same way. The convention began in Fluid (with f:for and f:cycle) and VHS implements it as well for consistency.

I wonder why they didn't just limit custom view helpers to work this way

That is what may happen, see the link I gave earlier. Which would be pretty bad if they do; both for complexity and flexibility.

Is it documented?

Implicitly, yes. The descriptions of various ViewHelpers like f:variable, f:for, f:cycle, f:alias etc. all explain how the variables are assigned in terms of scope. VHS doesn't consistently document this (it does in many cases but not all) even though the as argument behavior is always implemented the same way.

@NamelessCoder
Copy link
Member

Closing the issue since the problem is solved in VHS 7.x and the workarounds described in this issue can work around it on VHS 6.x.

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

No branches or pull requests

2 participants