Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

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

Ensure LookupContext in Digestor selects correct variant
  • Loading branch information...
commit 274d5e45e00f6bddab8fecceccad3ba6c1c66232 2 parents b4c9649 + 2c2326e
Rafael Mendonça França rafaelfranca authored
21 actionpack/test/controller/caching_test.rb
View
@@ -164,6 +164,13 @@ def formatted_fragment_cached
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 @@ def test_xml_formatted_fragment_caching
@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"
+ assert_response :success
+ expected_body = "<body>\n<p>PHONE</p>\n</body>\n"
+
+ assert_equal expected_body, @response.body
+
+ assert_equal "<p>PHONE</p>",
+ @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)
3  actionpack/test/fixtures/functional_caching/formatted_fragment_cached_with_variant.html+phone.erb
View
@@ -0,0 +1,3 @@
+<body>
+<%= cache do %><p>PHONE</p><% end %>
+</body>
21 actionview/lib/action_view/digestor.rb
View
@@ -18,16 +18,11 @@ class << self
# * <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 @@ def digest(*args)
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 @@ def compute_and_store_digest(cache_key, options) # called under @@digest_monitor
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 @@ def partial?
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
18 actionview/lib/action_view/helpers/cache_helper.rb
View
@@ -165,10 +165,20 @@ def cache_fragment_name(name = {}, options = nil)
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
8 actionview/lib/action_view/lookup_context.rb
View
@@ -246,5 +246,13 @@ def with_layout_format
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
3  actionview/lib/action_view/template.rb
View
@@ -97,7 +97,7 @@ class Template
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 @@ def initialize(source, identifier, handler, details)
@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
17 actionview/lib/action_view/template/resolver.rb
View
@@ -154,7 +154,8 @@ def decorate(templates, path_info, details, locals) #:nodoc:
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 @@ def query(path, details, formats)
}
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 @@ def mtime(p)
# 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 @@ def extract_handler_and_format(path, default_formats)
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
12 actionview/lib/action_view/testing/resolvers.rb
View
@@ -30,9 +30,13 @@ def query(path, exts, formats)
@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 @@ def query(path, exts, formats)
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
1  actionview/test/fixtures/test/hello_world.html+phone.erb
View
@@ -0,0 +1 @@
+Hello phone!
1  actionview/test/fixtures/test/hello_world.text+phone.erb
View
@@ -0,0 +1 @@
+Hello texty phone!
31 actionview/test/template/digestor_test.rb
View
@@ -15,23 +15,39 @@ def initialize(template_path)
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 @@ 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))
end
27 actionview/test/template/lookup_context_test.rb
View
@@ -93,6 +93,20 @@ def teardown
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 @@ def teardown
@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
Please sign in to comment.
Something went wrong with that request. Please try again.