Skip to content

Loading…

Ensure LookupContext in Digestor selects correct variant #14329

Merged
merged 10 commits into from

7 participants

@pch
pch commented

Related to: #14242, #14243 & #14293

Variants passed to LookupContext#find() seem to be ignored, so I used a setter instead: finder.variants = [ variant ] which seems to fix the problem.

I've also added some more test cases for variants. Hopefully this time passing tests will mean it actually works.

/cc @strzalek @dhh @jeremy

@lukaszx0 lukaszx0 added this to the 4.1.0 milestone
@lukaszx0 lukaszx0 self-assigned this
@lukaszx0 lukaszx0 commented on an outdated diff
actionpack/test/controller/caching_test.rb
@@ -242,6 +251,18 @@ 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
+ get :formatted_fragment_cached_with_variant, :format => "html", :variant => :phone
@lukaszx0 Ruby on Rails member
lukaszx0 added a note

I would remove setting variant inside action (line 168, above) and passing it through params hash here and just set it above get by simply request.variant = :phone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lukaszx0 lukaszx0 commented on an outdated diff
actionview/lib/action_view/helpers/cache_helper.rb
((6 lines not shown))
[
*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)
+ Digestor.digest(name: @virtual_path, format: formats.last.to_sym, variant: variant, finder: lookup_context, dependencies: view_cache_dependencies)
@lukaszx0 Ruby on Rails member
lukaszx0 added a note

As we're touching this code, how about making this method more readable?

def fragment_name_with_digest(name) #:nodoc:
  if @virtual_path
    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
end

I just think it's way more understandable what's going on there this way. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pch pch Ensure LookupContext in Digestor selects correct variant
Related to: #14242 #14243 14293

Variants passed to LookupContext#find() seem to be ignored, so
I've used the setter instead: `finder.variants = [ variant ]`.

I've also added some more test cases for variants. Hopefully this
time passing tests will mean it actually works.
025c691
@thedarkone thedarkone commented on an outdated diff
actionview/lib/action_view/digestor.rb
@@ -126,7 +126,10 @@ def partial?
end
def template
- @template ||= finder.find(logical_name, [], partial?, formats: [ format ], variants: [ variant ])
+ @template ||= begin
+ finder.variants = [ variant ]

I'm not sure about this, but it seems weird that finder's variants attr is changed just like this. Isn't this finder instance used elsewhere (ie. for template look up)? Shouldn't a previous variants value be restored after the finder.find call?

Why is variants: [ variant ] options param being ignored by finder.find, maybe it is better to fix the find method?

@pch
pch added a note

Yes, you're probably right. It looks that "formats" are also being ignored here, so technically it shouldn't have worked correctly before variants were introduced... LookupContext is still a bit of a mystery to me, so maybe @strzalek will have a better idea what's going on here.

@josevalim Ruby on Rails member

Why is variants: [ variant ] options param being ignored by finder.find, maybe it is better to fix the find method?

That actually seems to be the best way to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dhh
Ruby on Rails member

@strzalek, have you had a chance to review this? Are we done with this or is there still more to come?

@lukaszx0 lukaszx0 added the actionview label
@lukaszx0
Ruby on Rails member

I've just pushed more commits fixing more things :wink:

The root issue and confusion which caused problems and errors that David had was that we were wrongly calling LookupContext#find method in Digestor#template. We were passing formats: [format] hash as keys argument not options (see method signature: https://github.com/rails/rails/blob/master/actionview/lib/action_view/lookup_context.rb#L123).

Right now, we're setting both format and variant explicitly on LookupContext before calling find. Additionally this need to be wrapped in disable_cache block - now, when it works properly we need to do this. Besides that I've added variants to Template and polished few things.

I believe the mystery is finally solved and this issue is fixed.

/cc @rafaelfranca @josevalim @jeremy @tenderlove @dhh

@josevalim josevalim commented on an outdated diff
actionview/lib/action_view/digestor.rb
@@ -126,7 +121,14 @@ def partial?
end
def template
- @template ||= finder.find(logical_name, [], partial?, formats: [ format ], variants: [ variant ])
+ @template ||= begin
+ finder.formats = [format]
@josevalim Ruby on Rails member

Why are changing the finder state and not reverting it back? Maybe we should dup it?

@lukaszx0 Ruby on Rails member

I don't think it will have any implications as we're not passing this object anywhere, just getting value returned by #find but for sake of being safe, we can dup it. Should we?

@josevalim Ruby on Rails member

Well, we are not passing the object anywhere but we are receiving it from somewhere and that operation is mutating it, isn't? So unless someone from upstream created a new finder or duped it (which may actually be the best solution), we need to do it here.

finder is an instance of ActionView::LookupContext (a thingy that is instantiated on per request basis, its purpose is to do template lookups) its format and variants are setup for the current request (based on request headers etc). It is not ok to just change its format and variants as it might mess up template lookup for the current request.

Also don't like the .dup workaround as it depends on hoping that relevant attrs (format and variants) are implemented as a simple i-vars.

This code should be a part of ActionView::LookupContext, i.e. it should have a method like this:

def with_formats_and_variants(new_formats, new_variants)
  old_formats = formats
  self.formats = new_formats
  old_variants = variants
  self.variants = new_variants
  yield
ensure
  self.formats = old_formats
  self.variants = old_variants
end

which Digestor would then use:

finder.with_formats_and_variants([format], [variant]) do
  finder.disable_cache { finder.find(logical_name, [], partial?) }
end
@lukaszx0 Ruby on Rails member

@josevalim What do you think?

@josevalim Ruby on Rails member

Unless the finder is reused, we should be fine with just passing the format and variants explicitly to find. That's the main point. If the finder is used just for the digestor and multiple times in the digestor maybe duping is better. If it is shared, I would go with passing options to find. If for some reason it doesn't work, I would go with @thedarkone's approach.

There is a slight tension between how template rendering uses finder: its "registered_details" (such as formats, variants, locale etc.) are pretty much constant and unchanging through out the whole request, therefore the #find method was mainly meant to accept template name, view prefixes and whether its a partial template.

But looking at the code, I can't figure out why passing :formats and :variants doesn't work, it looks to be supported.

PS: duping the finder seems to work, but purely by accident: formats and variants are stored in @details i-var, which is a Hash, so after duping, the new finder still uses the same @details Hash as the old one, but details setters (finder.formats=) dup the @details Hash on writes, so it actually works.

@lukaszx0 Ruby on Rails member

But looking at the code, I can't figure out why passing :formats and :variants doesn't work, it looks to be supported.

I must admit that the way find works is confusing for me as well. I've added todo for myself to take care of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@josevalim
Ruby on Rails member

Besides that I've added variants to Template and polished few things.

Is there any particular reason for adding variants to template?

@rafaelfranca rafaelfranca commented on the diff
actionview/lib/action_view/template/resolver.rb
@@ -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)
@rafaelfranca Ruby on Rails member

This signature change will break custom resolvers in people application. I'm not sure if this method is part of the public API but @josevalim is the best person to decide this here.

@lukaszx0 Ruby on Rails member

Oh yes, I forgot to ask about it. I can make it backwards compatible if needed.

@rafaelfranca Ruby on Rails member

@josevalim how about this one?

@josevalim Ruby on Rails member

I guess it is fine. A couple people may be using it but it is a small change and it is required if we want to store variants in templates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca
Ruby on Rails member

Looks good @strzalek. Excelent work. I'll check with @josevalim the API change

@rafaelfranca
Ruby on Rails member

Ha! @josevalim is already review the patch :smile:

@lukaszx0
Ruby on Rails member

Is there any particular reason for adding variants to template?

@josevalim I know we discussed that when variants were proposed and decided no to have them there however I think we should keep that information. It's not needed now but just for keeping it consistent, I think it's worth to have it. Generally I expect to have access to variant everywhere where format is in use, and especially Template class which describes template files. I expect Template object of foo/bar.html+phone.erb file to have variant attribute the same way it has format. What do you think?

@josevalim
Ruby on Rails member

@strzalek I thought so, I just wanted to know for sure if the code was being added for consistency or a particular use case, thanks!

@josevalim
Ruby on Rails member

Two final questions:

  1. Why can't we simply pass the format as options? The finder.find API supports it:

    https://github.com/pch/rails/blob/digestor-lookup-fix/actionview/lib/action_view/lookup_context.rb#L123-L125

    And it takes care of guaranteeing things are not being mutated.

  2. Why disable_cache is necessary? What happens if we don't disable it? Disabling the cache seems like a not so good idea.

@rafaelfranca
Ruby on Rails member

2 . Why disable_cache is necessary? What happens if we don't disable it? Disabling the cache seems like a not so good idea.

The TemplateDigestor needs to read the source of the template. A cached template doesn't have source. Disabling the cache on this case if fine since we run the digestor only one time in the whole life cycle of the application.

Before this change we were already not using the cache values because the formats were being passed to the finders as locals and the cached version didn't have these locals.

@rafaelfranca
Ruby on Rails member

1 . Why can't we simply pass the format as options? The finder.find API supports it:

It is possible now. I think @strzalek didn't passed as options because we had the cache problem that I explained in the last comment, so he thought it was because the wrong call. Now that we know his problem were because of the cache we can pass format as options.

Good catch

@lukaszx0
Ruby on Rails member
  1. Why can't we simply pass the format as options? The finder.find API supports it

We'd still need to mutate object to set variant

@lukaszx0
Ruby on Rails member

It is possible now. I think @strzalek didn't passed as options because we had the cache problem that I explained in the last comment, so he thought it was because the wrong call. Now that we know his problem were because of the cache we can pass format as options.

Good catch

I had "AHA!" moment as well, but setting both formats and variants as options won't work:

      def template
        @template ||= begin
          finder.disable_cache do
            finder.find(logical_name, [], partial?, [], formats: [format], variants: [variant])
          end
        end
      end
 1) Error:
FunctionalFragmentCachingTest#test_fragment_caching_with_variant:
ActionView::Template::Error: undefined method `split' for nil:NilClass
    /Users/lukaszstrzalkowski/projects/rails/actionview/lib/action_view/dependency_tracker.rb:100:in `render_dependencies'
@jeremy jeremy commented on an outdated diff
actionview/lib/action_view/template/resolver.rb
@@ -188,15 +189,17 @@ def query(path, details, formats)
!sanitizer[File.dirname(filename)].include?(filename)
}
- template_paths.map { |template|
- handler, format = extract_handler_and_format(template, formats)
- contents = File.binread template
+ template_paths.map do |template|
+ handler, format, variant = extract_handler_and_format_and_variant(template, formats)
+ contents = File.binread(template)
@jeremy Ruby on Rails member
jeremy added a note

No need for style/syntax changes, distracts from the purpose of the code change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy commented on an outdated diff
actionview/lib/action_view/template/resolver.rb
((13 lines not shown))
:virtual_path => path.virtual,
:format => format,
- :updated_at => mtime(template))
- }
+ :variant => variant,
+ :updated_at => mtime(template)
+ })
@jeremy Ruby on Rails member
jeremy added a note

Same as above, adding explicit { when implicit were fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy commented on an outdated diff
actionview/lib/action_view/template/resolver.rb
@@ -311,7 +314,7 @@ def build_query(path, details)
"{#{details[ext].compact.uniq.map { |e| "#{prefix}#{e}," }.join}}"
end.join
- query + exts
+ query.concat(exts)
@jeremy Ruby on Rails member
jeremy added a note

Why change this to mutate query? Seems unrelated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy commented on an outdated diff
actionview/lib/action_view/testing/resolvers.rb
@@ -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)
- templates << Template.new(source, _path, handler,
- :virtual_path => path.virtual, :format => format, :updated_at => updated_at)
+ handler, format, variant = extract_handler_and_format_and_variant(_path, formats)
+ templates << Template.new(source, _path, handler, {
+ :virtual_path => path.virtual,
+ :format => format,
+ :variant => variant,
+ :updated_at => updated_at
+ })
@jeremy Ruby on Rails member
jeremy added a note

Keep the original implicit {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy commented on the diff
actionview/test/template/digestor_test.rb
((14 lines not shown))
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 = {})
@jeremy Ruby on Rails member
jeremy added a note

logical_name -> name Why? Loses meaning.

@lukaszx0 Ruby on Rails member

Inconsistencies in arguments number/naming made debugging much more harder. The FixtureFinder is mocked LookupContext and its find method looks like this https://github.com/rails/rails/blob/master/actionview/lib/action_view/lookup_context.rb#L123 - I've just copied it to match both. I would prefer to keep both in sync.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lukaszx0
Ruby on Rails member

I've addressed @jeremy's style comments and added with_formats_and_variants as per @thedarkone and @josevalim suggestion.

Is there anything left? Are we good to go? Please take a look.

/cc @rafaelfranca

@josevalim
Ruby on Rails member

Thanks @rafaelfranca and @strzalek for the explanations. It looks good to go!

@lukaszx0
Ruby on Rails member

:shipit:

@rafaelfranca
Ruby on Rails member

On it. Thanks @strzalek for the work. All everyone for the review.

@lukaszx0 lukaszx0 was unassigned by rafaelfranca
@rafaelfranca rafaelfranca self-assigned this
@rafaelfranca rafaelfranca merged commit 2c2326e into rails:master

1 check passed

Details default The Travis CI build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 9, 2014
  1. @pch

    Ensure LookupContext in Digestor selects correct variant

    pch committed
    Related to: #14242 #14243 14293
    
    Variants passed to LookupContext#find() seem to be ignored, so
    I've used the setter instead: `finder.variants = [ variant ]`.
    
    I've also added some more test cases for variants. Hopefully this
    time passing tests will mean it actually works.
Commits on Mar 13, 2014
  1. @lukaszx0

    Don't pass variant in params, it's ignored

    lukaszx0 committed with lukaszx0
    We're setting variant above, in request object directly
  2. @lukaszx0
Commits on Mar 14, 2014
  1. @lukaszx0

    Add variants to Template class

    lukaszx0 committed
  2. @lukaszx0
  3. @lukaszx0
  4. @lukaszx0
  5. @lukaszx0

    Set format in finder

    lukaszx0 committed
  6. @lukaszx0
  7. @lukaszx0
View
21 actionpack/test/controller/caching_test.rb
@@ -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)
View
3 ...ck/test/fixtures/functional_caching/formatted_fragment_cached_with_variant.html+phone.erb
@@ -0,0 +1,3 @@
+<body>
+<%= cache do %><p>PHONE</p><% end %>
+</body>
View
21 actionview/lib/action_view/digestor.rb
@@ -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
View
18 actionview/lib/action_view/helpers/cache_helper.rb
@@ -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
View
8 actionview/lib/action_view/lookup_context.rb
@@ -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
View
3 actionview/lib/action_view/template.rb
@@ -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
View
17 actionview/lib/action_view/template/resolver.rb
@@ -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)
@rafaelfranca Ruby on Rails member

This signature change will break custom resolvers in people application. I'm not sure if this method is part of the public API but @josevalim is the best person to decide this here.

@lukaszx0 Ruby on Rails member

Oh yes, I forgot to ask about it. I can make it backwards compatible if needed.

@rafaelfranca Ruby on Rails member

@josevalim how about this one?

@josevalim Ruby on Rails member

I guess it is fine. A couple people may be using it but it is a small change and it is required if we want to store variants in templates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
View
12 actionview/lib/action_view/testing/resolvers.rb
@@ -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
View
1 actionview/test/fixtures/test/hello_world.html+phone.erb
@@ -0,0 +1 @@
+Hello phone!
View
1 actionview/test/fixtures/test/hello_world.text+phone.erb
@@ -0,0 +1 @@
+Hello texty phone!
View
31 actionview/test/template/digestor_test.rb
@@ -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 = {})
@jeremy Ruby on Rails member
jeremy added a note

logical_name -> name Why? Loses meaning.

@lukaszx0 Ruby on Rails member

Inconsistencies in arguments number/naming made debugging much more harder. The FixtureFinder is mocked LookupContext and its find method looks like this https://github.com/rails/rails/blob/master/actionview/lib/action_view/lookup_context.rb#L123 - I've just copied it to match both. I would prefer to keep both in sync.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ 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
View
27 actionview/test/template/lookup_context_test.rb
@@ -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
Something went wrong with that request. Please try again.