Skip to content
Browse files

Digestor should just rely on the finder to know about the format and …

…the variant -- trying to pass it back in makes a mess of things (oh, and doesnt work)
  • Loading branch information...
1 parent 4bca347 commit 637bb726cac60aaa1f7e482836458aa73e17fbb7 @dhh dhh committed Mar 21, 2014
View
89 actionview/lib/action_view/digestor.rb
@@ -12,17 +12,13 @@ class << self
# 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(options_or_deprecated_name, *deprecated_args)
- options = _options_for_digest(options_or_deprecated_name, *deprecated_args)
+ def digest(options)
+ options.assert_valid_keys(:name, :finder, :dependencies, :partial)
- details_key = options[:finder].details_key.hash
- dependencies = Array.wrap(options[:dependencies])
- cache_key = ([options[:name], details_key, options[:format], options[:variant]].compact + dependencies).join('.')
+ cache_key = ([ options[:name], options[:finder].details_key.hash ].compact + Array.wrap(options[:dependencies])).join('.')
# this is a correctly done double-checked locking idiom
# (ThreadSafe::Cache's lookups have volatile semantics)
@@ -33,63 +29,41 @@ def digest(options_or_deprecated_name, *deprecated_args)
end
end
- def _options_for_digest(options_or_deprecated_name, *deprecated_args)
- if !options_or_deprecated_name.is_a?(Hash)
- ActiveSupport::Deprecation.warn \
- 'ActionView::Digestor.digest changed its method signature from ' \
- '#digest(name, format, finder, options) to #digest(options), ' \
- 'with options now including :name, :format, :finder, and ' \
- ':variant. Please update your code to pass a Hash argument. ' \
- 'Support for the old method signature will be removed in Rails 5.0.'
-
- _options_for_digest(deprecated_args[2] || {}).merge \
- name: options_or_deprecated_name,
- format: deprecated_args[0],
- finder: deprecated_args[1]
- else
- options_or_deprecated_name.assert_valid_keys(:name, :format, :variant, :finder, :dependencies, :partial)
- options_or_deprecated_name
- end
- end
-
private
+ 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.
+ pre_stored = @@cache.put_if_absent(cache_key, false).nil? # put_if_absent returns nil on insertion
+ PartialDigestor
+ else
+ Digestor
+ end
- 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.
- pre_stored = @@cache.put_if_absent(cache_key, false).nil? # put_if_absent returns nil on insertion
- PartialDigestor
- else
- Digestor
+ 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
end
-
- 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
- end
end
- attr_reader :name, :format, :variant, :path, :finder, :options
+ attr_reader :name, :finder, :options
- def initialize(options_or_deprecated_name, *deprecated_args)
- options = self.class._options_for_digest(options_or_deprecated_name, *deprecated_args)
- @options = options.except(:name, :format, :variant, :finder)
- @name, @format, @variant, @finder = options.values_at(:name, :format, :variant, :finder)
- @path = "#{@name}.#{format}".tap { |path| path << "+#{@variant}" if @variant }
+ def initialize(options)
+ @name, @finder = options.values_at(:name, :finder)
+ @options = options.except(:name, :finder)
end
def digest
Digest::MD5.hexdigest("#{source}-#{dependency_digest}").tap do |digest|
- logger.try :info, "Cache digest for #{path}: #{digest}"
+ logger.try :info, " Cache digest for #{template.inspect}: #{digest}"
end
rescue ActionView::MissingTemplate
- logger.try :error, "Couldn't find template for digesting: #{path}"
+ logger.try :error, " Couldn't find template for digesting: #{name}"
''
end
@@ -101,13 +75,12 @@ def dependencies
def nested_dependencies
dependencies.collect do |dependency|
- dependencies = PartialDigestor.new(name: dependency, format: format, variant: variant, finder: finder).nested_dependencies
+ dependencies = PartialDigestor.new(name: dependency, finder: finder).nested_dependencies
dependencies.any? ? { dependency => dependencies } : dependency
end
end
private
-
def logger
ActionView::Base.logger
end
@@ -121,11 +94,7 @@ def partial?
end
def template
- @template ||= begin
- finder.disable_cache do
- finder.find(logical_name, [], partial?, [], formats: [format], variants: [variant])
- end
- end
+ @template ||= finder.disable_cache { finder.find(logical_name, [], partial?) }
end
def source
@@ -134,7 +103,7 @@ def source
def dependency_digest
template_digests = dependencies.collect do |template_name|
- Digestor.digest(name: template_name, format: format, variant: variant, finder: finder, partial: true)
+ Digestor.digest(name: template_name, finder: finder, partial: true)
end
(template_digests + injected_dependencies).join("-")
View
16 actionview/lib/action_view/helpers/cache_helper.rb
@@ -165,20 +165,10 @@ def cache_fragment_name(name = {}, options = nil)
def fragment_name_with_digest(name) #:nodoc:
if @virtual_path
- variant = request.variant.is_a?(Array) ? request.variant.first : request.variant
+ names = Array(name.is_a?(Hash) ? controller.url_for(name).split("://").last : name)
+ digest = Digestor.digest name: @virtual_path, finder: lookup_context, dependencies: view_cache_dependencies
- 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]
+ [ *names, digest ]
else
name
end
View
17 actionview/test/template/digestor_test.rb
@@ -100,13 +100,13 @@ def test_directory_depth_dependency
end
def test_logging_of_missing_template
- assert_logged "Couldn't find template for digesting: messages/something_missing.html" do
+ assert_logged "Couldn't find template for digesting: messages/something_missing" do
digest("messages/show")
end
end
def test_logging_of_missing_template_ending_with_number
- assert_logged "Couldn't find template for digesting: messages/something_missing_1.html" do
+ assert_logged "Couldn't find template for digesting: messages/something_missing_1" do
digest("messages/show")
end
end
@@ -207,7 +207,7 @@ def test_old_style_hash_in_render_invocation
end
def test_variants
- assert_digest_difference("messages/new", false, variant: :iphone) do
+ assert_digest_difference("messages/new", false, variants: [:iphone]) do
change_template("messages/new", :iphone)
change_template("messages/_header", :iphone)
end
@@ -261,10 +261,6 @@ def test_digest_cache_cleanup_with_recursion_and_template_caching_off
ActionView::Resolver.caching = resolver_before
end
- def test_arguments_deprecation
- assert_deprecated(/will be removed/) { ActionView::Digestor.digest('messages/show', :html, finder) }
- assert_deprecated(/will be removed/) { ActionView::Digestor.new('messages/show', :html, finder) }
- end
private
def assert_logged(message)
@@ -294,10 +290,11 @@ def assert_digest_difference(template_name, persistent = false, options = {})
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))
+ finder.formats = [:html]
+ finder.variants = options.delete(:variants) || []
+
+ ActionView::Digestor.digest({ name: template_name, finder: finder }.merge(options))
end
def finder

0 comments on commit 637bb72

Please sign in to comment.
Something went wrong with that request. Please try again.