Merge pull request #14329 from pch/digestor-lookup-fix

Ensure LookupContext in Digestor selects correct variant

10 files changed, 106 insertions, 33 deletions

diff --git a/actionview/lib/action_view/digestor.rb b/actionview/lib/action_view/digestor.rb
index da43fef2e3..abbfdc786e 100644
--- a/actionview/lib/action_view/digestor.rb
+++ b/actionview/lib/action_view/digestor.rb
@@ -18,16 +18,11 @@ module ActionView
       # * <tt>dependencies</tt>  - An array of dependent views
       # * <tt>partial</tt>  - Specifies whether the template is a partial
       def digest(*args)
-        options = _setup_options(*args)
+        options = _options_for_digest(*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)
@@ -38,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")
 
@@ -81,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)
@@ -126,7 +121,11 @@ module ActionView
       end
 
       def template
-        @template ||= finder.find(logical_name, [], partial?, formats: [ format ], variants: [ variant ])
+        @template ||= begin
+          finder.with_formats_and_variants([format], [variant]) do
+            finder.disable_cache { finder.find(logical_name, [], partial?) }
+          end
+        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/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/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
 
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..72d1f43f12 100644
--- a/actionview/test/template/digestor_test.rb
+++ b/actionview/test/template/digestor_test.rb
@@ -15,23 +15,39 @@ end
 class FixtureFinder
   FIXTURES_DIR = "#{File.dirname(__FILE__)}/../fixtures/digestor"
 
-  attr_reader :details
+  attr_reader   :details
+  attr_accessor :formats
+  attr_accessor :variants
 
   def initialize
-    @details = {}
+    @details  = {}
+    @formats  = []
+    @variants = []
   end
 
   def details_key
     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
-    format += "+#{options[:variants].first}" if options[:variants].any?
+  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")
   end
+
+  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
@@ -286,6 +302,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
 
diff --git a/actionview/test/template/lookup_context_test.rb b/actionview/test/template/lookup_context_test.rb
index ce9485e146..b11f252110 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]
@@ -191,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