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

f:variable not working correctly inside f:for VH #846

Closed
timothebot opened this issue Dec 4, 2023 · 7 comments · Fixed by #849
Closed

f:variable not working correctly inside f:for VH #846

timothebot opened this issue Dec 4, 2023 · 7 comments · Fixed by #849

Comments

@timothebot
Copy link

This commit changed the behaviour of the "as" variable. Previously, it was added to the global Template Variable Container, now it gets added to the Local Variable Container.

What I don't understand is why you can't update the as variable inside the loop anymore:

<f:for each="{myNumbers}" as="number">
    <f:if condition="{number} == 2">
        <!-- code below gets executed, but does not change the variable -->
        {f:variable(name: 'number', value: '99')}
    </f:if>
    <div>
        {number}
    </div>
</f:for>

This used to work, but does not work anymore. It makes sense that the variable can't be accessed outside the loop, but why is it static inside it?
It would make sense that you can update it inside the loop.

@mbrodala
Copy link
Contributor

mbrodala commented Dec 4, 2023

Because f:variable affects the global scope, so it basically sets a global variable number unrelated to the local variable number here. You should be able to see this by debugging {_all} after the f:for, there should be your number having the value 99.

And of course, variable access prefers the local scope.

@timothebot
Copy link
Author

You should be able to see this by debugging {_all} after the f:for, there should be your number having the value 99.

Yes, but in my opinion, this behaviour is wrong. In theory, you now have two variables with the same name inside the loop, but you can only access one. This does not make sense.

If you pass the "as" variable down using the render VH, it works as intended:

<f:for each="{myNumbers}" as="number">
    <f:render section="MySection" arguments="{_all}" />
</f:for>

<f:section name="MySection">
    <f:if condition="{number} == 2">
        <!-- Now it works -->
        {f:variable(name: 'number', value: '99')}
    </f:if>
    <div>
        {number}
    </div>
</f:section>

The behaviour is weird and inconsistent. You should be able to update the "as" variable inside the loop, without affecting the global scope.

@s2b
Copy link
Contributor

s2b commented Dec 8, 2023

Hi @timothebot! The change to differentiate between global and local variables was made to make Fluid more predictable in other areas. However, I agree that the behavior you describe here is a problem.

One possible solution could be to "convert" an existing local variable to a global variable once it's set explicitly via VariableViewHelper. I don't know if this will have other side effects, but do you think that this could work for your case?

In practice this would mean that <f:variable name="myForVariable" value="123" /> would do the following:

$globalVariables->add('myForVariable', 123);
$localVariables->remove('myForVariable');

@s2b
Copy link
Contributor

s2b commented Dec 9, 2023

@timothebot Could you check if the attached PR fixes your issue?

@timothebot
Copy link
Author

@s2b It does fix the issue and it's definitely an improvement, but I still think it's weird that loops behave differently in Fluid than in PHP.

@s2b
Copy link
Contributor

s2b commented Dec 11, 2023

What exactly do you mean? Loops also behaved differently before these changes: The "local" variables were reset after the loop and could not be used afterwards.

@timothebot
Copy link
Author

@s2b ah yes, sorry, you are right. In this case it's okay 👍

@s2b s2b closed this as completed in #849 Dec 11, 2023
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 a pull request may close this issue.

3 participants