From 3077cdf921fed35c1e743db5911d7e4b589a684e Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 13 Feb 2019 15:35:20 -0800 Subject: [PATCH 1/5] rename push / pop function --- actionview/lib/action_view/base.rb | 2 +- actionview/lib/action_view/helpers/rendering_helper.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/actionview/lib/action_view/base.rb b/actionview/lib/action_view/base.rb index 556a5ee502..be2a662adc 100644 --- a/actionview/lib/action_view/base.rb +++ b/actionview/lib/action_view/base.rb @@ -286,7 +286,7 @@ module ActionView #:nodoc: self.class end - def in_context(options, locals) + def in_rendering_context(options) old_view_renderer = @view_renderer old_lookup_context = @lookup_context diff --git a/actionview/lib/action_view/helpers/rendering_helper.rb b/actionview/lib/action_view/helpers/rendering_helper.rb index 7323963c72..7ead691113 100644 --- a/actionview/lib/action_view/helpers/rendering_helper.rb +++ b/actionview/lib/action_view/helpers/rendering_helper.rb @@ -27,7 +27,7 @@ module ActionView def render(options = {}, locals = {}, &block) case options when Hash - in_context(options, locals) do |renderer| + in_rendering_context(options) do |renderer| if block_given? view_renderer.render_partial(self, options.merge(partial: options[:layout]), &block) else From 4f77982e213e31d9c6e7195c7db86429b8092744 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 13 Feb 2019 17:08:42 -0800 Subject: [PATCH 2/5] Add a test that writes to the collection cache --- actionview/test/template/render_test.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/actionview/test/template/render_test.rb b/actionview/test/template/render_test.rb index 5068e00c7d..8d28f71585 100644 --- a/actionview/test/template/render_test.rb +++ b/actionview/test/template/render_test.rb @@ -743,10 +743,17 @@ class CachedCollectionViewRenderTest < ActiveSupport::TestCase end teardown do - GC.start I18n.reload! end + test "template body written to cache" do + customer = Customer.new("david", 1) + key = cache_key(customer, "test/_customer") + assert_nil ActionView::PartialRenderer.collection_cache.read(key) + @view.render(partial: "test/customer", collection: [customer], cached: true) + assert_equal "Hello: david", ActionView::PartialRenderer.collection_cache.read(key) + end + test "collection caching does not cache by default" do customer = Customer.new("david", 1) key = cache_key(customer, "test/_customer") From d0733ba8c0528aa4bc6e890e189afaac62ebe58f Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 13 Feb 2019 17:17:48 -0800 Subject: [PATCH 3/5] Move inline rendering content-type test to a controller test This commit is to remove direct access to the "rendered_format" attribute on the lookup context. "rendered_format" is an implementation detail that we shouldn't test directly. --- actionview/test/actionpack/controller/render_test.rb | 11 +++++++++++ actionview/test/template/render_test.rb | 5 ----- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/actionview/test/actionpack/controller/render_test.rb b/actionview/test/actionpack/controller/render_test.rb index 727d3fbc1a..52c3c54d96 100644 --- a/actionview/test/actionpack/controller/render_test.rb +++ b/actionview/test/actionpack/controller/render_test.rb @@ -174,6 +174,10 @@ class TestController < ActionController::Base render inline: "<%= controller_name %>" end + def inline_rendered_format_without_format + render inline: "test" + end + # :ported: def render_custom_code render plain: "hello world", status: 404 @@ -659,6 +663,7 @@ class RenderTest < ActionController::TestCase get :hello_world_from_rxml_using_action, to: "test#hello_world_from_rxml_using_action" get :hello_world_from_rxml_using_template, to: "test#hello_world_from_rxml_using_template" get :hello_world_with_layout_false, to: "test#hello_world_with_layout_false" + get :inline_rendered_format_without_format, to: "test#inline_rendered_format_without_format" get :layout_overriding_layout, to: "test#layout_overriding_layout" get :layout_test, to: "test#layout_test" get :layout_test_with_different_layout, to: "test#layout_test_with_different_layout" @@ -1015,6 +1020,12 @@ class RenderTest < ActionController::TestCase assert_equal "\n\n

Hello

\n

This is grand!

\n\n
\n", @response.body end + def test_rendered_format_without_format + get :inline_rendered_format_without_format + assert_equal "test", @response.body + assert_equal "text/html", @response.content_type + end + def test_partials_list get :partials_list assert_equal "goodbyeHello: davidHello: marygoodbye\n", @response.body diff --git a/actionview/test/template/render_test.rb b/actionview/test/template/render_test.rb index 8d28f71585..ee8a110b44 100644 --- a/actionview/test/template/render_test.rb +++ b/actionview/test/template/render_test.rb @@ -69,11 +69,6 @@ module RenderTestCases assert_match "No Comment", @view.render(template: "comments/empty", formats: [:xml]) end - def test_rendered_format_without_format - @view.render(inline: "test") - assert_equal :html, @view.lookup_context.rendered_format - end - def test_render_partial_implicitly_use_format_of_the_rendered_template @view.lookup_context.formats = [:json] assert_equal "Hello world", @view.render(template: "test/one", formats: [:html]) From 1bc0a59d6ee198fa535c04f32527a1d6228b647d Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 13 Feb 2019 17:59:21 -0800 Subject: [PATCH 4/5] Return rendered template information instead of just strings This commit introduces "rendered template" and "rendered collection" objects. The template renderers can now return a more complex object than just strings. This allows the framework to get more information about the templates that were rendered. In this commit we use the rendered template object to set the "rendered_format" on the lookup context in the controller rather than all the way in the template renderer. That means we don't need to check the "rendered_format" every time we render a template, we just do it once after all templates have been rendered. --- .../action_view/renderer/abstract_renderer.rb | 48 +++++++++++++++++++ .../action_view/renderer/partial_renderer.rb | 25 ++++------ .../partial_renderer/collection_caching.rb | 10 ++-- .../lib/action_view/renderer/renderer.rb | 20 ++++++-- .../renderer/streaming_template_renderer.rb | 2 +- .../action_view/renderer/template_renderer.rb | 9 ++-- actionview/lib/action_view/rendering.rb | 9 +++- 7 files changed, 92 insertions(+), 31 deletions(-) diff --git a/actionview/lib/action_view/renderer/abstract_renderer.rb b/actionview/lib/action_view/renderer/abstract_renderer.rb index ae366ce54a..2a51f2ef99 100644 --- a/actionview/lib/action_view/renderer/abstract_renderer.rb +++ b/actionview/lib/action_view/renderer/abstract_renderer.rb @@ -27,6 +27,46 @@ module ActionView raise NotImplementedError end + class RenderedCollection # :nodoc: + attr_reader :rendered_templates + + def initialize(rendered_templates, spacer) + @rendered_templates = rendered_templates + @spacer = spacer + end + + def body + @rendered_templates.map(&:body).join(@spacer.body).html_safe + end + + def format + rendered_templates.first.format + end + + class EmptyCollection + def format; nil; end + def body; nil; end + end + + EMPTY = EmptyCollection.new + end + + class RenderedTemplate # :nodoc: + attr_reader :body, :layout, :template + + def initialize(body, layout, template) + @body = body + @layout = layout + @template = template + end + + def format + template.formats.first + end + + EMPTY_SPACER = Struct.new(:body).new + end + private def extract_details(options) # :doc: @@ -49,5 +89,13 @@ module ActionView @lookup_context.formats = formats | @lookup_context.formats end + + def build_rendered_template(content, layout, template) + RenderedTemplate.new content, layout, template + end + + def build_rendered_collection(templates, spacer) + RenderedCollection.new(templates, spacer) + end end end diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index f8a6f13ae9..f4319b8528 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -314,14 +314,6 @@ module ActionView template = nil end - @lookup_context.rendered_format ||= begin - if template && template.formats.first - template.formats.first - else - formats.first - end - end - if @collection render_collection(context, template) else @@ -334,10 +326,13 @@ module ActionView def render_collection(view, template) identifier = (template && template.identifier) || @path instrument(:collection, identifier: identifier, count: @collection.size) do |payload| - return nil if @collection.blank? + return RenderedCollection::EMPTY if @collection.blank? - if @options.key?(:spacer_template) - spacer = find_template(@options[:spacer_template], @locals.keys).render(view, @locals) + spacer = if @options.key?(:spacer_template) + spacer_template = find_template(@options[:spacer_template], @locals.keys) + build_rendered_template(spacer_template.render(view, @locals), nil, spacer_template) + else + RenderedTemplate::EMPTY_SPACER end collection_body = if template @@ -347,7 +342,7 @@ module ActionView else collection_without_template(view) end - collection_body.join(spacer).html_safe + build_rendered_collection(collection_body, spacer) end end @@ -369,7 +364,7 @@ module ActionView content = layout.render(view, locals) { content } if layout payload[:cache_hit] = view.view_renderer.cache_hits[template.virtual_path] - content + build_rendered_template(content, layout, template) end end @@ -460,7 +455,7 @@ module ActionView content = template.render(view, locals) content = layout.render(view, locals) { content } if layout partial_iteration.iterate! - content + build_rendered_template(content, layout, template) end end @@ -482,7 +477,7 @@ module ActionView template = (cache[path] ||= find_template(path, keys + [as, counter, iteration])) content = template.render(view, locals) partial_iteration.iterate! - content + build_rendered_template(content, nil, template) end end diff --git a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb b/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb index 9f1de5a397..401391290f 100644 --- a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb +++ b/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb @@ -40,7 +40,7 @@ module ActionView rendered_partials = @collection.empty? ? [] : yield index = 0 - fetch_or_cache_partial(cached_partials, order_by: keyed_collection.each_key) do + fetch_or_cache_partial(cached_partials, template, order_by: keyed_collection.each_key) do # This block is called once # for every cache miss while preserving order. rendered_partials[index].tap { index += 1 } @@ -81,11 +81,13 @@ module ActionView # # If the partial is not already cached it will also be # written back to the underlying cache store. - def fetch_or_cache_partial(cached_partials, order_by:) + def fetch_or_cache_partial(cached_partials, template, order_by:) order_by.map do |cache_key| - cached_partials.fetch(cache_key) do + if content = cached_partials[cache_key] + build_rendered_template(content, nil, template) + else yield.tap do |rendered_partial| - collection_cache.write(cache_key, rendered_partial) + collection_cache.write(cache_key, rendered_partial.body) end end end diff --git a/actionview/lib/action_view/renderer/renderer.rb b/actionview/lib/action_view/renderer/renderer.rb index 3f3a97529d..485eb1a5b4 100644 --- a/actionview/lib/action_view/renderer/renderer.rb +++ b/actionview/lib/action_view/renderer/renderer.rb @@ -19,10 +19,14 @@ module ActionView # Main render entry point shared by Action View and Action Controller. def render(context, options) + render_to_object(context, options).body + end + + def render_to_object(context, options) # :nodoc: if options.key?(:partial) - render_partial(context, options) + render_partial_to_object(context, options) else - render_template(context, options) + render_template_to_object(context, options) end end @@ -41,16 +45,24 @@ module ActionView # Direct access to template rendering. def render_template(context, options) #:nodoc: - TemplateRenderer.new(@lookup_context).render(context, options) + render_template_to_object(context, options).body end # Direct access to partial rendering. def render_partial(context, options, &block) #:nodoc: - PartialRenderer.new(@lookup_context).render(context, options, block) + render_partial_to_object(context, options, &block).body end def cache_hits # :nodoc: @cache_hits ||= {} end + + def render_template_to_object(context, options) #:nodoc: + TemplateRenderer.new(@lookup_context).render(context, options) + end + + def render_partial_to_object(context, options, &block) #:nodoc: + PartialRenderer.new(@lookup_context).render(context, options, block) + end end end diff --git a/actionview/lib/action_view/renderer/streaming_template_renderer.rb b/actionview/lib/action_view/renderer/streaming_template_renderer.rb index f414620923..279ef3c680 100644 --- a/actionview/lib/action_view/renderer/streaming_template_renderer.rb +++ b/actionview/lib/action_view/renderer/streaming_template_renderer.rb @@ -44,7 +44,7 @@ module ActionView # object that responds to each. This object is initialized with a block # that knows how to render the template. def render_template(view, template, layout_name = nil, locals = {}) #:nodoc: - return [super] unless layout_name && template.supports_streaming? + return [super.body] unless layout_name && template.supports_streaming? locals ||= {} layout = layout_name && find_layout(layout_name, locals.keys, [formats.first]) diff --git a/actionview/lib/action_view/renderer/template_renderer.rb b/actionview/lib/action_view/renderer/template_renderer.rb index c36baeffcd..7052fe0961 100644 --- a/actionview/lib/action_view/renderer/template_renderer.rb +++ b/actionview/lib/action_view/renderer/template_renderer.rb @@ -10,8 +10,6 @@ module ActionView prepend_formats(template.formats) - @lookup_context.rendered_format ||= (template.formats.first || formats.first) - render_template(context, template, options[:layout], options[:locals] || {}) end @@ -46,23 +44,24 @@ module ActionView # Renders the given template. A string representing the layout can be # supplied as well. def render_template(view, template, layout_name, locals) - render_with_layout(view, layout_name, locals) do |layout| + render_with_layout(view, layout_name, template, locals) do |layout| instrument(:template, identifier: template.identifier, layout: layout.try(:virtual_path)) do template.render(view, locals) { |*name| view._layout_for(*name) } end end end - def render_with_layout(view, path, locals) + def render_with_layout(view, path, template, locals) layout = path && find_layout(path, locals.keys, [formats.first]) content = yield(layout) - if layout + body = if layout view.view_flow.set(:layout, content) layout.render(view, locals) { |*name| view._layout_for(*name) } else content end + build_rendered_template(body, layout, template) end # This is the method which actually finds the layout using details in the lookup diff --git a/actionview/lib/action_view/rendering.rb b/actionview/lib/action_view/rendering.rb index b798e80b04..699ccb0ca8 100644 --- a/actionview/lib/action_view/rendering.rb +++ b/actionview/lib/action_view/rendering.rb @@ -109,10 +109,15 @@ module ActionView context = view_context context.assign assigns if assigns - lookup_context.rendered_format = nil if options[:formats] lookup_context.variants = variant if variant - context.view_renderer.render(context, options) + rendered_template = context.in_rendering_context(options) do |renderer| + renderer.render_to_object(context, options) + end + + lookup_context.rendered_format = rendered_template.format || lookup_context.formats.first + + rendered_template.body end # Assign the rendered format to look up context. From 3f84a814aa1d934fc0e577ff1d69f0e814b9d1c2 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 19 Feb 2019 14:57:29 -0800 Subject: [PATCH 5/5] Fix up style --- actionview/lib/action_view/renderer/abstract_renderer.rb | 4 ++-- actionview/lib/action_view/renderer/partial_renderer.rb | 8 ++++---- .../renderer/partial_renderer/collection_caching.rb | 2 +- actionview/lib/action_view/renderer/template_renderer.rb | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/actionview/lib/action_view/renderer/abstract_renderer.rb b/actionview/lib/action_view/renderer/abstract_renderer.rb index 2a51f2ef99..200dc3e10e 100644 --- a/actionview/lib/action_view/renderer/abstract_renderer.rb +++ b/actionview/lib/action_view/renderer/abstract_renderer.rb @@ -90,12 +90,12 @@ module ActionView @lookup_context.formats = formats | @lookup_context.formats end - def build_rendered_template(content, layout, template) + def build_rendered_template(content, template, layout = nil) RenderedTemplate.new content, layout, template end def build_rendered_collection(templates, spacer) - RenderedCollection.new(templates, spacer) + RenderedCollection.new templates, spacer end end end diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index f4319b8528..4ae6f635ae 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -330,7 +330,7 @@ module ActionView spacer = if @options.key?(:spacer_template) spacer_template = find_template(@options[:spacer_template], @locals.keys) - build_rendered_template(spacer_template.render(view, @locals), nil, spacer_template) + build_rendered_template(spacer_template.render(view, @locals), spacer_template) else RenderedTemplate::EMPTY_SPACER end @@ -364,7 +364,7 @@ module ActionView content = layout.render(view, locals) { content } if layout payload[:cache_hit] = view.view_renderer.cache_hits[template.virtual_path] - build_rendered_template(content, layout, template) + build_rendered_template(content, template, layout) end end @@ -455,7 +455,7 @@ module ActionView content = template.render(view, locals) content = layout.render(view, locals) { content } if layout partial_iteration.iterate! - build_rendered_template(content, layout, template) + build_rendered_template(content, template, layout) end end @@ -477,7 +477,7 @@ module ActionView template = (cache[path] ||= find_template(path, keys + [as, counter, iteration])) content = template.render(view, locals) partial_iteration.iterate! - build_rendered_template(content, nil, template) + build_rendered_template(content, template) end end diff --git a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb b/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb index 401391290f..ed59033e27 100644 --- a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb +++ b/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb @@ -84,7 +84,7 @@ module ActionView def fetch_or_cache_partial(cached_partials, template, order_by:) order_by.map do |cache_key| if content = cached_partials[cache_key] - build_rendered_template(content, nil, template) + build_rendered_template(content, template) else yield.tap do |rendered_partial| collection_cache.write(cache_key, rendered_partial.body) diff --git a/actionview/lib/action_view/renderer/template_renderer.rb b/actionview/lib/action_view/renderer/template_renderer.rb index 7052fe0961..c17d6182e8 100644 --- a/actionview/lib/action_view/renderer/template_renderer.rb +++ b/actionview/lib/action_view/renderer/template_renderer.rb @@ -61,7 +61,7 @@ module ActionView else content end - build_rendered_template(body, layout, template) + build_rendered_template(body, template, layout) end # This is the method which actually finds the layout using details in the lookup