From 239cfa5a440fd9b210cd011c53afed74bab3cac9 Mon Sep 17 00:00:00 2001 From: Justin Li Date: Tue, 12 May 2015 16:11:32 -0400 Subject: [PATCH 1/4] Use find_variable for parentloop --- lib/liquid/tags/for.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb index 33f12af..05b1b93 100644 --- a/lib/liquid/tags/for.rb +++ b/lib/liquid/tags/for.rb @@ -99,7 +99,7 @@ module Liquid # Store our progress through the collection for the continue flag context.registers[:for][@name] = from + segment.length - parent_loop = context['forloop'.freeze] + parent_loop = context.find_variable('forloop'.freeze) context.stack do segment.each_with_index do |item, index| From 4c9d2009f9f653afa3a8a1c090191d69f2a77406 Mon Sep 17 00:00:00 2001 From: Justin Li Date: Tue, 12 May 2015 16:49:39 -0400 Subject: [PATCH 2/4] Add find_own_variable method to look up internal context variables --- lib/liquid/context.rb | 13 ++++++++++++- lib/liquid/tags/for.rb | 2 +- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/lib/liquid/context.rb b/lib/liquid/context.rb index e93dcba..8f74afe 100644 --- a/lib/liquid/context.rb +++ b/lib/liquid/context.rb @@ -177,7 +177,18 @@ module Liquid variable = variable.to_liquid variable.context = self if variable.respond_to?(:context=) - return variable + variable + end + + def find_own_variable(key) + index = @scopes.find_index { |s| s.has_key?(key) } + return unless index + + scope = @scopes[index] + variable = lookup_and_evaluate(@scopes[index], key).to_liquid + variable.context = self if variable.respond_to?(:context=) + + variable end def lookup_and_evaluate(obj, key) diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb index 05b1b93..643e1a8 100644 --- a/lib/liquid/tags/for.rb +++ b/lib/liquid/tags/for.rb @@ -99,7 +99,7 @@ module Liquid # Store our progress through the collection for the continue flag context.registers[:for][@name] = from + segment.length - parent_loop = context.find_variable('forloop'.freeze) + parent_loop = context.find_own_variable('forloop'.freeze) context.stack do segment.each_with_index do |item, index| From 863e8968f0e51b9d87cf51326b3cd073a0a34f87 Mon Sep 17 00:00:00 2001 From: Justin Li Date: Tue, 12 May 2015 17:04:34 -0400 Subject: [PATCH 3/4] Use extra stack for forloop references --- lib/liquid/context.rb | 13 +------------ lib/liquid/tags/for.rb | 17 ++++++++++++----- 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/lib/liquid/context.rb b/lib/liquid/context.rb index 8f74afe..e93dcba 100644 --- a/lib/liquid/context.rb +++ b/lib/liquid/context.rb @@ -177,18 +177,7 @@ module Liquid variable = variable.to_liquid variable.context = self if variable.respond_to?(:context=) - variable - end - - def find_own_variable(key) - index = @scopes.find_index { |s| s.has_key?(key) } - return unless index - - scope = @scopes[index] - variable = lookup_and_evaluate(@scopes[index], key).to_liquid - variable.context = self if variable.respond_to?(:context=) - - variable + return variable end def lookup_and_evaluate(obj, key) diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb index 643e1a8..578002a 100644 --- a/lib/liquid/tags/for.rb +++ b/lib/liquid/tags/for.rb @@ -69,7 +69,8 @@ module Liquid end def render(context) - context.registers[:for] ||= Hash.new(0) + for_offsets = context.registers[:for] ||= Hash.new(0) + for_stack = context.registers[:for_stack] ||= [] collection = context.evaluate(@collection_name) collection = collection.to_a if collection.is_a?(Range) @@ -78,7 +79,7 @@ module Liquid return render_else(context) unless iterable?(collection) from = if @from == :continue - context.registers[:for][@name].to_i + for_offsets[@name].to_i else context.evaluate(@from).to_i end @@ -97,14 +98,15 @@ module Liquid length = segment.length # Store our progress through the collection for the continue flag - context.registers[:for][@name] = from + segment.length + for_offsets[@name] = from + segment.length - parent_loop = context.find_own_variable('forloop'.freeze) + parent_loop = for_stack.last + for_stack.push(nil) context.stack do segment.each_with_index do |item, index| context[@variable_name] = item - context['forloop'.freeze] = { + loop_vars = { 'name'.freeze => @name, 'length'.freeze => length, 'index'.freeze => index + 1, @@ -116,6 +118,9 @@ module Liquid 'parentloop'.freeze => parent_loop } + context['forloop'.freeze] = loop_vars + for_stack[-1] = loop_vars + result << @for_block.render(context) # Handle any interrupts if they exist. @@ -126,6 +131,8 @@ module Liquid end end end + + for_stack.pop result end From 648a4888af6b95c1da2d61690f86daba1a33dbdb Mon Sep 17 00:00:00 2001 From: Justin Li Date: Thu, 14 May 2015 15:02:20 -0400 Subject: [PATCH 4/4] Pop the for_stack register in an ensure --- lib/liquid/tags/for.rb | 3 ++- test/integration/error_handling_test.rb | 19 ------------------- test/integration/tags/for_tag_test.rb | 10 ++++++++++ test/test_helper.rb | 19 +++++++++++++++++++ 4 files changed, 31 insertions(+), 20 deletions(-) diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb index 578002a..95defc7 100644 --- a/lib/liquid/tags/for.rb +++ b/lib/liquid/tags/for.rb @@ -132,8 +132,9 @@ module Liquid end end - for_stack.pop result + ensure + for_stack.pop end protected diff --git a/test/integration/error_handling_test.rb b/test/integration/error_handling_test.rb index c21b4b4..468808c 100644 --- a/test/integration/error_handling_test.rb +++ b/test/integration/error_handling_test.rb @@ -1,24 +1,5 @@ require 'test_helper' -class ErrorDrop < Liquid::Drop - def standard_error - raise Liquid::StandardError, 'standard error' - end - - def argument_error - raise Liquid::ArgumentError, 'argument error' - end - - def syntax_error - raise Liquid::SyntaxError, 'syntax error' - end - - def exception - raise Exception, 'exception' - end - -end - class ErrorHandlingTest < Minitest::Test include Liquid diff --git a/test/integration/tags/for_tag_test.rb b/test/integration/tags/for_tag_test.rb index 23752bb..968ba5d 100644 --- a/test/integration/tags/for_tag_test.rb +++ b/test/integration/tags/for_tag_test.rb @@ -388,4 +388,14 @@ HERE assert_template_result(expected, template, loader_assigns) assert_template_result(expected, template, array_assigns) end + + def test_for_cleans_up_registers + context = Context.new(ErrorDrop.new) + + assert_raises(StandardError) do + Liquid::Template.parse('{% for i in (1..2) %}{{ standard_error }}{% endfor %}').render!(context) + end + + assert context.registers[:for_stack].empty? + end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 32f8204..3c27113 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -88,3 +88,22 @@ class ThingWithToLiquid 'foobar' end end + +class ErrorDrop < Liquid::Drop + def standard_error + raise Liquid::StandardError, 'standard error' + end + + def argument_error + raise Liquid::ArgumentError, 'argument error' + end + + def syntax_error + raise Liquid::SyntaxError, 'syntax error' + end + + def exception + raise Exception, 'exception' + end +end +