From f3fc5c4b5f36db37edc8ab553a35b06b48226c0a Mon Sep 17 00:00:00 2001 From: Yehuda Katz + Carl Lerche Date: Wed, 2 Sep 2009 15:00:22 -0700 Subject: Refactor ActionView::Resolver --- .gitignore | 2 + actionmailer/lib/action_mailer/base.rb | 2 +- actionpack/lib/action_controller/legacy/layout.rb | 2 +- actionpack/lib/action_view.rb | 1 + actionpack/lib/action_view/template/resolver.rb | 172 ++++++++++++--------- actionpack/test/lib/fixture_template.rb | 66 ++------ actionpack/test/new_base/render_file_test.rb | 2 +- .../test/template/compiled_templates_test.rb | 3 + 8 files changed, 116 insertions(+), 134 deletions(-) diff --git a/.gitignore b/.gitignore index 28303ce3e3..7a61c37718 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ +.DS_Store debug.log doc/rdoc activemodel/doc @@ -13,6 +14,7 @@ actionpack/pkg activemodel/test/fixtures/fixture_database.sqlite3 actionmailer/pkg activesupport/pkg +actionpack/test/tmp activesupport/test/fixtures/isolation_test railties/pkg railties/test/500.html diff --git a/actionmailer/lib/action_mailer/base.rb b/actionmailer/lib/action_mailer/base.rb index 5ecefe7c09..065c6c8ba1 100644 --- a/actionmailer/lib/action_mailer/base.rb +++ b/actionmailer/lib/action_mailer/base.rb @@ -489,7 +489,7 @@ module ActionMailer #:nodoc: # "the_template_file.text.html.erb", etc.). Only do this if parts # have not already been specified manually. # if @parts.empty? - template_root.find_all_by_parts(@template, {}, template_path).each do |template| + template_root.find_all(@template, {}, template_path).each do |template| @parts << Part.new( :content_type => template.mime_type ? template.mime_type.to_s : "text/plain", :disposition => "inline", diff --git a/actionpack/lib/action_controller/legacy/layout.rb b/actionpack/lib/action_controller/legacy/layout.rb index 43aea0eba2..db02bf1066 100644 --- a/actionpack/lib/action_controller/legacy/layout.rb +++ b/actionpack/lib/action_controller/legacy/layout.rb @@ -200,7 +200,7 @@ module ActionController #:nodoc: end def layout_list #:nodoc: - Array(view_paths).sum([]) { |path| Dir["#{path.to_str}/layouts/**/*"] } + Array(view_paths).sum([]) { |path| Dir["#{path}/layouts/**/*"] } end memoize :layout_list diff --git a/actionpack/lib/action_view.rb b/actionpack/lib/action_view.rb index 70176a0ea4..d90afb1913 100644 --- a/actionpack/lib/action_view.rb +++ b/actionpack/lib/action_view.rb @@ -40,6 +40,7 @@ module ActionView autoload :MissingTemplate, 'action_view/base' autoload :Partials, 'action_view/render/partials' autoload :Resolver, 'action_view/template/resolver' + autoload :PathResolver, 'action_view/template/resolver' autoload :PathSet, 'action_view/paths' autoload :Rendering, 'action_view/render/rendering' autoload :Renderable, 'action_view/template/renderable' diff --git a/actionpack/lib/action_view/template/resolver.rb b/actionpack/lib/action_view/template/resolver.rb index 4442db22f6..410562efcc 100644 --- a/actionpack/lib/action_view/template/resolver.rb +++ b/actionpack/lib/action_view/template/resolver.rb @@ -1,9 +1,28 @@ require "pathname" +require "active_support/core_ext/class" require "action_view/template/template" module ActionView # Abstract superclass class Resolver + + class_inheritable_accessor(:registered_details) + self.registered_details = {} + + def self.register_detail(name, options = {}) + registered_details[name] = lambda do |val| + val ||= yield + val |= [nil] unless options[:allow_nil] == false + val + end + end + + register_detail(:locale) { [I18n.locale] } + register_detail(:formats) { Mime::SET.symbols } + register_detail(:handlers, :allow_nil => false) do + TemplateHandlers.extensions + end + def initialize(options = {}) @cache = options[:cache] @cached = {} @@ -11,15 +30,18 @@ module ActionView # Normalizes the arguments and passes it on to find_template def find(*args) - find_all_by_parts(*args).first + find_all(*args).first end - - def find_all_by_parts(name, details = {}, prefix = nil, partial = nil) - details[:locales] = [I18n.locale] - name = name.to_s.gsub(handler_matcher, '').split("/") - find_templates(name.pop, details, [prefix, *name].compact.join("/"), partial) + + def find_all(name, details = {}, prefix = nil, partial = nil) + details = normalize_details(details) + name, prefix = normalize_name(name, prefix) + + cached([name, details, prefix, partial]) do + find_templates(name, details, prefix, partial) + end end - + private # This is what child classes implement. No defaults are needed @@ -28,29 +50,24 @@ module ActionView def find_templates(name, details, prefix, partial) raise NotImplementedError end - - def valid_handlers - @valid_handlers ||= TemplateHandlers.extensions - end - def handler_matcher - @handler_matcher ||= begin - e = valid_handlers.join('|') - /\.(?:#{e})$/ + def normalize_details(details) + details = details.dup + registered_details.each do |k, v| + details[k] = v.call(details[k]) end + details end - def handler_glob - @handler_glob ||= begin - e = TemplateHandlers.extensions.map{|h| ".#{h}"}.join(",") - "{#{e}}" - end - end - - def formats_glob - @formats_glob ||= begin - '{' + Mime::SET.symbols.map { |l| ".#{l}," }.join + '}' - end + # Support legacy foo.erb names even though we now ignore .erb + # as well as incorrectly putting part of the path in the template + # name instead of the prefix. + def normalize_name(name, prefix) + handlers = TemplateHandlers.extensions.join('|') + name = name.to_s.gsub(/\.(?:#{handlers})$/, '') + + parts = name.split('/') + return parts.pop, [prefix, *parts].compact.join("/") end def cached(key) @@ -60,67 +77,49 @@ module ActionView end end - class FileSystemResolver < Resolver + class PathResolver < Resolver - def self.cached_glob - @@cached_glob ||= {} - end - - def initialize(path, options = {}) - raise ArgumentError, "path already is a Resolver class" if path.is_a?(Resolver) - super(options) - @path = Pathname.new(path).expand_path - end + EXTENSION_ORDER = [:locale, :formats, :handlers] def to_s @path.to_s end alias to_path to_s - def find_templates(name, details, prefix, partial, root = "#{@path}/") - if glob = details_to_glob(name, details, prefix, partial, root) - cached(glob) do - Dir[glob].map do |path| - next if File.directory?(path) - source = File.read(path) - identifier = Pathname.new(path).expand_path.to_s - - Template.new(source, identifier, *path_to_details(path)) - end.compact - end - end + def find_templates(name, details, prefix, partial) + path = build_path(name, details, prefix, partial) + query(path, EXTENSION_ORDER.map { |ext| details[ext] }) end - + private - # :api: plugin - def details_to_glob(name, details, prefix, partial, root) - self.class.cached_glob[[name, prefix, partial, details, root]] ||= begin - path = "" - path << "#{prefix}/" unless prefix.empty? - path << (partial ? "_#{name}" : name) - - extensions = "" - [:locales, :formats].each do |k| - # TODO: OMG NO - if details[k] == [:"*/*"] - extensions << formats_glob if k == :formats - elsif exts = details[k] - extensions << '{' + exts.map {|e| ".#{e},"}.join + '}' - else - extensions << formats_glob if k == :formats - end - end - - "#{root}#{path}#{extensions}#{handler_glob}" + def build_path(name, details, prefix, partial) + path = "" + path << "#{prefix}/" unless prefix.empty? + path << (partial ? "_#{name}" : name) + path + end + + def query(path, exts) + query = "#{@path}/#{path}" + exts.each do |ext| + query << '{' << ext.map {|e| e && ".#{e}" }.join(',') << '}' end + + Dir[query].map do |path| + next if File.directory?(path) + source = File.read(path) + identifier = Pathname.new(path).expand_path.to_s + + Template.new(source, identifier, *path_to_details(path)) + end.compact end - # TODO: fix me - # :api: plugin + # # TODO: fix me + # # :api: plugin def path_to_details(path) # [:erb, :format => :html, :locale => :en, :partial => true/false] - if m = path.match(%r'/(_)?[\w-]+(\.[\w-]+)*\.(\w+)$') + if m = path.match(%r'(?:^|/)(_)?[\w-]+(\.[\w-]+)*\.(\w+)$') partial = m[1] == '_' details = (m[2]||"").split('.').reject { |e| e.empty? } handler = Template.handler_class_for_extension(m[3]) @@ -133,13 +132,32 @@ module ActionView end end - class FileSystemResolverWithFallback < FileSystemResolver + class FileSystemResolver < PathResolver + def initialize(path, options = {}) + raise ArgumentError, "path already is a Resolver class" if path.is_a?(Resolver) + super(options) + @path = Pathname.new(path).expand_path + end + end + + # OMG HAX + # TODO: remove hax + class FileSystemResolverWithFallback < Resolver + def initialize(path, options = {}) + super(options) + @paths = [FileSystemResolver.new(path, options), FileSystemResolver.new("", options), FileSystemResolver.new("/", options)] + end - def find_templates(name, details, prefix, partial) - templates = super - return super(name, details, prefix, partial, '') if templates.empty? - templates + def find_templates(*args) + @paths.each do |p| + template = p.find_templates(*args) + return template unless template.empty? + end + [] end + def to_s + @paths.first.to_s + end end end \ No newline at end of file diff --git a/actionpack/test/lib/fixture_template.rb b/actionpack/test/lib/fixture_template.rb index 9a9abb691d..0a365b3a7e 100644 --- a/actionpack/test/lib/fixture_template.rb +++ b/actionpack/test/lib/fixture_template.rb @@ -1,70 +1,28 @@ module ActionView #:nodoc: - class FixtureResolver < Resolver + class FixtureResolver < PathResolver def initialize(hash = {}, options = {}) super(options) @hash = hash end - def find_templates(name, details, prefix, partial) - if regexp = details_to_regexp(name, details, prefix, partial) - cached(regexp) do - templates = [] - @hash.select { |k,v| k =~ regexp }.each do |path, source| - templates << Template.new(source, path, *path_to_details(path)) - end - templates.sort_by {|t| -t.details.values.compact.size } - end - end - end - private - def formats_regexp - @formats_regexp ||= begin - formats = Mime::SET.symbols - '(?:' + formats.map { |l| "\\.#{Regexp.escape(l.to_s)}" }.join('|') + ')?' - end - end - - def handler_regexp - e = TemplateHandlers.extensions.map{|h| "\\.#{Regexp.escape(h.to_s)}"}.join("|") - "(?:#{e})" + def or_extensions(array) + "(?:" << array.map {|e| e && Regexp.escape(".#{e}")}.join("|") << ")" end - def details_to_regexp(name, details, prefix, partial) - path = "" - path << "#{prefix}/" unless prefix.empty? - path << (partial ? "_#{name}" : name) - - extensions = "" - [:locales, :formats].each do |k| - # TODO: OMG NO - if details[k] == [:"*/*"] - extensions << formats_regexp if k == :formats - elsif exts = details[k] - extensions << '(?:' + exts.map {|e| "\\.#{Regexp.escape(e.to_s)}"}.join('|') + ')?' - else - extensions << formats_regexp if k == :formats - end + def query(path, exts) + query = Regexp.escape(path) + exts.each do |ext| + query << '(?:' << ext.map {|e| e && Regexp.escape(".#{e}") }.join('|') << ')' end - %r'^#{Regexp.escape(path)}#{extensions}#{handler_regexp}$' - end - - # TODO: fix me - # :api: plugin - def path_to_details(path) - # [:erb, :format => :html, :locale => :en, :partial => true/false] - if m = path.match(%r'(_)?[\w-]+((?:\.[\w-]+)*)\.(\w+)$') - partial = m[1] == '_' - details = (m[2]||"").split('.').reject { |e| e.empty? } - handler = Template.handler_class_for_extension(m[3]) - - format = Mime[details.last] && details.pop.to_sym - locale = details.last && details.pop.to_sym - - return handler, :format => format, :locale => locale, :partial => partial + templates = [] + @hash.select { |k,v| k =~ /^#{query}$/ }.each do |path, source| + templates << Template.new(source, path, *path_to_details(path)) end + templates.sort_by {|t| -t.details.values.compact.size } end + end end \ No newline at end of file diff --git a/actionpack/test/new_base/render_file_test.rb b/actionpack/test/new_base/render_file_test.rb index 769949be0c..8d7f49dbc2 100644 --- a/actionpack/test/new_base/render_file_test.rb +++ b/actionpack/test/new_base/render_file_test.rb @@ -3,7 +3,7 @@ require File.join(File.expand_path(File.dirname(__FILE__)), "test_helper") module RenderFile class BasicController < ActionController::Base - self.view_paths = "." + self.view_paths = File.dirname(__FILE__) def index render :file => File.join(File.dirname(__FILE__), *%w[.. fixtures test hello_world]) diff --git a/actionpack/test/template/compiled_templates_test.rb b/actionpack/test/template/compiled_templates_test.rb index 7734e6da73..632988bb2e 100644 --- a/actionpack/test/template/compiled_templates_test.rb +++ b/actionpack/test/template/compiled_templates_test.rb @@ -14,6 +14,9 @@ class CompiledTemplatesTest < Test::Unit::TestCase assert_equal "two", render(:file => "test/render_file_with_locals_and_default.erb", :locals => { :secret => "two" }) end + # This is broken in 1.8.6 (not supported in Rails 3.0) because the cache uses a Hash + # key. Since Ruby 1.8.6 implements Hash#hash using the hash's object_id, it will never + # successfully get a cache hit here. def test_template_changes_are_not_reflected_with_cached_templates assert_equal "Hello world!", render(:file => "test/hello_world.erb") modify_template "test/hello_world.erb", "Goodbye world!" do -- cgit v1.2.3