Optimize and clean up how details key get expired.

10 files changed, 72 insertions, 38 deletions

diff --git a/actionpack/lib/abstract_controller.rb b/actionpack/lib/abstract_controller.rb
index 7028cd0379..2da4dc052c 100644
--- a/actionpack/lib/abstract_controller.rb
+++ b/actionpack/lib/abstract_controller.rb
@@ -7,6 +7,7 @@ require 'active_support/core_ext/class/attribute'
 require 'active_support/core_ext/module/attr_internal'
 require 'active_support/core_ext/module/delegation'
 require 'active_support/core_ext/module/anonymous'
+require 'active_support/i18n'
 
 module AbstractController
   extend ActiveSupport::Autoload
diff --git a/actionpack/lib/abstract_controller/rendering.rb b/actionpack/lib/abstract_controller/rendering.rb
index 5c34b83563..42f4939108 100644
--- a/actionpack/lib/abstract_controller/rendering.rb
+++ b/actionpack/lib/abstract_controller/rendering.rb
@@ -9,10 +9,38 @@ module AbstractController
     end
   end
 
+  # This is a class to fix I18n global state. Whenever you provide I18n.locale during a request,
+  # it will trigger the lookup_context and consequently expire the cache.
+  # TODO Add some deprecation warnings to remove I18n.locale from controllers
+  class I18nProxy < ::I18n::Config #:nodoc:
+    attr_reader :i18n_config, :lookup_context
+
+    def initialize(i18n_config, lookup_context)
+      @i18n_config, @lookup_context = i18n_config, lookup_context
+    end
+
+    def locale
+      @i18n_config.locale
+    end
+
+    def locale=(value)
+      @i18n_config.locale = value
+      @lookup_context.update_details(:locale => @i18n_config.locale)
+    end
+  end
+
   module Rendering
     extend ActiveSupport::Concern
     include AbstractController::ViewPaths
 
+    # Overwrite process to setup I18n proxy.
+    def process(*) #:nodoc:
+      old_config, I18n.config = I18n.config, I18nProxy.new(I18n.config, lookup_context)
+      super
+    ensure
+      I18n.config = old_config
+    end
+
     # An instance of a view class. The default view class is ActionView::Base
     #
     # The view class must have the following methods:
diff --git a/actionpack/lib/abstract_controller/view_paths.rb b/actionpack/lib/abstract_controller/view_paths.rb
index 6513c0778e..c2a9f6336d 100644
--- a/actionpack/lib/abstract_controller/view_paths.rb
+++ b/actionpack/lib/abstract_controller/view_paths.rb
@@ -7,7 +7,8 @@ module AbstractController
       self._view_paths = ActionView::PathSet.new
     end
 
-    delegate :template_exists?, :view_paths, :formats, :formats=, :to => :lookup_context
+    delegate :template_exists?, :view_paths, :formats, :formats=,
+             :locale, :locale=, :to => :lookup_context
 
     # LookupContext is the object responsible to hold all information required to lookup
     # templates, i.e. view paths and details. Check ActionView::LookupContext for more
diff --git a/actionpack/lib/action_view.rb b/actionpack/lib/action_view.rb
index 7e9ac8720c..afe6386105 100644
--- a/actionpack/lib/action_view.rb
+++ b/actionpack/lib/action_view.rb
@@ -58,6 +58,7 @@ module ActionView
   autoload :TestCase, 'action_view/test_case'
 end
 
+require 'active_support/i18n'
 require 'active_support/core_ext/string/output_safety'
 
 I18n.load_path << "#{File.dirname(__FILE__)}/action_view/locale/en.yml"
diff --git a/actionpack/lib/action_view/base.rb b/actionpack/lib/action_view/base.rb
index 564ea6684c..ffe3060404 100644
--- a/actionpack/lib/action_view/base.rb
+++ b/actionpack/lib/action_view/base.rb
@@ -194,7 +194,7 @@ module ActionView #:nodoc:
     attr_accessor :base_path, :assigns, :template_extension, :lookup_context
     attr_internal :captures, :request, :layout, :controller, :template, :config
 
-    delegate :find, :exists?, :formats, :formats=,
+    delegate :find, :exists?, :formats, :formats=, :locale, :locale=,
              :view_paths, :view_paths=, :with_fallbacks, :update_details, :to => :lookup_context
 
     delegate :request_forgery_protection_token, :template, :params, :session, :cookies, :response, :headers,
diff --git a/actionpack/lib/action_view/lookup_context.rb b/actionpack/lib/action_view/lookup_context.rb
index 4a4dc0d06c..91885c7370 100644
--- a/actionpack/lib/action_view/lookup_context.rb
+++ b/actionpack/lib/action_view/lookup_context.rb
@@ -1,4 +1,5 @@
 require 'active_support/core_ext/object/try'
+require 'active_support/core_ext/object/blank'
 
 module ActionView
   # LookupContext is the object responsible to hold all information required to lookup
@@ -14,16 +15,14 @@ module ActionView
 
     def self.register_detail(name, options = {})
       registered_details[name] = lambda do |value|
-        value = (value.blank? || options[:accessible] == false) ?
-          Array(yield) : Array(value)
+        value = Array(value.presence || yield)
         value |= [nil] unless options[:allow_nil] == false
         value
       end
     end
 
     register_detail(:formats) { Mime::SET.symbols }
-    register_detail(:locale, :accessible => false) { [I18n.locale] }
-    register_detail(:handlers, :accessible => false) { Template::Handlers.extensions }
+    register_detail(:locale)  { [I18n.locale] }
 
     class DetailsKey #:nodoc:
       attr_reader :details
@@ -38,16 +37,12 @@ module ActionView
       def initialize(details)
         @details, @hash = details, details.hash
       end
-
-      def outdated?(details)
-        @details != details
-      end
     end
 
     def initialize(view_paths, details = {})
+      @details_key = nil
       self.view_paths = view_paths
       self.details = details
-      @details_key = nil
     end
 
     module ViewPaths
@@ -60,18 +55,15 @@ module ActionView
       end
 
       def find(name, prefix = nil, partial = false)
-        key = details_key
-        @view_paths.find(name, prefix, partial || false, key.details, key)
+        @view_paths.find(name, prefix, partial || false, details, details_key)
       end
 
       def find_all(name, prefix = nil, partial = false)
-        key = details_key
-        @view_paths.find_all(name, prefix, partial || false, key.details, key)
+        @view_paths.find_all(name, prefix, partial || false, details, details_key)
       end
 
       def exists?(name, prefix = nil, partial = false)
-        key = details_key
-        @view_paths.exists?(name, prefix, partial || false, key.details, key)
+        @view_paths.exists?(name, prefix, partial || false, details, details_key)
       end
 
       # Add fallbacks to the view paths. Useful in cases you are rendering a file.
@@ -89,25 +81,20 @@ module ActionView
     end
 
     module Details
-      def details
-        @details = normalize_details(@details)
-      end
+      attr_reader :details
 
-      def details=(new_details)
-        @details = new_details
-        details
+      def details=(details)
+        @details = normalize_details(details)
+        @details_key = nil if @details_key && @details_key.details != @details
       end
 
-      # TODO This is too expensive. Revisit this.
       def details_key
-        latest_details = self.details
-        @details_key   = nil if @details_key.try(:outdated?, latest_details)
-        @details_key ||= DetailsKey.get(latest_details)
+        @details_key ||= DetailsKey.get(@details)
       end
 
       # Shortcut to read formats from details.
       def formats
-        self.details[:formats]
+        @details[:formats].compact
       end
 
       # Shortcut to set formats in details.
@@ -115,11 +102,25 @@ module ActionView
         self.details = @details.merge(:formats => value)
       end
 
+      # Shortcut to read locale.
+      def locale
+        I18n.locale
+      end
+
+      # Shortcut to set locale in details and I18n.
+      def locale=(value)
+        I18n.locale = value
+
+        unless I18n.config.respond_to?(:lookup_context)
+          self.details = @details.merge(:locale => value)
+        end
+      end
+
       # Update the details keys by merging the given hash into the current
       # details hash. If a block is given, the details are modified just during
       # the execution of the block and reverted to the previous value after.
       def update_details(new_details)
-        old_details  = self.details
+        old_details  = @details
         self.details = old_details.merge(new_details)
 
         if block_given?
@@ -140,7 +141,7 @@ module ActionView
         self.class.registered_details.each do |k, v|
           details[k] = v.call(details[k])
         end
-        details
+        details.freeze
       end
     end
 
diff --git a/actionpack/lib/action_view/template/resolver.rb b/actionpack/lib/action_view/template/resolver.rb
index c43b357e9e..a43597e728 100644
--- a/actionpack/lib/action_view/template/resolver.rb
+++ b/actionpack/lib/action_view/template/resolver.rb
@@ -21,6 +21,7 @@ module ActionView
     # Normalizes the arguments and passes it on to find_template.
     def find_all(name, prefix=nil, partial=false, details={}, key=nil)
       name, prefix = normalize_name(name, prefix)
+      details = details.merge(:handlers => default_handlers)
 
       cached(key, prefix, name, partial) do
         find_templates(name, prefix, partial, details)
@@ -33,6 +34,10 @@ module ActionView
       @caching ||= !defined?(Rails.application) || Rails.application.config.cache_classes
     end
 
+    def default_handlers
+      Template::Handlers.extensions + [nil]
+    end
+
     # This is what child classes implement. No defaults are needed
     # because Resolver guarantees that the arguments are present and
     # normalized.
diff --git a/actionpack/test/template/compiled_templates_test.rb b/actionpack/test/template/compiled_templates_test.rb
index 0a3409064f..2c719757e4 100644
--- a/actionpack/test/template/compiled_templates_test.rb
+++ b/actionpack/test/template/compiled_templates_test.rb
@@ -14,9 +14,6 @@ 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
diff --git a/actionpack/test/template/render_test.rb b/actionpack/test/template/render_test.rb
index 37a3975a54..cea8ab1bce 100644
--- a/actionpack/test/template/render_test.rb
+++ b/actionpack/test/template/render_test.rb
@@ -33,17 +33,17 @@ module RenderTestCases
   end
 
   def test_render_file_with_localization
-    old_locale, I18n.locale = I18n.locale, :da
+    old_locale, @view.locale = @view.locale, :da
     assert_equal "Hey verden", @view.render(:file => "test/hello_world")
   ensure
-    I18n.locale = old_locale
+    @view.locale = old_locale
   end
 
   def test_render_file_with_dashed_locale
-    old_locale, I18n.locale = I18n.locale, :"pt-BR"
+    old_locale, @view.locale = @view.locale, :"pt-BR"
     assert_equal "Ola mundo", @view.render(:file => "test/hello_world")
   ensure
-    I18n.locale = old_locale
+    @view.locale = old_locale
   end
 
   def test_render_file_at_top_level
diff --git a/activesupport/activesupport.gemspec b/activesupport/activesupport.gemspec
index 3b5c7203b3..78fb48924e 100644
--- a/activesupport/activesupport.gemspec
+++ b/activesupport/activesupport.gemspec
@@ -18,7 +18,7 @@ Gem::Specification.new do |s|
 
   s.has_rdoc = true
 
-  s.add_dependency('i18n',            '~> 0.3.4')
+  s.add_dependency('i18n',            '~> 0.3.6.pre')
   s.add_dependency('tzinfo',          '~> 0.3.16')
   s.add_dependency('builder',         '~> 2.1.2')
   s.add_dependency('memcache-client', '~> 1.7.5')