render_to_output_buffer

.gitignore

@@ -7,3 +7,4 @@ pkg
 .ruby-version
 Gemfile.lock
 .bundle
+.byebug_history

.rubocop_todo.yml

@@ -1,6 +1,6 @@
 # This configuration was generated by
 # `rubocop --auto-gen-config`
-# on 2019-03-19 11:04:37 -0400 using RuboCop version 0.53.0.
+# on 2019-04-22 19:11:24 -0400 using RuboCop version 0.53.0.
 # The point is for the user to remove these configuration records
 # one by one as the offenses are removed from the code base.
 # Note that changes in the inspected code, or installation of new
@@ -46,18 +46,18 @@ Lint/Void:
   Exclude:
     - 'lib/liquid/parse_context.rb'
 
-# Offense count: 54
+# Offense count: 53
 Metrics/AbcSize:
   Max: 56
 
 # Offense count: 12
 Metrics/CyclomaticComplexity:
-  Max: 12
+  Max: 13
 
 # Offense count: 112
 # Configuration parameters: CountComments.
 Metrics/MethodLength:
-  Max: 37
+  Max: 38
 
 # Offense count: 8
 Metrics/PerceivedComplexity:
@@ -90,7 +90,7 @@ Naming/UncommunicativeMethodParamName:
     - 'test/integration/template_test.rb'
     - 'test/unit/condition_unit_test.rb'
 
-# Offense count: 10
+# Offense count: 12
 # Cop supports --auto-correct.
 # Configuration parameters: EnforcedStyle.
 # SupportedStyles: prefer_alias, prefer_alias_method
@@ -253,7 +253,7 @@ Style/WhileUntilModifier:
   Exclude:
     - 'lib/liquid/tags/case.rb'
 
-# Offense count: 640
+# Offense count: 648
 # Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns.
 # URISchemes: http, https
 Metrics/LineLength:

lib/liquid/block.rb

@@ -13,8 +13,9 @@ module Liquid
       end
     end
 
-    def render(context, output = '')
-      @body.render(context, output)
+    # For backwards compatibility
+    def render(context)
+      @body.render(context)
     end
 
     def blank?

lib/liquid/block_body.rb

@@ -66,14 +66,19 @@ module Liquid
       @blank
     end
 
-    def render(context, output = '')
+    def render(context)
+      render_to_output_buffer(context, '')
+    end
+
+    def render_to_output_buffer(context, output)
       context.resource_limits.render_score += @nodelist.length
 
       idx = 0
       while node = @nodelist[idx]
+        previous_output_size = output.bytesize
+
         case node
         when String
-          check_resources(context, node)
           output << node
         when Variable
           render_node(context, output, node)
@@ -91,6 +96,8 @@ module Liquid
           break if context.interrupt? # might have happened through an include
         end
         idx += 1
+
+        raise_if_resource_limits_reached(context, output.bytesize - previous_output_size)
       end
 
       output
@@ -99,10 +106,7 @@ module Liquid
     private
 
     def render_node(context, output, node)
-      node.render(context, output)
-      check_resources(context, output)
-    rescue MemoryError => e
-      raise e
+      node.render_to_output_buffer(context, output)
     rescue UndefinedVariable, UndefinedDropMethod, UndefinedFilter => e
       context.handle_error(e, node.line_number)
     rescue ::StandardError => e
@@ -110,8 +114,8 @@ module Liquid
       output << context.handle_error(e, line_number)
     end
 
-    def check_resources(context, output)
-      context.resource_limits.render_length = output.bytesize
+    def raise_if_resource_limits_reached(context, length)
+      context.resource_limits.render_length += length
       return unless context.resource_limits.reached?
       raise MemoryError.new("Memory limits exceeded".freeze)
     end

lib/liquid/profiler/hooks.rb

@@ -11,13 +11,13 @@ module Liquid
   end
 
   class Include < Tag
-    def render_with_profiling(context, output)
+    def render_to_output_buffer_with_profiling(context, output)
       Profiler.profile_children(context.evaluate(@template_name_expr).to_s) do
-        render_without_profiling(context, output)
+        render_to_output_buffer_without_profiling(context, output)
       end
     end
 
-    alias_method :render_without_profiling, :render
-    alias_method :render, :render_with_profiling
+    alias_method :render_to_output_buffer_without_profiling, :render_to_output_buffer
+    alias_method :render_to_output_buffer, :render_to_output_buffer_with_profiling
   end
 end

lib/liquid/tag.rb

@@ -32,7 +32,15 @@ module Liquid
       self.class.name.downcase
     end
 
-    def render(_context, output = '')
+    def render(_context)
+      ''.freeze
+    end
+
+    # For backwards compatibility with custom tags. In a future release, the semantics
+    # of the `render_to_output_buffer` method will become the default and the `render`
+    # method will be removed.
+    def render_to_output_buffer(context, output)
+      output << render(context)
       output
     end
 

lib/liquid/tags/assign.rb

@@ -22,8 +22,8 @@ module Liquid
       end
     end
 
-    def render(context, output = '')
-      val = @from.render(context, nil)
+    def render_to_output_buffer(context, output)
+      val = @from.render(context)
       context.scopes.last[@to] = val
       context.resource_limits.assign_score += assign_score_of(val)
       output

lib/liquid/tags/capture.rb

@@ -22,10 +22,11 @@ module Liquid
       end
     end
 
-    def render(context, output = '')
+    def render_to_output_buffer(context, output)
+      previous_output_size = output.bytesize
       super
       context.scopes.last[@to] = output
-      context.resource_limits.assign_score = output.bytesize
+      context.resource_limits.assign_score += (output.bytesize - previous_output_size)
       output
     end
 

lib/liquid/tags/case.rb

@@ -38,16 +38,16 @@ module Liquid
       end
     end
 
-    def render(context, output = '')
+    def render_to_output_buffer(context, output)
       context.stack do
         execute_else_block = true
 
         @blocks.each do |block|
           if block.else?
-            block.attachment.render(context, output) if execute_else_block
+            block.attachment.render_to_output_buffer(context, output) if execute_else_block
           elsif block.evaluate(context)
             execute_else_block = false
-            block.attachment.render(context, output)
+            block.attachment.render_to_output_buffer(context, output)
           end
         end
       end

lib/liquid/tags/comment.rb

@@ -1,6 +1,6 @@
 module Liquid
   class Comment < Block
-    def render(_context, output = '')
+    def render_to_output_buffer(_context, output)
       output
     end
 

lib/liquid/tags/cycle.rb

@@ -31,7 +31,7 @@ module Liquid
       end
     end
 
-    def render(context, output = '')
+    def render_to_output_buffer(context, output)
       context.registers[:cycle] ||= {}
 
       context.stack do

lib/liquid/tags/decrement.rb

@@ -23,7 +23,7 @@ module Liquid
       @variable = markup.strip
     end
 
-    def render(context, output = '')
+    def render_to_output_buffer(context, output)
       value = context.environments.first[@variable] ||= 0
       value -= 1
       context.environments.first[@variable] = value

lib/liquid/tags/for.rb

@@ -70,7 +70,7 @@ module Liquid
       @else_block = BlockBody.new
     end
 
-    def render(context, output = '')
+    def render_to_output_buffer(context, output)
       segment = collection_segment(context)
 
       if segment.empty?
@@ -78,6 +78,8 @@ module Liquid
       else
         render_segment(context, output, segment)
       end
+
+      output
     end
 
     protected
@@ -155,7 +157,7 @@ module Liquid
 
           segment.each do |item|
             context[@variable_name] = item
-            @for_block.render(context, output)
+            @for_block.render_to_output_buffer(context, output)
             loop_vars.send(:increment!)
 
             # Handle any interrupts if they exist.
@@ -188,7 +190,7 @@ module Liquid
 
     def render_else(context, output)
       if @else_block
-        @else_block.render(context, output)
+        @else_block.render_to_output_buffer(context, output)
       else
         output
       end

lib/liquid/tags/if.rb

@@ -39,11 +39,11 @@ module Liquid
       end
     end
 
-    def render(context, output)
+    def render_to_output_buffer(context, output)
       context.stack do
         @blocks.each do |block|
           if block.evaluate(context)
-            return block.attachment.render(context, output)
+            return block.attachment.render_to_output_buffer(context, output)
           end
         end
       end

lib/liquid/tags/ifchanged.rb

@@ -1,8 +1,9 @@
 module Liquid
   class Ifchanged < Block
-    def render(context, output)
+    def render_to_output_buffer(context, output)
       context.stack do
-        block_output = super(context, '')
+        block_output = ''
+        super(context, block_output)
 
         if block_output != context.registers[:ifchanged]
           context.registers[:ifchanged] = block_output

lib/liquid/tags/include.rb

@@ -42,7 +42,7 @@ module Liquid
     def parse(_tokens)
     end
 
-    def render(context, output)
+    def render_to_output_buffer(context, output)
       template_name = context.evaluate(@template_name_expr)
       raise ArgumentError.new(options[:locale].t("errors.argument.include")) unless template_name
 
@@ -68,11 +68,11 @@ module Liquid
           if variable.is_a?(Array)
             variable.each do |var|
               context[context_variable_name] = var
-              partial.render(context, output: output)
+              partial.render_to_output_buffer(context, output)
             end
           else
             context[context_variable_name] = variable
-            partial.render(context, output: output)
+            partial.render_to_output_buffer(context, output)
           end
         end
       ensure

lib/liquid/tags/increment.rb

@@ -20,7 +20,7 @@ module Liquid
       @variable = markup.strip
     end
 
-    def render(context, output = '')
+    def render_to_output_buffer(context, output)
       value = context.environments.first[@variable] ||= 0
       context.environments.first[@variable] = value + 1
       output << value.to_s

lib/liquid/tags/raw.rb

@@ -22,7 +22,7 @@ module Liquid
       raise SyntaxError.new(parse_context.locale.t("errors.syntax.tag_never_closed".freeze, block_name: block_name))
     end
 
-    def render(_context, output = '')
+    def render_to_output_buffer(_context, output)
       output << @body
       output
     end

lib/liquid/tags/table_row.rb

@@ -18,7 +18,7 @@ module Liquid
       end
     end
 
-    def render(context, output = '')
+    def render_to_output_buffer(context, output)
       collection = context.evaluate(@collection_name) or return ''.freeze
 
       from = @attributes.key?('offset'.freeze) ? context.evaluate(@attributes['offset'.freeze]).to_i : 0

lib/liquid/tags/unless.rb

@@ -6,18 +6,18 @@ module Liquid
   #   {% unless x < 0 %} x is greater than zero {% endunless %}
   #
   class Unless < If
-    def render(context, output = '')
+    def render_to_output_buffer(context, output)
       context.stack do
         # First condition is interpreted backwards ( if not )
         first_block = @blocks.first
         unless first_block.evaluate(context)
-          return first_block.attachment.render(context, output)
+          return first_block.attachment.render_to_output_buffer(context, output)
         end
 
         # After the first condition unless works just like if
         @blocks[1..-1].each do |block|
           if block.evaluate(context)
-            return block.attachment.render(context, output)
+            return block.attachment.render_to_output_buffer(context, output)
           end
         end
       end

lib/liquid/template.rb

@@ -207,11 +207,9 @@ module Liquid
       begin
         # render the nodelist.
         # for performance reasons we get an array back here. join will make a string out of it.
-        result = with_profiling(context) do
-          output ||= self.class.output_buffer.clear
-          @root.render(context, output)
+        with_profiling(context) do
+          @root.render_to_output_buffer(context, output || '')
         end
-        result.respond_to?(:join) ? result.join : result
       rescue Liquid::MemoryError => e
         context.handle_error(e)
       ensure
@@ -219,15 +217,15 @@ module Liquid
       end
     end
 
-    def self.output_buffer
-      @output_buffer ||= String.new(capacity: 1024)
-    end
-
     def render!(*args)
       @rethrow_errors = true
       render(*args)
     end
 
+    def render_to_output_buffer(context, output)
+      render(context, output: output)
+    end
+
     private
 
     def tokenize(source)

lib/liquid/variable.rb

@@ -78,30 +78,28 @@ module Liquid
       filterargs
     end
 
-    def render(context, output = '')
+    def render(context)
       obj = @filters.inject(context.evaluate(@name)) do |output, (filter_name, filter_args, filter_kwargs)|
         filter_args = evaluate_filter_expressions(context, filter_args, filter_kwargs)
         context.invoke(filter_name, output, *filter_args)
       end
 
       obj = context.apply_global_filter(obj)
-
       taint_check(context, obj)
+      obj
+    end
+
+    def render_to_output_buffer(context, output)
+      obj = render(context)
 
-      if output
       if obj.is_a?(Array)
         output << obj.join
       elsif obj.nil?
-        elsif !obj.is_a?(String)
-          output << obj.to_s
       else
-          output << obj
+        output << obj.to_s
       end
 
       output
-      else
-        obj
-      end
     end
 
     private

performance/shopify/comment_form.rb

@@ -12,7 +12,7 @@ class CommentForm < Liquid::Block
     end
   end
 
-  def render(context, output = '')
+  def render_to_output_buffer(context, output)
     article = context[@variable_name]
 
     context.stack do

performance/shopify/paginate.rb

@@ -21,7 +21,7 @@ class Paginate < Liquid::Block
     end
   end
 
-  def render(context, output = '')
+  def render_to_output_buffer(context, output)
     @context = context
 
     context.stack do

test/integration/blank_test.rb

@@ -1,12 +1,10 @@
 require 'test_helper'
 
 class FoobarTag < Liquid::Tag
-  def render(context, output = '')
+  def render_to_output_buffer(context, output)
     output << ' '
     output
   end
-
-  Liquid::Template.register_tag('foobar', FoobarTag)
 end
 
 class BlankTestFileSystem
@@ -32,8 +30,10 @@ class BlankTest < Minitest::Test
   end
 
   def test_new_tags_are_not_blank_by_default
+    with_custom_tag('foobar', FoobarTag) do
       assert_template_result(" " * N, wrap_in_for("{% foobar %}"))
     end
+  end
 
   def test_loops_are_blank
     assert_template_result("", wrap_in_for(" "))

test/integration/tags/include_tag_test.rb

@@ -66,7 +66,7 @@ class CustomInclude < Liquid::Tag
   def parse(tokens)
   end
 
-  def render(context, output = '')
+  def render_to_output_buffer(context, output)
     output << @template_name[1..-2]
     output
   end

test/integration/template_test.rb

@@ -177,31 +177,31 @@ class TemplateTest < Minitest::Test
 
   def test_render_length_persists_between_blocks
     t = Template.parse("{% if true %}aaaa{% endif %}")
-    t.resource_limits.render_length_limit = 3
+    t.resource_limits.render_length_limit = 7
     assert_equal "Liquid error: Memory limits exceeded", t.render
-    t.resource_limits.render_length_limit = 4
+    t.resource_limits.render_length_limit = 8
     assert_equal "aaaa", t.render
 
     t = Template.parse("{% if true %}aaaa{% endif %}{% if true %}bbb{% endif %}")
-    t.resource_limits.render_length_limit = 6
+    t.resource_limits.render_length_limit = 13
     assert_equal "Liquid error: Memory limits exceeded", t.render
-    t.resource_limits.render_length_limit = 7
+    t.resource_limits.render_length_limit = 14
     assert_equal "aaaabbb", t.render
 
     t = Template.parse("{% if true %}a{% endif %}{% if true %}b{% endif %}{% if true %}a{% endif %}{% if true %}b{% endif %}{% if true %}a{% endif %}{% if true %}b{% endif %}")
-    t.resource_limits.render_length_limit = 4
-    assert_equal "Liquid error: Memory limits exceeded", t.render
     t.resource_limits.render_length_limit = 5
     assert_equal "Liquid error: Memory limits exceeded", t.render
-    t.resource_limits.render_length_limit = 6
+    t.resource_limits.render_length_limit = 11
+    assert_equal "Liquid error: Memory limits exceeded", t.render
+    t.resource_limits.render_length_limit = 12
     assert_equal "ababab", t.render
   end
 
   def test_render_length_uses_number_of_bytes_not_characters
     t = Template.parse("{% if true %}すごい{% endif %}")
-    t.resource_limits.render_length_limit = 8
+    t.resource_limits.render_length_limit = 10
     assert_equal "Liquid error: Memory limits exceeded", t.render
-    t.resource_limits.render_length_limit = 9
+    t.resource_limits.render_length_limit = 18
     assert_equal "すごい", t.render
   end
 

test/test_helper.rb

@@ -84,6 +84,13 @@ module Minitest
     ensure
       Liquid::Template.error_mode = old_mode
     end
+
+    def with_custom_tag(tag_name, tag_class)
+      Liquid::Template.register_tag(tag_name, tag_class)
+      yield
+    ensure
+      Liquid::Template.tags.delete(tag_name)
+    end
   end
 end
 

test/unit/block_unit_test.rb

@@ -44,10 +44,47 @@ class BlockUnitTest < Minitest::Test
   end
 
   def test_with_custom_tag
-    Liquid::Template.register_tag("testtag", Block)
+    with_custom_tag('testtag', Block) do
       assert Liquid::Template.parse("{% testtag %} {% endtesttag %}")
-  ensure
-    Liquid::Template.tags.delete('testtag')
+    end
+  end
+
+  def test_custom_block_tags_have_a_default_render_to_output_buffer_method_for_backwards_compatibility
+    klass1 = Class.new(Block) do
+      def render(*)
+        'hello'
+      end
+    end
+
+    with_custom_tag('blabla', klass1) do
+      template = Liquid::Template.parse("{% blabla %} bla {% endblabla %}")
+
+      assert_equal 'hello', template.render
+
+      buf = ''
+      output = template.render({}, output: buf)
+      assert_equal 'hello', output
+      assert_equal 'hello', buf
+      assert_equal buf.object_id, output.object_id
+    end
+
+    klass2 = Class.new(klass1) do
+      def render(*)
+        'foo' + super + 'bar'
+      end
+    end
+
+    with_custom_tag('blabla', klass2) do
+      template = Liquid::Template.parse("{% blabla %} foo {% endblabla %}")
+
+      assert_equal 'foohellobar', template.render
+
+      buf = ''
+      output = template.render({}, output: buf)
+      assert_equal 'foohellobar', output
+      assert_equal 'foohellobar', buf
+      assert_equal buf.object_id, output.object_id
+    end
   end
 
   private

test/unit/tag_unit_test.rb

@@ -18,4 +18,42 @@ class TagUnitTest < Minitest::Test
     tag = Tag.parse("some_tag", "", Tokenizer.new(""), ParseContext.new)
     assert_equal 'some_tag', tag.tag_name
   end
+
+  def test_custom_tags_have_a_default_render_to_output_buffer_method_for_backwards_compatibility
+    klass1 = Class.new(Tag) do
+      def render(*)
+        'hello'
+      end
+    end
+
+    with_custom_tag('blabla', klass1) do
+      template = Liquid::Template.parse("{% blabla %}")
+
+      assert_equal 'hello', template.render
+
+      buf = ''
+      output = template.render({}, output: buf)
+      assert_equal 'hello', output
+      assert_equal 'hello', buf
+      assert_equal buf.object_id, output.object_id
+    end
+
+    klass2 = Class.new(klass1) do
+      def render(*)
+        'foo' + super + 'bar'
+      end
+    end
+
+    with_custom_tag('blabla', klass2) do
+      template = Liquid::Template.parse("{% blabla %}")
+
+      assert_equal 'foohellobar', template.render
+
+      buf = ''
+      output = template.render({}, output: buf)
+      assert_equal 'foohellobar', output
+      assert_equal 'foohellobar', buf
+      assert_equal buf.object_id, output.object_id
+    end
+  end
 end