diff --git a/lib/liquid/tags/cycle.rb b/lib/liquid/tags/cycle.rb index 5a2048fc..60f44d04 100644 --- a/lib/liquid/tags/cycle.rb +++ b/lib/liquid/tags/cycle.rb @@ -58,19 +58,27 @@ module Liquid def rigid_parse(markup) p = @parse_context.new_parser(markup) - if p.look(:id) && p.look(:colon, 1) - @name = p.consume(:id) - @is_named = true - p.consume(:colon) - end - @variables = [] raise SyntaxError, options[:locale].t("errors.syntax.cycle") if p.look(:end_of_string) - while (var = safe_parse_expression(p)) - @variables << var - break unless p.consume?(:comma) + first_expression = safe_parse_expression(p) + if p.look(:colon) + # cycle name: expr1, expr2, ... + @name = first_expression + @is_named = true + p.consume(:colon) + # After the colon, parse the first variable (required for named cycles) + @variables << maybe_dup_lookup(safe_parse_expression(p)) + else + # cycle expr1, expr2, ... + @variables << maybe_dup_lookup(first_expression) + end + + # Parse remaining comma-separated expressions + while p.consume?(:comma) + break if p.look(:end_of_string) + @variables << maybe_dup_lookup(safe_parse_expression(p)) end p.consume(:end_of_string) @@ -106,14 +114,22 @@ module Liquid var =~ /\s*(#{QuotedFragment})\s*/o next unless Regexp.last_match(1) - # Expression Parser returns cached objects, and we need to dup them to - # start the cycle over for each new cycle call. - # Liquid-C does not have a cache, so we don't need to dup the object. var = parse_expression(Regexp.last_match(1)) - var.is_a?(VariableLookup) ? var.dup : var + maybe_dup_lookup(var) end.compact end + # For backwards compatibility, whenever a lookup is used in an unnamed cycle, + # we make it so that the @variables.to_s produces different strings for cycles + # called with the same arguments (since @variables.to_s is used as the cycle counter key) + # This makes it so {% cycle a, b %} and {% cycle a, b %} have independent counters even if a and b share value. + # This is not true for literal values, {% cycle "a", "b" %} and {% cycle "a", "b" %} share the same counter. + # I was really scratching my head about this one, but migrating away from this would be more headache + # than it's worth. So we're keeping this quirk for now. + def maybe_dup_lookup(var) + var.is_a?(VariableLookup) ? var.dup : var + end + class ParseTreeVisitor < Liquid::ParseTreeVisitor def children Array(@node.variables) diff --git a/test/integration/tags/cycle_tag_test.rb b/test/integration/tags/cycle_tag_test.rb index dde8a1bc..b0ee6925 100644 --- a/test/integration/tags/cycle_tag_test.rb +++ b/test/integration/tags/cycle_tag_test.rb @@ -3,20 +3,11 @@ require 'test_helper' class CycleTagTest < Minitest::Test - def test_simple_cycle - template = <<~LIQUID - {%- cycle '1', '2', '3' -%} - {%- cycle '1', '2', '3' -%} - {%- cycle '1', '2', '3' -%} - LIQUID - - assert_template_result("123", template) - end def test_simple_cycle_inside_for_loop template = <<~LIQUID {%- for i in (1..3) -%} - {% cycle '1', '2', '3' %} + {%- cycle '1', '2', '3' -%} {%- endfor -%} LIQUID @@ -36,14 +27,61 @@ class CycleTagTest < Minitest::Test assert_template_result("123", template) end - def test_cycle_tag_always_resets_cycle + def test_cycle_named_groups_string template = <<~LIQUID - {%- assign a = "1" -%} - {%- cycle a, "2" -%} - {%- cycle a, "2" -%} + {%- for i in (1..3) -%} + {%- cycle 'placeholder1': 1, 2, 3 -%} + {%- cycle 'placeholder2': 1, 2, 3 -%} + {%- endfor -%} LIQUID - assert_template_result("11", template) + assert_template_result("112233", template) + end + + def test_cycle_named_groups_vlookup + template = <<~LIQUID + {%- assign placeholder1 = 'placeholder1' -%} + {%- assign placeholder2 = 'placeholder2' -%} + {%- for i in (1..3) -%} + {%- cycle placeholder1: 1, 2, 3 -%} + {%- cycle placeholder2: 1, 2, 3 -%} + {%- endfor -%} + LIQUID + + assert_template_result("112233", template) + end + + def test_unnamed_cycle_have_independent_counters_when_used_with_lookups + template = <<~LIQUID + {%- assign a = "1" -%} + {%- for i in (1..3) -%} + {%- cycle a, "2" -%} + {%- cycle a, "2" -%} + {%- endfor -%} + LIQUID + + assert_template_result("112211", template) + end + + def test_unnamed_cycle_dependent_counter_when_used_with_literal_values + template = <<~LIQUID + {%- cycle "1", "2" -%} + {%- cycle "1", "2" -%} + {%- cycle "1", "2" -%} + LIQUID + + assert_template_result("121", template) + end + + def test_optional_trailing_comma + template = <<~LIQUID + {%- cycle "1", "2", -%} + {%- cycle "1", "2", -%} + {%- cycle "1", "2", -%} + {%- cycle "1", -%} + LIQUID + + assert_template_result("1211", template) end def test_cycle_tag_without_arguments @@ -56,19 +94,19 @@ class CycleTagTest < Minitest::Test def test_cycle_tag_with_error_mode # QuotedFragment is more permissive than what Parser#expression allows. - temlate1 = "{% assign 5 = 'b' %}{% cycle .5, .4 %}" - temlate2 = "{% cycle .5: 'a', 'b' %}" + template1 = "{% assign 5 = 'b' %}{% cycle .5, .4 %}" + template2 = "{% cycle .5: 'a', 'b' %}" [:lax, :strict].each do |mode| with_error_mode(mode) do - assert_template_result("b", temlate1) - assert_template_result("a", temlate2) + assert_template_result("b", template1) + assert_template_result("a", template2) end end with_error_mode(:rigid) do - error1 = assert_raises(Liquid::SyntaxError) { Template.parse(temlate1) } - error2 = assert_raises(Liquid::SyntaxError) { Template.parse(temlate2) } + error1 = assert_raises(Liquid::SyntaxError) { Template.parse(template1) } + error2 = assert_raises(Liquid::SyntaxError) { Template.parse(template2) } expected_error = /Liquid syntax error: \[:dot, "."\] is not a valid expression/