mirror of
https://github.com/rails/rails.git
synced 2022-11-09 12:12:34 -05:00
Merge pull request #14329 from pch/digestor-lookup-fix
Ensure LookupContext in Digestor selects correct variant
This commit is contained in:
commit
274d5e45e0
12 changed files with 130 additions and 33 deletions
|
@ -164,6 +164,13 @@ class FunctionalCachingController < CachingController
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def formatted_fragment_cached_with_variant
|
||||||
|
respond_to do |format|
|
||||||
|
format.html.phone
|
||||||
|
format.html
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
def fragment_cached_without_digest
|
def fragment_cached_without_digest
|
||||||
end
|
end
|
||||||
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")}")
|
@store.read("views/test.host/functional_caching/formatted_fragment_cached/#{template_digest("functional_caching/formatted_fragment_cached", "xml")}")
|
||||||
end
|
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
|
private
|
||||||
def template_digest(name, format, variant = nil)
|
def template_digest(name, format, variant = nil)
|
||||||
ActionView::Digestor.digest(name: name, format: format, variant: variant, finder: @controller.lookup_context)
|
ActionView::Digestor.digest(name: name, format: format, variant: variant, finder: @controller.lookup_context)
|
||||||
|
|
|
@ -0,0 +1,3 @@
|
||||||
|
<body>
|
||||||
|
<%= cache do %><p>PHONE</p><% end %>
|
||||||
|
</body>
|
|
@ -18,16 +18,11 @@ module ActionView
|
||||||
# * <tt>dependencies</tt> - An array of dependent views
|
# * <tt>dependencies</tt> - An array of dependent views
|
||||||
# * <tt>partial</tt> - Specifies whether the template is a partial
|
# * <tt>partial</tt> - Specifies whether the template is a partial
|
||||||
def digest(*args)
|
def digest(*args)
|
||||||
options = _setup_options(*args)
|
options = _options_for_digest(*args)
|
||||||
|
|
||||||
name = options[:name]
|
details_key = options[:finder].details_key.hash
|
||||||
format = options[:format]
|
|
||||||
variant = options[:variant]
|
|
||||||
finder = options[:finder]
|
|
||||||
|
|
||||||
details_key = finder.details_key.hash
|
|
||||||
dependencies = Array.wrap(options[:dependencies])
|
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
|
# this is a correctly done double-checked locking idiom
|
||||||
# (ThreadSafe::Cache's lookups have volatile semantics)
|
# (ThreadSafe::Cache's lookups have volatile semantics)
|
||||||
|
@ -38,7 +33,7 @@ module ActionView
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
def _setup_options(*args)
|
def _options_for_digest(*args)
|
||||||
unless args.first.is_a?(Hash)
|
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")
|
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
|
attr_reader :name, :format, :variant, :finder, :options
|
||||||
|
|
||||||
def initialize(*args)
|
def initialize(*args)
|
||||||
@options = self.class._setup_options(*args)
|
@options = self.class._options_for_digest(*args)
|
||||||
|
|
||||||
@name = @options.delete(:name)
|
@name = @options.delete(:name)
|
||||||
@format = @options.delete(:format)
|
@format = @options.delete(:format)
|
||||||
|
@ -126,7 +121,11 @@ module ActionView
|
||||||
end
|
end
|
||||||
|
|
||||||
def template
|
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
|
end
|
||||||
|
|
||||||
def source
|
def source
|
||||||
|
|
|
@ -165,10 +165,20 @@ module ActionView
|
||||||
|
|
||||||
def fragment_name_with_digest(name) #:nodoc:
|
def fragment_name_with_digest(name) #:nodoc:
|
||||||
if @virtual_path
|
if @virtual_path
|
||||||
[
|
variant = request.variant.is_a?(Array) ? request.variant.first : request.variant
|
||||||
*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)
|
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
|
else
|
||||||
name
|
name
|
||||||
end
|
end
|
||||||
|
|
|
@ -246,5 +246,13 @@ module ActionView
|
||||||
end
|
end
|
||||||
end
|
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
|
||||||
end
|
end
|
||||||
|
|
|
@ -97,7 +97,7 @@ module ActionView
|
||||||
|
|
||||||
extend Template::Handlers
|
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
|
attr_reader :source, :identifier, :handler, :original_encoding, :updated_at
|
||||||
|
|
||||||
|
@ -123,6 +123,7 @@ module ActionView
|
||||||
@virtual_path = details[:virtual_path]
|
@virtual_path = details[:virtual_path]
|
||||||
@updated_at = details[:updated_at] || Time.now
|
@updated_at = details[:updated_at] || Time.now
|
||||||
@formats = Array(format).map { |f| f.respond_to?(:ref) ? f.ref : f }
|
@formats = Array(format).map { |f| f.respond_to?(:ref) ? f.ref : f }
|
||||||
|
@variants = [details[:variant]]
|
||||||
@compile_mutex = Mutex.new
|
@compile_mutex = Mutex.new
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -154,7 +154,8 @@ module ActionView
|
||||||
cached = nil
|
cached = nil
|
||||||
templates.each do |t|
|
templates.each do |t|
|
||||||
t.locals = locals
|
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))
|
t.virtual_path ||= (cached ||= build_path(*path_info))
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
@ -189,13 +190,15 @@ module ActionView
|
||||||
}
|
}
|
||||||
|
|
||||||
template_paths.map { |template|
|
template_paths.map { |template|
|
||||||
handler, format = extract_handler_and_format(template, formats)
|
handler, format, variant = extract_handler_and_format_and_variant(template, formats)
|
||||||
contents = File.binread template
|
contents = File.binread(template)
|
||||||
|
|
||||||
Template.new(contents, File.expand_path(template), handler,
|
Template.new(contents, File.expand_path(template), handler,
|
||||||
:virtual_path => path.virtual,
|
:virtual_path => path.virtual,
|
||||||
:format => format,
|
:format => format,
|
||||||
:updated_at => mtime(template))
|
:variant => variant,
|
||||||
|
:updated_at => mtime(template)
|
||||||
|
)
|
||||||
}
|
}
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -228,7 +231,7 @@ module ActionView
|
||||||
# Extract handler and formats from path. If a format cannot be a found neither
|
# 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
|
# from the path, or the handler, we should return the array of formats given
|
||||||
# to the resolver.
|
# 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 = File.basename(path).split(".")
|
||||||
pieces.shift
|
pieces.shift
|
||||||
|
|
||||||
|
@ -240,10 +243,10 @@ module ActionView
|
||||||
end
|
end
|
||||||
|
|
||||||
handler = Template.handler_for_extension(extension)
|
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]
|
format &&= Template::Types[format]
|
||||||
|
|
||||||
[handler, format]
|
[handler, format, variant]
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -30,9 +30,13 @@ module ActionView #:nodoc:
|
||||||
@hash.each do |_path, array|
|
@hash.each do |_path, array|
|
||||||
source, updated_at = array
|
source, updated_at = array
|
||||||
next unless _path =~ query
|
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,
|
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
|
end
|
||||||
|
|
||||||
templates.sort_by {|t| -t.identifier.match(/^#{query}$/).captures.reject(&:blank?).size }
|
templates.sort_by {|t| -t.identifier.match(/^#{query}$/).captures.reject(&:blank?).size }
|
||||||
|
@ -41,8 +45,8 @@ module ActionView #:nodoc:
|
||||||
|
|
||||||
class NullResolver < PathResolver
|
class NullResolver < PathResolver
|
||||||
def query(path, exts, formats)
|
def query(path, exts, formats)
|
||||||
handler, format = extract_handler_and_format(path, formats)
|
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)]
|
[ActionView::Template.new("Template generated by Null Resolver", path, handler, :virtual_path => path, :format => format, :variant => variant)]
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
1
actionview/test/fixtures/test/hello_world.html+phone.erb
vendored
Normal file
1
actionview/test/fixtures/test/hello_world.html+phone.erb
vendored
Normal file
|
@ -0,0 +1 @@
|
||||||
|
Hello phone!
|
1
actionview/test/fixtures/test/hello_world.text+phone.erb
vendored
Normal file
1
actionview/test/fixtures/test/hello_world.text+phone.erb
vendored
Normal file
|
@ -0,0 +1 @@
|
||||||
|
Hello texty phone!
|
|
@ -15,23 +15,39 @@ end
|
||||||
class FixtureFinder
|
class FixtureFinder
|
||||||
FIXTURES_DIR = "#{File.dirname(__FILE__)}/../fixtures/digestor"
|
FIXTURES_DIR = "#{File.dirname(__FILE__)}/../fixtures/digestor"
|
||||||
|
|
||||||
attr_reader :details
|
attr_reader :details
|
||||||
|
attr_accessor :formats
|
||||||
|
attr_accessor :variants
|
||||||
|
|
||||||
def initialize
|
def initialize
|
||||||
@details = {}
|
@details = {}
|
||||||
|
@formats = []
|
||||||
|
@variants = []
|
||||||
end
|
end
|
||||||
|
|
||||||
def details_key
|
def details_key
|
||||||
details.hash
|
details.hash
|
||||||
end
|
end
|
||||||
|
|
||||||
def find(logical_name, keys, partial, options)
|
def find(name, prefixes = [], partial = false, keys = [], options = {})
|
||||||
partial_name = partial ? logical_name.gsub(%r|/([^/]+)$|, '/_\1') : logical_name
|
partial_name = partial ? name.gsub(%r|/([^/]+)$|, '/_\1') : name
|
||||||
format = options[:formats].first.to_s
|
format = @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")
|
FixtureTemplate.new("digestor/#{partial_name}.#{format}.erb")
|
||||||
end
|
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
|
end
|
||||||
|
|
||||||
class TemplateDigestorTest < ActionView::TestCase
|
class TemplateDigestorTest < ActionView::TestCase
|
||||||
|
@ -286,6 +302,9 @@ class TemplateDigestorTest < ActionView::TestCase
|
||||||
|
|
||||||
def digest(template_name, options = {})
|
def digest(template_name, options = {})
|
||||||
options = options.dup
|
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))
|
ActionView::Digestor.digest({ name: template_name, format: :html, finder: finder }.merge(options))
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -93,6 +93,20 @@ class LookupContextTest < ActiveSupport::TestCase
|
||||||
assert_equal "Hey verden", template.source
|
assert_equal "Hey verden", template.source
|
||||||
end
|
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
|
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)
|
ActionView::Template::Handlers::Builder.expects(:default_format).returns(nil)
|
||||||
@lookup_context.formats = [:text]
|
@lookup_context.formats = [:text]
|
||||||
|
@ -191,6 +205,19 @@ class LookupContextTest < ActiveSupport::TestCase
|
||||||
@lookup_context.prefixes = ["foo"]
|
@lookup_context.prefixes = ["foo"]
|
||||||
assert_equal ["foo"], @lookup_context.prefixes
|
assert_equal ["foo"], @lookup_context.prefixes
|
||||||
end
|
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
|
end
|
||||||
|
|
||||||
class LookupContextWithFalseCaching < ActiveSupport::TestCase
|
class LookupContextWithFalseCaching < ActiveSupport::TestCase
|
||||||
|
|
Loading…
Reference in a new issue