Remove a line feed for CI

lib/liquid.rb

@@ -57,6 +57,8 @@ require 'liquid/forloop_drop'
 require 'liquid/extensions'
 require 'liquid/errors'
 require 'liquid/interrupts'
+require 'liquid/filter'
+require 'liquid/filter_template'
 require 'liquid/strainer_factory'
 require 'liquid/strainer_template'
 require 'liquid/expression'

lib/liquid/filter.rb

@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+require 'set'
+
+module Liquid
+  # A filter in liquid is a class which contain invokable logic from liquid templates.
+  #
+  # Public methods in filter classes are callable.
+  #
+  # The use for liquid filters is to make logic functions available to the web designers.
+  #
+  # Example:
+  #
+  #   class StringFilter < Liquid::Filter
+  #     def upcase(input)
+  #       input.upcase
+  #     end
+  #   end
+  #
+  #   tmpl = Liquid::Template.parse('Result: {{ "test" | upcase }}')
+  #   tmpl.render({}, filters: [StringFilter])
+  #   => "Result: TEST"
+  class Filter
+    class << self
+      def invokable_methods
+        @invokable_methods ||= begin
+          blacklist = Liquid::Filter.public_instance_methods
+          whitelist = public_instance_methods - blacklist
+
+          Set.new(whitelist.map(&:to_s))
+        end
+      end
+    end
+
+    def initialize(context)
+      @context = context
+    end
+
+    private
+
+    attr_reader :context
+  end
+end

lib/liquid/filter_template.rb

@@ -0,0 +1,40 @@
+# frozen_string_literal: true
+
+require 'set'
+
+module Liquid
+  # FilterTemplate is the computed class for the filters system.
+  #
+  # Historically Liquid used to include filters as Module to the context strainer.
+  # This lead to the absence of sandbox between filters (one filter could override private methods of another filter).
+  #
+  # With the implementation of Liquid::Filter, it is now possible for the modules from legacy code to be automatically
+  #   wrapped into a Liquid::Filter generated class.
+  #
+  # This should not be considered as the base behaviour, it is preferred to create filters going forward directly as
+  #   classes that are child of Liquid::Filter.
+  class FilterTemplate < Filter
+    class << self
+      def include(mod)
+        super
+
+        @init_module = mod
+      end
+
+      # Override of the `invokable_methods`.
+      # We can't rely on the parent logic as some modules might have been defining methods that shadow Class methods.
+      #
+      # Eg.:
+      # mod = Liquid::StandardFilters
+      # filter = Class.new(FilterTemplate)
+      # filter.include(mod)
+      # mod.public_instance_methods - (filter.public_instance_methods - Class.public_instance_methods)
+      # => [:prepend]
+      def invokable_methods
+        whitelist = @init_module.public_instance_methods
+
+        @invokable_methods ||= Set.new(whitelist.map(&:to_s))
+      end
+    end
+  end
+end

lib/liquid/strainer_template.rb

@@ -14,40 +14,69 @@ module Liquid
     end
 
     class << self
-      def add_filter(filter)
-        return if include?(filter)
-
-        invokable_non_public_methods = (filter.private_instance_methods + filter.protected_instance_methods).select { |m| invokable?(m) }
-        if invokable_non_public_methods.any?
-          raise MethodOverrideError, "Filter overrides registered public methods as non public: #{invokable_non_public_methods.join(', ')}"
+      def add_filter(mod)
+        filter = if mod.is_a?(Class) && mod.ancestors.include?(Liquid::Filter)
+          mod
+        elsif mod.instance_of?(Module)
+          convert_mod_to_filter(mod)
+        else
+          raise(ArgumentError, "wrong argument type Proc (expected Liquid::Filter)")
         end
 
-        include(filter)
+        filter.invokable_methods.each do |method|
+          filter_method_map[method] = filter
+        end
+      end
 
-        filter_methods.merge(filter.public_instance_methods.map(&:to_s))
+      def filter_for(method)
+        filter_method_map[method]
       end
 
       def invokable?(method)
-        filter_methods.include?(method.to_s)
+        filter_method_map.key?(method)
       end
 
       private
 
-      def filter_methods
-        @filter_methods ||= Set.new
+      def filter_method_map
+        @filter_method_map ||= {}
+      end
+
+      def convert_mod_to_filter(mod)
+        @filter_classes ||= {}
+        @filter_classes[mod] ||= begin
+          klass = Class.new(FilterTemplate)
+          klass.include(mod)
+          klass
+        end
       end
     end
 
     def invoke(method, *args)
       if self.class.invokable?(method)
-        send(method, *args)
-      elsif @context.strict_filters
-        raise Liquid::UndefinedFilter, "undefined filter #{method}"
+        begin
+          instance = filter_instance_for(method)
+          instance.public_send(method, *args)
+        rescue ::ArgumentError => e
+          raise Liquid::ArgumentError, e.message, e.backtrace
+        end
+      elsif context.strict_filters
+        raise(Liquid::UndefinedFilter, "undefined filter #{method}")
       else
         args.first
       end
-    rescue ::ArgumentError => e
-      raise Liquid::ArgumentError, e.message, e.backtrace
     end
+
+    private
+
+    def filter_instance_for(method)
+      @filter_instances ||= {}
+      @filter_instances.fetch(method) do
+        klass = self.class.filter_for(method)
+        klass.new(context)
+      end
+    end
+
+    attr_reader :context
   end
 end

test/unit/context_unit_test.rb

@@ -147,13 +147,13 @@ class ContextUnitTest < Minitest::Test
 
     context = Context.new
     context.add_filters(filter)
-    assert_equal('hi? hi!', context.invoke(:hi, 'hi?'))
+    assert_equal('hi? hi!', context.invoke('hi', 'hi?'))
 
     context = Context.new
-    assert_equal('hi?', context.invoke(:hi, 'hi?'))
+    assert_equal('hi?', context.invoke('hi', 'hi?'))
 
     context.add_filters(filter)
-    assert_equal('hi? hi!', context.invoke(:hi, 'hi?'))
+    assert_equal('hi? hi!', context.invoke('hi', 'hi?'))
   end
 
   def test_only_intended_filters_make_it_there

test/unit/strainer_factory_unit_test.rb

@@ -82,16 +82,6 @@ class StrainerFactoryUnitTest < Minitest::Test
     assert_equal("has_method?", strainer.invoke("invoke", "has_method?", "invoke"))
   end
 
-  def test_strainer_uses_a_class_cache_to_avoid_method_cache_invalidation
-    a = Module.new
-    b = Module.new
-    strainer = StrainerFactory.create(@context, [a, b])
-    assert_kind_of(StrainerTemplate, strainer)
-    assert_kind_of(a, strainer)
-    assert_kind_of(b, strainer)
-    assert_kind_of(Liquid::StandardFilters, strainer)
-  end
-
   def test_add_global_filter_clears_cache
     assert_equal('input', StrainerFactory.create(@context).invoke('late_added_filter', 'input'))
     StrainerFactory.add_global_filter(LateAddedFilter)

test/unit/strainer_template_unit_test.rb

@@ -10,73 +10,9 @@ class StrainerTemplateUnitTest < Minitest::Test
     s = c.strainer
     wrong_filter = ->(v) { v.reverse }
 
-    exception = assert_raises(TypeError) do
+    exception = assert_raises(ArgumentError) do
       s.class.add_filter(wrong_filter)
     end
-    assert_equal(exception.message, "wrong argument type Proc (expected Module)")
-  end
-
-  module PrivateMethodOverrideFilter
-    private
-
-    def public_filter
-      "overriden as private"
-    end
-  end
-
-  def test_add_filter_raises_when_module_privately_overrides_registered_public_methods
-    strainer = Context.new.strainer
-
-    error = assert_raises(Liquid::MethodOverrideError) do
-      strainer.class.add_filter(PrivateMethodOverrideFilter)
-    end
-    assert_equal('Liquid error: Filter overrides registered public methods as non public: public_filter', error.message)
-  end
-
-  module ProtectedMethodOverrideFilter
-    protected
-
-    def public_filter
-      "overriden as protected"
-    end
-  end
-
-  def test_add_filter_raises_when_module_overrides_registered_public_method_as_protected
-    strainer = Context.new.strainer
-
-    error = assert_raises(Liquid::MethodOverrideError) do
-      strainer.class.add_filter(ProtectedMethodOverrideFilter)
-    end
-    assert_equal('Liquid error: Filter overrides registered public methods as non public: public_filter', error.message)
-  end
-
-  module PublicMethodOverrideFilter
-    def public_filter
-      "public"
-    end
-  end
-
-  def test_add_filter_does_not_raise_when_module_overrides_previously_registered_method
-    strainer = Context.new.strainer
-    with_global_filter do
-      strainer.class.add_filter(PublicMethodOverrideFilter)
-      assert(strainer.class.send(:filter_methods).include?('public_filter'))
-    end
-  end
-
-  def test_add_filter_does_not_include_already_included_module
-    mod = Module.new do
-      class << self
-        attr_accessor :include_count
-        def included(_mod)
-          self.include_count += 1
-        end
-      end
-      self.include_count = 0
-    end
-    strainer = Context.new.strainer
-    strainer.class.add_filter(mod)
-    strainer.class.add_filter(mod)
-    assert_equal(1, mod.include_count)
+    assert_equal(exception.message, "Liquid error: wrong argument type Proc (expected Liquid::Filter)")
   end
 end