diff --git a/.gitignore b/.gitignore index 7ac01c1..90bf6dc 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,4 @@ pkg .ruby-version Gemfile.lock .bundle +.byebug_history diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 6868a7a..4af3202 100644 --- a/.rubocop_todo.yml +++ b/.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: diff --git a/lib/liquid/block.rb b/lib/liquid/block.rb index 843b6b7..549a3a3 100644 --- a/lib/liquid/block.rb +++ b/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? diff --git a/lib/liquid/block_body.rb b/lib/liquid/block_body.rb index f404d20..c2478ce 100644 --- a/lib/liquid/block_body.rb +++ b/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 diff --git a/lib/liquid/profiler/hooks.rb b/lib/liquid/profiler/hooks.rb index 112a7d3..cda166b 100644 --- a/lib/liquid/profiler/hooks.rb +++ b/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 diff --git a/lib/liquid/tag.rb b/lib/liquid/tag.rb index d9d4ff7..5099ccb 100644 --- a/lib/liquid/tag.rb +++ b/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 diff --git a/lib/liquid/tags/assign.rb b/lib/liquid/tags/assign.rb index 62a4747..767d874 100644 --- a/lib/liquid/tags/assign.rb +++ b/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 diff --git a/lib/liquid/tags/capture.rb b/lib/liquid/tags/capture.rb index b4d418b..d717b76 100644 --- a/lib/liquid/tags/capture.rb +++ b/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 diff --git a/lib/liquid/tags/case.rb b/lib/liquid/tags/case.rb index 385c878..92b2ed0 100644 --- a/lib/liquid/tags/case.rb +++ b/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 diff --git a/lib/liquid/tags/comment.rb b/lib/liquid/tags/comment.rb index cb7739f..cad3931 100644 --- a/lib/liquid/tags/comment.rb +++ b/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 diff --git a/lib/liquid/tags/cycle.rb b/lib/liquid/tags/cycle.rb index cbcff79..e42244d 100644 --- a/lib/liquid/tags/cycle.rb +++ b/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 diff --git a/lib/liquid/tags/decrement.rb b/lib/liquid/tags/decrement.rb index 9cf3073..08ddd4d 100644 --- a/lib/liquid/tags/decrement.rb +++ b/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 diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb index 01df484..8dceab0 100644 --- a/lib/liquid/tags/for.rb +++ b/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 diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index a1e4eec..25534a9 100644 --- a/lib/liquid/tags/if.rb +++ b/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 diff --git a/lib/liquid/tags/ifchanged.rb b/lib/liquid/tags/ifchanged.rb index 2c026f1..e3040ce 100644 --- a/lib/liquid/tags/ifchanged.rb +++ b/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 diff --git a/lib/liquid/tags/include.rb b/lib/liquid/tags/include.rb index 28a7481..24acf9d 100644 --- a/lib/liquid/tags/include.rb +++ b/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 diff --git a/lib/liquid/tags/increment.rb b/lib/liquid/tags/increment.rb index 4182c46..5af1242 100644 --- a/lib/liquid/tags/increment.rb +++ b/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 diff --git a/lib/liquid/tags/raw.rb b/lib/liquid/tags/raw.rb index 4f6aa15..4fa75d9 100644 --- a/lib/liquid/tags/raw.rb +++ b/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 diff --git a/lib/liquid/tags/table_row.rb b/lib/liquid/tags/table_row.rb index 09b69e9..9532102 100644 --- a/lib/liquid/tags/table_row.rb +++ b/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 diff --git a/lib/liquid/tags/unless.rb b/lib/liquid/tags/unless.rb index 8d4c32e..18856c3 100644 --- a/lib/liquid/tags/unless.rb +++ b/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 diff --git a/lib/liquid/template.rb b/lib/liquid/template.rb index c22f164..35db674 100644 --- a/lib/liquid/template.rb +++ b/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) diff --git a/lib/liquid/variable.rb b/lib/liquid/variable.rb index 5e03706..cbf9986 100644 --- a/lib/liquid/variable.rb +++ b/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 - 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 - end + def render_to_output_buffer(context, output) + obj = render(context) - output + if obj.is_a?(Array) + output << obj.join + elsif obj.nil? else - obj + output << obj.to_s end + + output end private diff --git a/performance/shopify/comment_form.rb b/performance/shopify/comment_form.rb index ce33a2c..7b5bd53 100644 --- a/performance/shopify/comment_form.rb +++ b/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 diff --git a/performance/shopify/paginate.rb b/performance/shopify/paginate.rb index 875e58b..0abd11f 100644 --- a/performance/shopify/paginate.rb +++ b/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 diff --git a/test/integration/blank_test.rb b/test/integration/blank_test.rb index cc09781..2b46ad7 100644 --- a/test/integration/blank_test.rb +++ b/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,7 +30,9 @@ class BlankTest < Minitest::Test end def test_new_tags_are_not_blank_by_default - assert_template_result(" " * N, wrap_in_for("{% foobar %}")) + with_custom_tag('foobar', FoobarTag) do + assert_template_result(" " * N, wrap_in_for("{% foobar %}")) + end end def test_loops_are_blank diff --git a/test/integration/tags/include_tag_test.rb b/test/integration/tags/include_tag_test.rb index cf83f59..14bb8c3 100644 --- a/test/integration/tags/include_tag_test.rb +++ b/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 diff --git a/test/integration/template_test.rb b/test/integration/template_test.rb index c8b7770..0dc0ae5 100644 --- a/test/integration/template_test.rb +++ b/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 diff --git a/test/test_helper.rb b/test/test_helper.rb index ac5ab53..34e7553 100755 --- a/test/test_helper.rb +++ b/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 diff --git a/test/unit/block_unit_test.rb b/test/unit/block_unit_test.rb index 6a27a7d..9f7b94f 100644 --- a/test/unit/block_unit_test.rb +++ b/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) - assert Liquid::Template.parse("{% testtag %} {% endtesttag %}") - ensure - Liquid::Template.tags.delete('testtag') + with_custom_tag('testtag', Block) do + assert Liquid::Template.parse("{% testtag %} {% endtesttag %}") + 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 diff --git a/test/unit/tag_unit_test.rb b/test/unit/tag_unit_test.rb index c4b901b..a3fb40e 100644 --- a/test/unit/tag_unit_test.rb +++ b/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