Merge pull request #14243 from pch/digestor-variants

Variants in ActionView::Digestor

actionpack/test/controller/caching_test.rb

@@ -243,8 +243,8 @@ CACHED
   end
 
   private
-    def template_digest(name, format)
-      ActionView::Digestor.digest(name, format, @controller.lookup_context)
+    def template_digest(name, format, variant = nil)
+      ActionView::Digestor.digest(name: name, format: format, variant: variant, finder: @controller.lookup_context)
     end
 end
 

actionview/CHANGELOG.md

@@ -5,4 +5,13 @@
 
     *Sergey Prikhodko*
 
+*   Take variants into account when calculating template digests in ActionView::Digestor.
+
+    The arguments to ActionView::Digestor#digest are now being passed as a hash
+    to support variants and allow more flexibility in the future. The support for
+    regular (required) arguments is deprecated and will be removed in Rails 5.0 or later.
+
+    *Piotr Chmolowski, Łukasz Strzałkowski*
+
+
 Please check [4-1-stable](https://github.com/rails/rails/blob/4-1-stable/actionview/CHANGELOG.md) for previous changes.

actionview/lib/action_view/digestor.rb

@@ -9,23 +9,56 @@ module ActionView
     @@digest_monitor = Monitor.new
 
     class << self
-      def digest(name, format, finder, options = {})
+      # Supported options:
+      #
+      # * <tt>name</tt>   - Template name
+      # * <tt>format</tt>  - Template format
+      # * <tt>variant</tt>  - Variant of +format+ (optional)
+      # * <tt>finder</tt>  - An instance of ActionView::LookupContext
+      # * <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)
+
+        name = options[:name]
+        format = options[:format]
+        variant = options[:variant]
+        finder = options[:finder]
+
         details_key = finder.details_key.hash
         dependencies = Array.wrap(options[:dependencies])
-        cache_key = ([name, details_key, format] + dependencies).join('.')
+        cache_key = ([name, details_key, format, variant].compact + dependencies).join('.')
 
         # this is a correctly done double-checked locking idiom
         # (ThreadSafe::Cache's lookups have volatile semantics)
         @@cache[cache_key] || @@digest_monitor.synchronize do
           @@cache.fetch(cache_key) do # re-check under lock
-            compute_and_store_digest(cache_key, name, format, finder, options)
+            compute_and_store_digest(cache_key, options)
           end
         end
       end
 
+      def _setup_options(*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")
+
+          {
+            name:   args.first,
+            format: args.second,
+            finder: args.third,
+          }.merge(args.fourth || {})
+        else
+          options = args.first
+          options.assert_valid_keys(:name, :format, :variant, :finder, :dependencies, :partial)
+
+          options
+        end
+      end
+
       private
-      def compute_and_store_digest(cache_key, name, format, finder, options) # called under @@digest_monitor lock
-        klass = if options[:partial] || name.include?("/_")
+
+      def compute_and_store_digest(cache_key, options) # called under @@digest_monitor lock
+        klass = if options[:partial] || options[:name].include?("/_")
           # Prevent re-entry or else recursive templates will blow the stack.
           # There is no need to worry about other threads seeing the +false+ value,
           # as they will then have to wait for this thread to let go of the @@digest_monitor lock.
@@ -35,20 +68,25 @@ module ActionView
           Digestor
         end
 
-        digest = klass.new(name, format, finder, options).digest
+        digest = klass.new(options).digest
         # Store the actual digest if config.cache_template_loading is true
         @@cache[cache_key] = stored_digest = digest if ActionView::Resolver.caching?
         digest
       ensure
         # something went wrong or ActionView::Resolver.caching? is false, make sure not to corrupt the @@cache
-        @@cache.delete_pair(cache_key, false) if pre_stored && !stored_digest 
+        @@cache.delete_pair(cache_key, false) if pre_stored && !stored_digest
       end
     end
 
-    attr_reader :name, :format, :finder, :options
+    attr_reader :name, :format, :variant, :finder, :options
 
-    def initialize(name, format, finder, options={})
-      @name, @format, @finder, @options = name, format, finder, options
+    def initialize(*args)
+      @options = self.class._setup_options(*args)
+
+      @name = @options.delete(:name)
+      @format = @options.delete(:format)
+      @variant = @options.delete(:variant)
+      @finder = @options.delete(:finder)
     end
 
     def digest
@@ -68,7 +106,7 @@ module ActionView
 
     def nested_dependencies
       dependencies.collect do |dependency|
-        dependencies = PartialDigestor.new(dependency, format, finder).nested_dependencies
+        dependencies = PartialDigestor.new(name: dependency, format: format, finder: finder).nested_dependencies
         dependencies.any? ? { dependency => dependencies } : dependency
       end
     end
@@ -88,7 +126,7 @@ module ActionView
       end
 
       def template
-        @template ||= finder.find(logical_name, [], partial?, formats: [ format ])
+        @template ||= finder.find(logical_name, [], partial?, formats: [ format ], variants: [ variant ])
       end
 
       def source
@@ -97,7 +135,7 @@ module ActionView
 
       def dependency_digest
         template_digests = dependencies.collect do |template_name|
-          Digestor.digest(template_name, format, finder, partial: true)
+          Digestor.digest(name: template_name, format: format, finder: finder, partial: true)
         end
 
         (template_digests + injected_dependencies).join("-")

actionview/lib/action_view/helpers/cache_helper.rb

@@ -167,7 +167,7 @@ module ActionView
         if @virtual_path
           [
             *Array(name.is_a?(Hash) ? controller.url_for(name).split("://").last : name),
-            Digestor.digest(@virtual_path, formats.last.to_sym, lookup_context, dependencies: view_cache_dependencies)
+            Digestor.digest(name: @virtual_path, format: formats.last.to_sym, variant: request.variant, finder: lookup_context, dependencies: view_cache_dependencies)
           ]
         else
           name

actionview/test/fixtures/digestor/messages/new.html+iphone.erb

@@ -0,0 +1,15 @@
+<%# Template Dependency: messages/message %>
+
+<%= render "header" %>
+<%= render "comments/comments" %>
+
+<%= render "messages/actions/move" %>
+
+<%= render @message.history.events %>
+
+<%# render "something_missing"   %>
+<%# render "something_missing_1" %>
+
+<%
+  # Template Dependency: messages/form
+%>

actionview/test/template/digestor_test.rb

@@ -26,7 +26,11 @@ class FixtureFinder
   end
 
   def find(logical_name, keys, partial, options)
-    FixtureTemplate.new("digestor/#{partial ? logical_name.gsub(%r|/([^/]+)$|, '/_\1') : logical_name}.#{options[:formats].first}.erb")
+    partial_name = partial ? logical_name.gsub(%r|/([^/]+)$|, '/_\1') : logical_name
+    format = options[:formats].first.to_s
+    format += "+#{options[:variants].first}" if options[:variants].any?
+
+    FixtureTemplate.new("digestor/#{partial_name}.#{format}.erb")
   end
 end
 
@@ -194,6 +198,13 @@ class TemplateDigestorTest < ActionView::TestCase
     end
   end
 
+  def test_variants
+    assert_digest_difference("messages/new", false, variant: :iphone) do
+      change_template("messages/new",     :iphone)
+      change_template("messages/_header", :iphone)
+    end
+  end
+
   def test_dependencies_via_options_results_in_different_digest
     digest_plain        = digest("comments/_comment")
     digest_fridge       = digest("comments/_comment", dependencies: ["fridge"])
@@ -242,6 +253,11 @@ class TemplateDigestorTest < ActionView::TestCase
     ActionView::Resolver.caching = resolver_before
   end
 
+  def test_arguments_deprecation
+    assert_deprecated(/should be provided as a hash/) { ActionView::Digestor.digest('messages/show', :html, finder) }
+    assert_deprecated(/should be provided as a hash/) { ActionView::Digestor.new('messages/show', :html, finder) }
+  end
+
   private
     def assert_logged(message)
       old_logger = ActionView::Base.logger
@@ -258,26 +274,29 @@ class TemplateDigestorTest < ActionView::TestCase
       end
     end
 
-    def assert_digest_difference(template_name, persistent = false)
-      previous_digest = digest(template_name)
+    def assert_digest_difference(template_name, persistent = false, options = {})
+      previous_digest = digest(template_name, options)
       ActionView::Digestor.cache.clear unless persistent
 
       yield
 
-      assert previous_digest != digest(template_name), "digest didn't change"
+      assert previous_digest != digest(template_name, options), "digest didn't change"
       ActionView::Digestor.cache.clear
     end
 
-    def digest(template_name, options={})
-      ActionView::Digestor.digest(template_name, :html, finder, options)
+    def digest(template_name, options = {})
+      options = options.dup
+      ActionView::Digestor.digest({ name: template_name, format: :html, finder: finder }.merge(options))
     end
 
     def finder
       @finder ||= FixtureFinder.new
     end
 
-    def change_template(template_name)
-      File.open("digestor/#{template_name}.html.erb", "w") do |f|
+    def change_template(template_name, variant = nil)
+      variant = "+#{variant}" if variant.present?
+
+      File.open("digestor/#{template_name}.html#{variant}.erb", "w") do |f|
         f.write "\nTHIS WAS CHANGED!"
       end
     end