From 025c691536b22cc3f0ba802f6c303dd6f955c1c3 Mon Sep 17 00:00:00 2001 From: Piotr Chmolowski Date: Sat, 8 Mar 2014 23:09:54 +0100 Subject: [PATCH 01/10] Ensure LookupContext in Digestor selects correct variant Related to: #14242 #14243 14293 Variants passed to LookupContext#find() seem to be ignored, so I've used the setter instead: `finder.variants = [ variant ]`. I've also added some more test cases for variants. Hopefully this time passing tests will mean it actually works. --- actionpack/test/controller/caching_test.rb | 21 +++++++++++++++++++ ...ragment_cached_with_variant.html+phone.erb | 3 +++ actionview/lib/action_view/digestor.rb | 5 ++++- .../lib/action_view/helpers/cache_helper.rb | 18 ++++++++++++---- .../fixtures/test/hello_world.html+phone.erb | 1 + .../fixtures/test/hello_world.text+phone.erb | 1 + actionview/test/template/digestor_test.rb | 8 ++++--- .../test/template/lookup_context_test.rb | 14 +++++++++++++ 8 files changed, 63 insertions(+), 8 deletions(-) create mode 100644 actionpack/test/fixtures/functional_caching/formatted_fragment_cached_with_variant.html+phone.erb create mode 100644 actionview/test/fixtures/test/hello_world.html+phone.erb create mode 100644 actionview/test/fixtures/test/hello_world.text+phone.erb diff --git a/actionpack/test/controller/caching_test.rb b/actionpack/test/controller/caching_test.rb index 591d0eccc3..0ba8045cb5 100644 --- a/actionpack/test/controller/caching_test.rb +++ b/actionpack/test/controller/caching_test.rb @@ -164,6 +164,13 @@ class FunctionalCachingController < CachingController end end + def formatted_fragment_cached_with_variant + respond_to do |format| + format.html.phone + format.html + end + end + def fragment_cached_without_digest end end @@ -242,6 +249,20 @@ CACHED @store.read("views/test.host/functional_caching/formatted_fragment_cached/#{template_digest("functional_caching/formatted_fragment_cached", "xml")}") end + + def test_fragment_caching_with_variant + @request.variant = :phone + + get :formatted_fragment_cached_with_variant, :format => "html", :variant => :phone + assert_response :success + expected_body = "\n

PHONE

\n\n" + + assert_equal expected_body, @response.body + + assert_equal "

PHONE

", + @store.read("views/test.host/functional_caching/formatted_fragment_cached_with_variant/#{template_digest("functional_caching/formatted_fragment_cached_with_variant", :html, :phone)}") + end + private def template_digest(name, format, variant = nil) ActionView::Digestor.digest(name: name, format: format, variant: variant, finder: @controller.lookup_context) diff --git a/actionpack/test/fixtures/functional_caching/formatted_fragment_cached_with_variant.html+phone.erb b/actionpack/test/fixtures/functional_caching/formatted_fragment_cached_with_variant.html+phone.erb new file mode 100644 index 0000000000..e523b74ae3 --- /dev/null +++ b/actionpack/test/fixtures/functional_caching/formatted_fragment_cached_with_variant.html+phone.erb @@ -0,0 +1,3 @@ + +<%= cache do %>

PHONE

<% end %> + diff --git a/actionview/lib/action_view/digestor.rb b/actionview/lib/action_view/digestor.rb index da43fef2e3..4e3773abe7 100644 --- a/actionview/lib/action_view/digestor.rb +++ b/actionview/lib/action_view/digestor.rb @@ -126,7 +126,10 @@ module ActionView end def template - @template ||= finder.find(logical_name, [], partial?, formats: [ format ], variants: [ variant ]) + @template ||= begin + finder.variants = [ variant ] + finder.find(logical_name, [], partial?, formats: [ format ]) + end end def source diff --git a/actionview/lib/action_view/helpers/cache_helper.rb b/actionview/lib/action_view/helpers/cache_helper.rb index 30e4e5e329..3177d18c4d 100644 --- a/actionview/lib/action_view/helpers/cache_helper.rb +++ b/actionview/lib/action_view/helpers/cache_helper.rb @@ -165,10 +165,20 @@ module ActionView def fragment_name_with_digest(name) #:nodoc: if @virtual_path - [ - *Array(name.is_a?(Hash) ? controller.url_for(name).split("://").last : name), - Digestor.digest(name: @virtual_path, format: formats.last.to_sym, variant: request.variant, finder: lookup_context, dependencies: view_cache_dependencies) - ] + variant = request.variant.is_a?(Array) ? request.variant.first : request.variant + + options = { + name: @virtual_path, + format: formats.last.to_sym, + variant: variant, + finder: lookup_context, + dependencies: view_cache_dependencies + } + + names = Array(name.is_a?(Hash) ? controller.url_for(name).split("://").last : name) + digest = Digestor.digest(options) + + [*names, digest] else name end diff --git a/actionview/test/fixtures/test/hello_world.html+phone.erb b/actionview/test/fixtures/test/hello_world.html+phone.erb new file mode 100644 index 0000000000..b4f236f878 --- /dev/null +++ b/actionview/test/fixtures/test/hello_world.html+phone.erb @@ -0,0 +1 @@ +Hello phone! \ No newline at end of file diff --git a/actionview/test/fixtures/test/hello_world.text+phone.erb b/actionview/test/fixtures/test/hello_world.text+phone.erb new file mode 100644 index 0000000000..611e2ee442 --- /dev/null +++ b/actionview/test/fixtures/test/hello_world.text+phone.erb @@ -0,0 +1 @@ +Hello texty phone! \ No newline at end of file diff --git a/actionview/test/template/digestor_test.rb b/actionview/test/template/digestor_test.rb index 2406a64310..0580758dab 100644 --- a/actionview/test/template/digestor_test.rb +++ b/actionview/test/template/digestor_test.rb @@ -15,10 +15,12 @@ end class FixtureFinder FIXTURES_DIR = "#{File.dirname(__FILE__)}/../fixtures/digestor" - attr_reader :details + attr_reader :details + attr_accessor :variants def initialize - @details = {} + @details = {} + @variants = [] end def details_key @@ -28,7 +30,7 @@ class FixtureFinder def find(logical_name, keys, partial, options) partial_name = partial ? logical_name.gsub(%r|/([^/]+)$|, '/_\1') : logical_name format = options[:formats].first.to_s - format += "+#{options[:variants].first}" if options[:variants].any? + format += "+#{@variants.first}" if @variants.any? FixtureTemplate.new("digestor/#{partial_name}.#{format}.erb") end diff --git a/actionview/test/template/lookup_context_test.rb b/actionview/test/template/lookup_context_test.rb index ce9485e146..4f7823045e 100644 --- a/actionview/test/template/lookup_context_test.rb +++ b/actionview/test/template/lookup_context_test.rb @@ -93,6 +93,20 @@ class LookupContextTest < ActiveSupport::TestCase assert_equal "Hey verden", template.source end + test "find templates with given variants" do + @lookup_context.formats = [:html] + @lookup_context.variants = [:phone] + + template = @lookup_context.find("hello_world", %w(test)) + assert_equal "Hello phone!", template.source + + @lookup_context.variants = [:phone] + @lookup_context.formats = [:text] + + template = @lookup_context.find("hello_world", %w(test)) + assert_equal "Hello texty phone!", template.source + end + test "found templates respects given formats if one cannot be found from template or handler" do ActionView::Template::Handlers::Builder.expects(:default_format).returns(nil) @lookup_context.formats = [:text] From f72feae9bafbadfd4da4e383bb302afc33c7d3e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Strza=C5=82kowski?= Date: Tue, 11 Mar 2014 16:30:00 +0100 Subject: [PATCH 02/10] Don't pass variant in params, it's ignored We're setting variant above, in request object directly --- actionpack/test/controller/caching_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionpack/test/controller/caching_test.rb b/actionpack/test/controller/caching_test.rb index 0ba8045cb5..9923b90bae 100644 --- a/actionpack/test/controller/caching_test.rb +++ b/actionpack/test/controller/caching_test.rb @@ -253,7 +253,7 @@ CACHED def test_fragment_caching_with_variant @request.variant = :phone - get :formatted_fragment_cached_with_variant, :format => "html", :variant => :phone + get :formatted_fragment_cached_with_variant, :format => "html" assert_response :success expected_body = "\n

PHONE

\n\n" From 0ca6836a5a5dc249f82c98d34e17205a559157cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Strza=C5=82kowski?= Date: Tue, 11 Mar 2014 16:30:58 +0100 Subject: [PATCH 03/10] Don't create addition vars, use options[] directly --- actionview/lib/action_view/digestor.rb | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/actionview/lib/action_view/digestor.rb b/actionview/lib/action_view/digestor.rb index 4e3773abe7..df6e7bba60 100644 --- a/actionview/lib/action_view/digestor.rb +++ b/actionview/lib/action_view/digestor.rb @@ -20,14 +20,9 @@ module ActionView def digest(*args) options = _setup_options(*args) - name = options[:name] - format = options[:format] - variant = options[:variant] - finder = options[:finder] - - details_key = finder.details_key.hash + details_key = options[:finder].details_key.hash dependencies = Array.wrap(options[:dependencies]) - cache_key = ([name, details_key, format, variant].compact + dependencies).join('.') + cache_key = ([options[:name], details_key, options[:format], options[:variant]].compact + dependencies).join('.') # this is a correctly done double-checked locking idiom # (ThreadSafe::Cache's lookups have volatile semantics) From c63b18de1865182e027a97ea4186717a71792b81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Strza=C5=82kowski?= Date: Wed, 12 Mar 2014 15:42:21 +0100 Subject: [PATCH 04/10] Add variants to Template class --- actionview/lib/action_view/template.rb | 3 ++- actionview/lib/action_view/template/resolver.rb | 17 ++++++++++------- actionview/lib/action_view/testing/resolvers.rb | 12 ++++++++---- 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/actionview/lib/action_view/template.rb b/actionview/lib/action_view/template.rb index 961a969b6e..9d39d02a37 100644 --- a/actionview/lib/action_view/template.rb +++ b/actionview/lib/action_view/template.rb @@ -97,7 +97,7 @@ module ActionView extend Template::Handlers - attr_accessor :locals, :formats, :virtual_path + attr_accessor :locals, :formats, :variants, :virtual_path attr_reader :source, :identifier, :handler, :original_encoding, :updated_at @@ -123,6 +123,7 @@ module ActionView @virtual_path = details[:virtual_path] @updated_at = details[:updated_at] || Time.now @formats = Array(format).map { |f| f.respond_to?(:ref) ? f.ref : f } + @variants = [details[:variant]] @compile_mutex = Mutex.new end diff --git a/actionview/lib/action_view/template/resolver.rb b/actionview/lib/action_view/template/resolver.rb index 3a3b74cdd5..403824bd8e 100644 --- a/actionview/lib/action_view/template/resolver.rb +++ b/actionview/lib/action_view/template/resolver.rb @@ -154,7 +154,8 @@ module ActionView cached = nil templates.each do |t| t.locals = locals - t.formats = details[:formats] || [:html] if t.formats.empty? + t.formats = details[:formats] || [:html] if t.formats.empty? + t.variants = details[:variants] || [] if t.variants.empty? t.virtual_path ||= (cached ||= build_path(*path_info)) end end @@ -189,13 +190,15 @@ module ActionView } template_paths.map { |template| - handler, format = extract_handler_and_format(template, formats) - contents = File.binread template + handler, format, variant = extract_handler_and_format_and_variant(template, formats) + contents = File.binread(template) Template.new(contents, File.expand_path(template), handler, :virtual_path => path.virtual, :format => format, - :updated_at => mtime(template)) + :variant => variant, + :updated_at => mtime(template) + ) } end @@ -228,7 +231,7 @@ module ActionView # Extract handler and formats from path. If a format cannot be a found neither # from the path, or the handler, we should return the array of formats given # to the resolver. - def extract_handler_and_format(path, default_formats) + def extract_handler_and_format_and_variant(path, default_formats) pieces = File.basename(path).split(".") pieces.shift @@ -240,10 +243,10 @@ module ActionView end handler = Template.handler_for_extension(extension) - format = pieces.last && pieces.last.split(EXTENSIONS[:variants], 2).first # remove variant from format + format, variant = pieces.last.split(EXTENSIONS[:variants], 2) if pieces.last format &&= Template::Types[format] - [handler, format] + [handler, format, variant] end end diff --git a/actionview/lib/action_view/testing/resolvers.rb b/actionview/lib/action_view/testing/resolvers.rb index af53ad3b25..dfb7d463b4 100644 --- a/actionview/lib/action_view/testing/resolvers.rb +++ b/actionview/lib/action_view/testing/resolvers.rb @@ -30,9 +30,13 @@ module ActionView #:nodoc: @hash.each do |_path, array| source, updated_at = array next unless _path =~ query - handler, format = extract_handler_and_format(_path, formats) + handler, format, variant = extract_handler_and_format_and_variant(_path, formats) templates << Template.new(source, _path, handler, - :virtual_path => path.virtual, :format => format, :updated_at => updated_at) + :virtual_path => path.virtual, + :format => format, + :variant => variant, + :updated_at => updated_at + ) end templates.sort_by {|t| -t.identifier.match(/^#{query}$/).captures.reject(&:blank?).size } @@ -41,8 +45,8 @@ module ActionView #:nodoc: class NullResolver < PathResolver def query(path, exts, formats) - handler, format = extract_handler_and_format(path, formats) - [ActionView::Template.new("Template generated by Null Resolver", path, handler, :virtual_path => path, :format => format)] + handler, format, variant = extract_handler_and_format_and_variant(path, formats) + [ActionView::Template.new("Template generated by Null Resolver", path, handler, :virtual_path => path, :format => format, :variant => variant)] end end From 48a6baea5ebb5539ab8ae9696cca6e95aac08a38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Strza=C5=82kowski?= Date: Wed, 12 Mar 2014 15:43:09 +0100 Subject: [PATCH 05/10] Don't pass hash as keys to #find method --- actionview/lib/action_view/digestor.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionview/lib/action_view/digestor.rb b/actionview/lib/action_view/digestor.rb index df6e7bba60..6ef962383d 100644 --- a/actionview/lib/action_view/digestor.rb +++ b/actionview/lib/action_view/digestor.rb @@ -123,7 +123,7 @@ module ActionView def template @template ||= begin finder.variants = [ variant ] - finder.find(logical_name, [], partial?, formats: [ format ]) + finder.find(logical_name, [], partial?) end end From 4725d588960213fd617d3d17c936aa206a161cfb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Strza=C5=82kowski?= Date: Thu, 13 Mar 2014 14:46:43 +0100 Subject: [PATCH 06/10] Disable LookupContext's cache when looking for template --- actionview/lib/action_view/digestor.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/actionview/lib/action_view/digestor.rb b/actionview/lib/action_view/digestor.rb index 6ef962383d..666291e1c1 100644 --- a/actionview/lib/action_view/digestor.rb +++ b/actionview/lib/action_view/digestor.rb @@ -122,8 +122,12 @@ module ActionView def template @template ||= begin - finder.variants = [ variant ] - finder.find(logical_name, [], partial?) + finder.formats = [format] + finder.variants = [variant] + + finder.disable_cache do + finder.find(logical_name, [], partial?) + end end end From 3b9daf0ff9f7981d375e7617b1ec46e9fc988aaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Strza=C5=82kowski?= Date: Thu, 13 Mar 2014 16:27:24 +0100 Subject: [PATCH 07/10] Rename _setup_options to _options_for_digest --- actionview/lib/action_view/digestor.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/actionview/lib/action_view/digestor.rb b/actionview/lib/action_view/digestor.rb index 666291e1c1..dad3cea73a 100644 --- a/actionview/lib/action_view/digestor.rb +++ b/actionview/lib/action_view/digestor.rb @@ -18,7 +18,7 @@ module ActionView # * dependencies - An array of dependent views # * partial - Specifies whether the template is a partial def digest(*args) - options = _setup_options(*args) + options = _options_for_digest(*args) details_key = options[:finder].details_key.hash dependencies = Array.wrap(options[:dependencies]) @@ -33,7 +33,7 @@ module ActionView end end - def _setup_options(*args) + def _options_for_digest(*args) unless args.first.is_a?(Hash) ActiveSupport::Deprecation.warn("Arguments to ActionView::Digestor should be provided as a hash. The support for regular arguments will be removed in Rails 5.0 or later") @@ -76,7 +76,7 @@ module ActionView attr_reader :name, :format, :variant, :finder, :options def initialize(*args) - @options = self.class._setup_options(*args) + @options = self.class._options_for_digest(*args) @name = @options.delete(:name) @format = @options.delete(:format) From 03b8922ee4ba6051ae18917b5904f8664e715695 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Strza=C5=82kowski?= Date: Thu, 13 Mar 2014 16:55:54 +0100 Subject: [PATCH 08/10] Set format in finder --- actionview/test/template/digestor_test.rb | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/actionview/test/template/digestor_test.rb b/actionview/test/template/digestor_test.rb index 0580758dab..2d4c434f9c 100644 --- a/actionview/test/template/digestor_test.rb +++ b/actionview/test/template/digestor_test.rb @@ -16,10 +16,12 @@ class FixtureFinder FIXTURES_DIR = "#{File.dirname(__FILE__)}/../fixtures/digestor" attr_reader :details + attr_accessor :formats attr_accessor :variants def initialize @details = {} + @formats = [] @variants = [] end @@ -27,9 +29,9 @@ class FixtureFinder details.hash end - def find(logical_name, keys, partial, options) - partial_name = partial ? logical_name.gsub(%r|/([^/]+)$|, '/_\1') : logical_name - format = options[:formats].first.to_s + def find(name, prefixes = [], partial = false, keys = [], options = {}) + partial_name = partial ? name.gsub(%r|/([^/]+)$|, '/_\1') : name + format = @formats.first.to_s format += "+#{@variants.first}" if @variants.any? FixtureTemplate.new("digestor/#{partial_name}.#{format}.erb") @@ -288,6 +290,9 @@ class TemplateDigestorTest < ActionView::TestCase def digest(template_name, options = {}) options = options.dup + finder.formats = [:html] + finder.variants = [options[:variant]] if options[:variant].present? + ActionView::Digestor.digest({ name: template_name, format: :html, finder: finder }.merge(options)) end From 9f677bf043eb359a91d346bb5a1e6ee81cef6665 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Strza=C5=82kowski?= Date: Thu, 13 Mar 2014 16:56:29 +0100 Subject: [PATCH 09/10] Add mocked disable_cache for FixtureFinder --- actionview/test/template/digestor_test.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/actionview/test/template/digestor_test.rb b/actionview/test/template/digestor_test.rb index 2d4c434f9c..1c47952f54 100644 --- a/actionview/test/template/digestor_test.rb +++ b/actionview/test/template/digestor_test.rb @@ -36,6 +36,10 @@ class FixtureFinder FixtureTemplate.new("digestor/#{partial_name}.#{format}.erb") end + + def disable_cache(&block) + yield + end end class TemplateDigestorTest < ActionView::TestCase From 2c2326e6eab4d192e361871787335ffa711af597 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Strza=C5=82kowski?= Date: Fri, 14 Mar 2014 13:40:46 +0100 Subject: [PATCH 10/10] Introduce #with_formats_and_variants to prevent problems with mutating finder object --- actionview/lib/action_view/digestor.rb | 7 ++----- actionview/lib/action_view/lookup_context.rb | 8 ++++++++ actionview/test/template/digestor_test.rb | 8 ++++++++ actionview/test/template/lookup_context_test.rb | 13 +++++++++++++ 4 files changed, 31 insertions(+), 5 deletions(-) diff --git a/actionview/lib/action_view/digestor.rb b/actionview/lib/action_view/digestor.rb index dad3cea73a..abbfdc786e 100644 --- a/actionview/lib/action_view/digestor.rb +++ b/actionview/lib/action_view/digestor.rb @@ -122,11 +122,8 @@ module ActionView def template @template ||= begin - finder.formats = [format] - finder.variants = [variant] - - finder.disable_cache do - finder.find(logical_name, [], partial?) + finder.with_formats_and_variants([format], [variant]) do + finder.disable_cache { finder.find(logical_name, [], partial?) } end end end diff --git a/actionview/lib/action_view/lookup_context.rb b/actionview/lib/action_view/lookup_context.rb index 76c9890776..d7f116c10c 100644 --- a/actionview/lib/action_view/lookup_context.rb +++ b/actionview/lib/action_view/lookup_context.rb @@ -246,5 +246,13 @@ module ActionView end end end + + def with_formats_and_variants(new_formats, new_variants) + old_formats, old_variants = formats, variants + self.formats, self.variants = new_formats, new_variants + yield + ensure + self.formats, self.variants = old_formats, old_variants + end end end diff --git a/actionview/test/template/digestor_test.rb b/actionview/test/template/digestor_test.rb index 1c47952f54..72d1f43f12 100644 --- a/actionview/test/template/digestor_test.rb +++ b/actionview/test/template/digestor_test.rb @@ -40,6 +40,14 @@ class FixtureFinder def disable_cache(&block) yield end + + def with_formats_and_variants(new_formats, new_variants) + old_formats, old_variants = formats, variants + self.formats, self.variants = new_formats, new_variants + yield + ensure + self.formats, self.variants = old_formats, old_variants + end end class TemplateDigestorTest < ActionView::TestCase diff --git a/actionview/test/template/lookup_context_test.rb b/actionview/test/template/lookup_context_test.rb index 4f7823045e..b11f252110 100644 --- a/actionview/test/template/lookup_context_test.rb +++ b/actionview/test/template/lookup_context_test.rb @@ -205,6 +205,19 @@ class LookupContextTest < ActiveSupport::TestCase @lookup_context.prefixes = ["foo"] assert_equal ["foo"], @lookup_context.prefixes end + + test "with_formats_and_variants preserves original values after execution" do + @lookup_context.formats = [:html] + @lookup_context.variants = [:phone] + + @lookup_context.with_formats_and_variants([:xml], [:tablet]) do + assert_equal [:xml], @lookup_context.formats + assert_equal [:tablet], @lookup_context.variants + end + + assert_equal [:html], @lookup_context.formats + assert_equal [:phone], @lookup_context.variants + end end class LookupContextWithFalseCaching < ActiveSupport::TestCase