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

Cycle context variables #1519

Open
jg-rp opened this issue Feb 14, 2022 · 1 comment · May be fixed by #1520
Open

Cycle context variables #1519

jg-rp opened this issue Feb 14, 2022 · 1 comment · May be fixed by #1520

Comments

@jg-rp
Copy link
Contributor

jg-rp commented Feb 14, 2022

Noting that documentation for the cycle tag only mentions cycling through lists of strings, and existing integration tests cover cycling through lists of integers, is the behaviour demonstrated by the following example considered a bug?

require 'liquid'

template = Liquid::Template.parse(
  "{% cycle a, b, c %} {% cycle a, b, c %} {% cycle a, b, c %}")

puts(template.render!({"a" => 1, "b" => 2, "c" => 3}))

Output:

1 1 1

Expected Output:

1 2 3

I can see that the cycle tag was changed with this commit to parse cycle arguments as expressions, rather than treating them as strings. It appears that a string representation of objects returned from parse_expression() doesn't make a good context key.

@jg-rp jg-rp linked a pull request Feb 14, 2022 that will close this issue
@dylanahsmith
Copy link
Contributor

Good catch, that was definitely an unintentional behaviour change, where the intention of that commit was only to move the parsing of expression objects to happen during template parsing rather than being repeated each time the tag is rendered.

I'm not sure why that behaviour wasn't covered by tests or noticed in the last 8 years. Unfortunately, we may how have liquid templates that rely on that bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants