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

[FEATURE] Proper support of local variables in ViewHelpers #828

Merged
merged 3 commits into from
Nov 21, 2023

Conversation

s2b
Copy link
Contributor

@s2b s2b commented Nov 20, 2023

In its current implementation, the f:alias ViewHelper has side effects: It adds variables to the global variable container, which are not restored properly after the ViewHelper call. If a variable name is used as alias which already exists, that variable is unset after the ViewHelper call.

To resolve this, a new ScopedVariableProvider is introduced to handle local variables
inside ViewHelper calls properly. After the local scope, global variables are restored to their original value in case they existed. At the same time, other modifications to the variable provider inside the f:alias call, such as a call to f:variable, should still be persistent after the ViewHelper call. This ensures that the change is non-breaking for the vast majority of usages. The only situation where this could be breaking is when you relied on the arguably broken behavior that f:alias unsets the variables afterwards.

This is also the reason why we can't simple store and reset the whole variable container. This would be a breaking change and would also be a diversion from other ViewHelpers, such as f:if, where f:variable also leaks out. To change this behavior for select ViewHelpers would be inconsistent and hard to understand for users of Fluid.

before:

<f:variable name="myVariable" value="initialValue" />
<f:alias map="{myVariable: \'myValue\'}">...</f:alias>
{myVariable} <!-- outputs empty string -->

after:

<f:variable name="myVariable" value="initialValue" />
<f:alias map="{myVariable: \'myValue\'}">...</f:alias>
{myVariable} <!-- outputs "initialValue" -->

This behavior also applies to other ViewHelpers that provide variables to their child nodes, such as f:for, f:render, f:groupedFor, f:cycle. These will be modified accordingly in separate PRs.

Resolves #666

@s2b s2b force-pushed the bugfix/aliasWithoutSideEffects branch from bd08a45 to 90fe573 Compare November 20, 2023 12:01
Currently, the VariableProviderInterface suggests that its implementations
can receive an array of variables as a constructor argument to be used
as initial state of the provider instance. However, this assumption is
already not correct anymore because ChainedVariableProvider receives
an array of VariableProvider instances instead. In addition, this
restricts possible other implementations of the interface.

Noteworthy in the code and relevant projects using Fluid:

* Neos implements its own VariableProvider which extends StandardVariableProvider. The change has no effect there.
https://github.com/neos/fluidadaptor/blob/8.3/Classes/Core/ViewHelper/TemplateVariableContainer.php

* Fluid uses the constructor mostly with concrete implementations (mostly StandardVariableProvider, but also ChainedVariableProvider).

* ConsoleRunner uses a dynamic class name, but no constructor arguments.
https://github.com/TYPO3/Fluid/blob/main/src/Tools/ConsoleRunner.php#L116C68-L116C68

* flux, vhs and fluid-parameters only use StandardVariableProvider

* Fluid Components and Fluid Styleguide only use StandardVariableProvider
@s2b s2b force-pushed the bugfix/aliasWithoutSideEffects branch from 90fe573 to 2550b22 Compare November 21, 2023 14:01
@mbrodala
Copy link
Member

Only a few words: bam! 💥

@s2b s2b force-pushed the bugfix/aliasWithoutSideEffects branch from 2550b22 to c554b1d Compare November 21, 2023 14:07
@s2b s2b changed the title [BUGFIX] Eliminate side-effects from AliasViewHelper [FEATURE] Proper support of local variables in ViewHelpers Nov 21, 2023
@lolli42 lolli42 force-pushed the bugfix/aliasWithoutSideEffects branch from c554b1d to 9bd7d7d Compare November 21, 2023 17:12
@lolli42 lolli42 merged commit cdd96af into main Nov 21, 2023
4 checks passed
@lolli42 lolli42 deleted the bugfix/aliasWithoutSideEffects branch November 21, 2023 17:17
@mbrodala
Copy link
Member

Thanks a lot for this missing piece!

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.

AliasViewHelper has side effects
3 participants