Remove to_liquid conversion from StandardFilters

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
+      value = contextualize(value)
-      # Also to note: VariableLookup command flags do not know about it which might be ok
+
-      #
+      # Memoization layer: Proc resolution and other to_liquid computations are persisted
-      # Original text from VariableLookup: if its a proc we will replace the entry with the proc
+      if obj.respond_to?(:[]=) && !obj.frozen?
-      #
-      # 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 <ObjectInContext>
-      if original.is_a?(Proc) && obj.respond_to?(:[]=)
         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.
+    # Convert input objects into liquid aware representations
-    # The strait forward name is to use "to_liquid(object)" but I do not think this fully captures what it do.
+    # Also assigns the context (self) through context=
-    # 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".
     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.
+        # TODO: Ideally all contextualized object would define "context=" even if they perform a noop
-      # For now this is not really a pressing issue to deal with.
+        # We want to ensure non-liquid objects aren't leaked in the context visible to the templates
-      object.context = self if object.respond_to?(:context=)
+        # 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

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

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

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)

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])