diff --git a/lib/liquid/context.rb b/lib/liquid/context.rb index ffeb81a8..ee948e14 100644 --- a/lib/liquid/context.rb +++ b/lib/liquid/context.rb @@ -199,19 +199,15 @@ module Liquid raise Liquid::UndefinedVariable, "undefined variable #{key}" end - original = obj[key] + value = obj[key] - value = contextualize(original) + # Skip contextualization and memoization when not found + return if value.nil? - # TODO: This memoization layer knows about the Proc behaviour and it does not put me at ease - # Also to note: VariableLookup command flags do not know about it which might be ok - # - # Original text from VariableLookup: if its a proc we will replace the entry with the proc - # - # Removing original.is_a?(Proc) from condition leads to: FrozenError: can't modify frozen Hash: {} - # Removing obj.respond_to?(:[]=) from condition leads to: Test suite passes - # obj[key] = value (no conditions): FrozenError + NoMethodError: undefined method `[]=' for - if original.is_a?(Proc) && obj.respond_to?(:[]=) + value = contextualize(value) + + # Memoization layer: Proc resolution and other to_liquid computations are persisted + if obj.respond_to?(:[]=) && !obj.frozen? obj[key] = value end @@ -233,21 +229,54 @@ module Liquid @disabled_tags.fetch(tag_name, 0) > 0 end - # TODO: Let's think as to how name this. - # The strait forward name is to use "to_liquid(object)" but I do not think this fully captures what it do. - # Also, "to_liquid" is already used anywhere and is more of a generic name at this point. - # When we call "to_liquid" we are "sanitizing" the input? - # We also want to "bind" the object to the current rendering "@context". + # Convert input objects into liquid aware representations + # Also assigns the context (self) through context= def contextualize(object) if object.is_a?(Proc) object = object.arity == 0 ? object.call : object.call(self) end - object = object.to_liquid + # TODO: This is a block of code that needs some extra polish + # My goal is to centralize as much as possible the contextualization/sanitization of objects being exchanged + # between the inputs given by the caller and templates being executed. + # I desire for Filters to not have to worry about object conversion (eg.: StandardFilters#each) + # Filters implemented outside Shopify/Liquid shouldn't have to worry about this layer of internals + # Using a filter shouldn't create a worry for data leak and missing context + # Running `ruby -I test` with the following implementation is successful + # Running `rake test` which will also run tests with the liquid-c gem will lead to errors + # This is due to liquid-c optimizing some code paths + # Eg.: https://github.com/Shopify/liquid-c/blame/master/ext/liquid_c/context.h#L44-L45 + # We would need to also add the following code in liquid-c + # If we were to only consider Array and Hash as special use cases, this might be worth the effort + # Alternatively I think considering Array and Hash as the only two cases of nested objects is not quite right + # It might be better for extension.rb to be responsible for this. Array#to_liquid to return self is somewhat be wrong + # One does not prevent the other, need to make sure the generic implementation works and we can optimize over it + # Note: Moving this logic to the different patches in extensions.rb + # Some changes in liquid-c is most likely required + # Liquid-c has early return optimization I have yet to track all code paths it relates to + # if (klass == rb_cString || klass == rb_cArray || klass == rb_cHash) + # return value; + if object.is_a?(Array) + object = object.map do |obj| + contextualize(obj) + end + elsif object.is_a?(Hash) + new_obj = {} + object.map do |k, obj| + new_obj[k] = contextualize(obj) + end + object = new_obj + else + object = object.to_liquid - # TODO: ideally all contextualized object would define "context=" even if they end up noop-ing it. - # For now this is not really a pressing issue to deal with. - object.context = self if object.respond_to?(:context=) + # TODO: Ideally all contextualized object would define "context=" even if they perform a noop + # We want to ensure non-liquid objects aren't leaked in the context visible to the templates + # Having a standard interface is a direction we can take. + # Alternatively, we could impose only a pre-determine array of available type is allowed + # Eg.: String, Integer, Symbol, Liquid::Drop not SomeCustomModelFromTheHostApplication + # For now this might not be as pressing issue to deal with + object.context = self if object.respond_to?(:context=) + end object end diff --git a/lib/liquid/standardfilters.rb b/lib/liquid/standardfilters.rb index 6ff1fcd6..efcd420a 100644 --- a/lib/liquid/standardfilters.rb +++ b/lib/liquid/standardfilters.rb @@ -582,10 +582,6 @@ module Liquid def each @input.each do |e| - # TODO: this conversion should occur outside the filter. - # This is a change I desire to make but might not make the cut for the current change at hand. - e = @context.contextualize(e) - yield(e) end end diff --git a/lib/liquid/variable_lookup.rb b/lib/liquid/variable_lookup.rb index fa6b5199..7f1dd145 100644 --- a/lib/liquid/variable_lookup.rb +++ b/lib/liquid/variable_lookup.rb @@ -68,9 +68,6 @@ module Liquid return nil unless context.strict_variables raise Liquid::UndefinedVariable, "undefined variable #{key}" end - - # If we are dealing with a drop here we have to - object.context = context if object.respond_to?(:context=) end object diff --git a/test/integration/drop_test.rb b/test/integration/drop_test.rb index 48beeba2..51caf8a9 100644 --- a/test/integration/drop_test.rb +++ b/test/integration/drop_test.rb @@ -149,6 +149,8 @@ class DropsTest < Minitest::Test assert_equal(' carrot ', output) end + # This test succeed in the ruby implementation, but not in liquid-c + # See Context#contextualize def test_context_drop_array_with_map output = Liquid::Template.parse(' {{ contexts | map: "bar" }} ').render!('contexts' => [ContextDrop.new, ContextDrop.new], 'bar' => "carrot") assert_equal(' carrotcarrot ', output) diff --git a/test/integration/standard_filter_test.rb b/test/integration/standard_filter_test.rb index 52dcb6a8..0fff6be4 100644 --- a/test/integration/standard_filter_test.rb +++ b/test/integration/standard_filter_test.rb @@ -417,6 +417,8 @@ class StandardFiltersTest < Minitest::Test assert_template_result("", '{{ "foo" | map: "inspect" }}') end + # This test succeed in the ruby implementation, but not in liquid-c + # See Context#contextualize def test_map_calls_to_liquid t = TestThing.new assert_template_result("woot: 1", '{{ foo | map: "whatever" }}', "foo" => [t]) @@ -433,6 +435,8 @@ class StandardFiltersTest < Minitest::Test assert_template_result("42", template, "thing" => hash) end + # This test succeed in the ruby implementation, but not in liquid-c + # See Context#contextualize def test_sort_calls_to_liquid t = TestThing.new Liquid::Template.parse('{{ foo | sort: "whatever" }}').render("foo" => [t])