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

Strange handling of FALSE with include #1002

Open
AlphaHasi opened this issue Apr 3, 2018 · 3 comments
Open

Strange handling of FALSE with include #1002

AlphaHasi opened this issue Apr 3, 2018 · 3 comments

Comments

@AlphaHasi
Copy link

AlphaHasi commented Apr 3, 2018

Inside a template

{% assign var1 = false %}
{% unless var1 %}
  {% assign var1 = 'give var1 a value' %}
{% endunless %}
{% unless var2 %}
  {% assign var2 = 'give var2 a value' %}
{% endunless %}
{{ var1 }} {{ var2 }}

The output is give var1 a value give var2 a value.

If there is a parent with

{% assign var1 = false %}
{% include 'second_template' %}

and then inside the second template

{% unless var1 %}
  {% assign var1 = 'give var1 a value' %}
{% endunless %}
{% unless var2 %}
  {% assign var2 = 'give var2 a value' %}
{% endunless %}
{{ var1 }} {{ var2 }}

The output is also give var1 a value give var2 a value.

But if you include like this

{% include 'second_template', var1: false %}

and then inside the second template again

{% unless var1 %}
  {% assign var1 = 'give var1 a value' %}
{% endunless %}
{% unless var2 %}
  {% assign var2 = 'give var2 a value' %}
{% endunless %}
{{ var1 }} {{ var2 }}

The output is false give var2 a value.

The odd thing is, that unless is still triggered, but false is not overwritten and just used as plain output. If this is meant as feature it should be like that for all variables, but for example an empty string can get overwritten if a test with if/unless fails.

@dylanahsmith
Copy link
Contributor

It looks like what is happening is that the assign (or capture) tags assign to the bottom of liquid's scope stack. This means those variables are global and not scoped depending on what block tag they appear within. The include tag attributes are assigned to a new scope that is pushed on the stack, similar to scopes created by the block tags: case, cycle, for, if, unless, ifchanged, table_row. So those scope variable always get precedence over the variables set with assign or capture.

I agree that the behaviour is confusing, although changing the behaviour seems likely to break existing templates given that it looks like it has always worked this way.

To avoid this problem, simply avoid re-using the same variable name for include tag attributes as assign/capture variables.

@AlphaHasi
Copy link
Author

Thanks!

Yes, I understand the problem, that you can't change a system that is in use so widely. I thought it is a bug, but so it seems to be like that on purpose.

I used new names now. Again. — The need to use new names for several things gets very fast out of hand, some includes, for-loops, also "creating arrays" for these loops with the help of generated strings, includes of snippets, usage of CMS-tags/metafield-strings to create links and filenames (e.g. for external pictures), append that very often comes with own rules, comparison that breaks silently etc.

With Shopify/liquid I am very fast on a huge amount of nearly unreadable code lines for things that could be done in other template "languages" in some single lines, easy to read and understandable. "Liquid is just a template language", yes. On Shopify the company I currently work for, has several shops and a huge need of flexible maintainable content and views, we can't add a lot of apps to enhance the back-end part, so a lot of things happens in the view. Liquid is very difficult to use, if you don't just plug&play.

Just for adding some confusion to the above question:

Parent:

{% include 'second_template', var1: '' %}
show parent: _{{ var1 }}_<br>

second_template.liquid:

{% assign var1 = 'give var1 a value' %}
show child: _{{ var1 }}_<br>

Output:

show child: __
show parent: _give var1 a value_

So even when I try to understand the scope, I can't. It is prevented from overwriting inside the child and gives back the original value, but the parent then later has the value that was supposed to update the value inside the child …

But you can close this issue, as it seems to be on purpose like that.

@dylanahsmith
Copy link
Contributor

I thought it is a bug, but so it seems to be like that on purpose.

I'm not certain it was done on purpose. It could be that this interaction between the two decisions of assign/capture variables being global and include tag attributes being scoped to the templates had an unconsidered side effect.

I think the behaviour might be more intuitive if the tag attributes were assigned to the bottom of the stack so they behaved as assign/capture variables, but with the previous values of these attributes being saved and restored around the render of the included template.

With the current behaviour, I don't think it would normally make sense to assign/capture to a variable that has been shadowed by a variable on the stack. So perhaps strict_variables: true should treat this as an error so that it is possible to surface a warning. We would also need a check like that if we were going to assess the impact of a change to that behaviour.

It also seems like this behaviour should be documented, given that it isn't intuitive.

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