First pass at cleaning up action caching

actionpack/lib/action_controller/caching.rb

@@ -52,19 +52,23 @@ module ActionController #:nodoc:
       end
     end
 
-    protected
+    def caching_allowed?
-      # Convenience accessor
+      request.get? && response.status == 200
-      def cache(key, options = {}, &block)
+    end
-        if cache_configured?
-          cache_store.fetch(ActiveSupport::Cache.expand_cache_key(key, :controller), options, &block)
-        else
-          yield
-        end
-      end
 
-    private
+  protected
-      def cache_configured?
+    # Convenience accessor
-        self.class.cache_configured?
+    def cache(key, options = {}, &block)
+      if cache_configured?
+        cache_store.fetch(ActiveSupport::Cache.expand_cache_key(key, :controller), options, &block)
+      else
+        yield
       end
+    end
+
+  private
+    def cache_configured?
+      self.class.cache_configured?
+    end
   end
 end

actionpack/lib/action_controller/caching/actions.rb

@@ -2,9 +2,10 @@ require 'set'
 
 module ActionController #:nodoc:
   module Caching
-    # Action caching is similar to page caching by the fact that the entire output of the response is cached, but unlike page caching,
+    # Action caching is similar to page caching by the fact that the entire output of the response is
-    # every request still goes through the Action Pack. The key benefit of this is that filters are run before the cache is served, which
+    # cached, but unlike page caching, every request still goes through the Action Pack. The key benefit
-    # allows for authentication and other restrictions on whether someone is allowed to see the cache. Example:
+    # of this is that filters are run before the cache is served, which allows for authentication and other
+    # restrictions on whether someone is allowed to see the cache. Example:
     #
     #   class ListsController < ApplicationController
     #     before_filter :authenticate, :except => :public
@@ -12,44 +13,53 @@ module ActionController #:nodoc:
     #     caches_action :index, :show, :feed
     #   end
     #
-    # In this example, the public action doesn't require authentication, so it's possible to use the faster page caching method. But both the
+    # In this example, the public action doesn't require authentication, so it's possible to use the faster
-    # show and feed action are to be shielded behind the authenticate filter, so we need to implement those as action caches.
+    # page caching method. But both the show and feed action are to be shielded behind the authenticate
+    # filter, so we need to implement those as action caches.
     #
-    # Action caching internally uses the fragment caching and an around filter to do the job. The fragment cache is named according to both
+    # Action caching internally uses the fragment caching and an around filter to do the job. The fragment
-    # the current host and the path. So a page that is accessed at http://david.somewhere.com/lists/show/1 will result in a fragment named
+    # cache is named according to both the current host and the path. So a page that is accessed at
-    # "david.somewhere.com/lists/show/1". This allows the cacher to differentiate between "david.somewhere.com/lists/" and
+    # http://david.somewhere.com/lists/show/1 will result in a fragment named
-    # "jamis.somewhere.com/lists/" -- which is a helpful way of assisting the subdomain-as-account-key pattern.
+    # "david.somewhere.com/lists/show/1". This allows the cacher to differentiate between
+    # "david.somewhere.com/lists/" and
+    # "jamis.somewhere.com/lists/" -- which is a helpful way of assisting the subdomain-as-account-key
+    # pattern.
     #
-    # Different representations of the same resource, e.g. <tt>http://david.somewhere.com/lists</tt> and <tt>http://david.somewhere.com/lists.xml</tt>
+    # Different representations of the same resource, e.g. <tt>http://david.somewhere.com/lists</tt> and
-    # are treated like separate requests and so are cached separately. Keep in mind when expiring an action cache that <tt>:action => 'lists'</tt> is not the same
+    # <tt>http://david.somewhere.com/lists.xml</tt>
-    # as <tt>:action => 'list', :format => :xml</tt>.
+    # are treated like separate requests and so are cached separately. Keep in mind when expiring an
+    # action cache that <tt>:action => 'lists'</tt> is not the same as
+    # <tt>:action => 'list', :format => :xml</tt>.
     #
-    # You can set modify the default action cache path by passing a :cache_path option.  This will be passed directly to ActionCachePath.path_for.  This is handy
+    # You can set modify the default action cache path by passing a :cache_path option.  This will be
-    # for actions with multiple possible routes that should be cached differently.  If a block is given, it is called with the current controller instance.
+    # passed directly to ActionCachePath.path_for.  This is handy for actions with multiple possible
+    # routes that should be cached differently.  If a block is given, it is called with the current
+    # controller instance.
     #
-    # And you can also use :if (or :unless) to pass a Proc that specifies when the action should be cached.
+    # And you can also use :if (or :unless) to pass a Proc that specifies when the action should
+    # be cached.
     #
     # Finally, if you are using memcached, you can also pass :expires_in.
     #
     #   class ListsController < ApplicationController
     #     before_filter :authenticate, :except => :public
     #     caches_page   :public
-    #     caches_action :index, :if => Proc.new { |c| !c.request.format.json? } # cache if is not a JSON request
+    #     caches_action :index, :if => proc { |c| !c.request.format.json? } # cache if is not a JSON request
     #     caches_action :show, :cache_path => { :project => 1 }, :expires_in => 1.hour
-    #     caches_action :feed, :cache_path => Proc.new { |controller|
+    #     caches_action :feed, :cache_path => proc { |controller|
     #       controller.params[:user_id] ?
     #         controller.send(:user_list_url, controller.params[:user_id], controller.params[:id]) :
     #         controller.send(:list_url, controller.params[:id]) }
     #   end
     #
-    # If you pass :layout => false, it will only cache your action content. It is useful when your layout has dynamic information.
+    # If you pass :layout => false, it will only cache your action content. It is useful when your
+    # layout has dynamic information.
     #
     module Actions
-      def self.included(base) #:nodoc:
+      extend ActiveSupport::Concern
-        base.extend(ClassMethods)
+
-          base.class_eval do
+      included do
-            attr_accessor :rendered_action_cache, :action_cache_path
+        attr_accessor :rendered_action_cache, :action_cache_path
-          end
       end
 
       module ClassMethods
@@ -58,22 +68,35 @@ module ActionController #:nodoc:
         def caches_action(*actions)
           return unless cache_configured?
           options = actions.extract_options!
-          filter_options = { :only => actions, :if => options.delete(:if), :unless => options.delete(:unless) }
+          filter_options = options.extract!(:if, :unless).merge(:only => actions)
+          cache_options  = options.extract!(:layout, :cache_path).merge(:store_options => options)
 
-          cache_filter = ActionCacheFilter.new(:layout => options.delete(:layout), :cache_path => options.delete(:cache_path), :store_options => options)
+          around_filter ActionCacheFilter.new(cache_options), filter_options
-
-          around_filter cache_filter, filter_options
         end
       end
 
+      def _render_cache_fragment(cache, extension, layout)
+        self.rendered_action_cache = true
+        response.content_type = Mime[extension].to_s if extension
+        options = { :text => cache }
+        options.merge!(:layout => true) if layout
+        render options
+      end
+
+      def _save_fragment(name, layout, options)
+        return unless caching_allowed?
+
+        content = layout ? view_context.content_for(:layout) : response_body
+        write_fragment(name, content, options)
+      end
+
       protected
         def expire_action(options = {})
           return unless cache_configured?
 
-          if options[:action].is_a?(Array)
+          actions = options[:action]
-            options[:action].dup.each do |action|
+          if actions.is_a?(Array)
-              expire_fragment(ActionCachePath.path_for(self, options.merge({ :action => action }), false))
+            actions.each {|action| expire_action(options.merge(:action => action)) }
-            end
           else
             expire_fragment(ActionCachePath.path_for(self, options, false))
           end
@@ -81,57 +104,21 @@ module ActionController #:nodoc:
 
       class ActionCacheFilter #:nodoc:
         def initialize(options, &block)
-          @options = options
+          @cache_path, @store_options, @layout =
+            options.values_at(:cache_path, :store_options, :layout)
         end
 
         def filter(controller)
-          should_continue = before(controller)
+          path_options = @cache_path.respond_to?(:call) ? @cache_path.call(controller) : @cache_path
-          yield if should_continue
+          cache_path = ActionCachePath.new(controller, path_options || {})
-          after(controller)
-        end
 
-        def before(controller)
+          if cache = controller.read_fragment(cache_path.path, @store_options)
-          cache_path = ActionCachePath.new(controller, path_options_for(controller, @options.slice(:cache_path)))
+            controller._render_cache_fragment(cache, cache_path.extension, @layout == false)
-
-          if cache = controller.read_fragment(cache_path.path, @options[:store_options])
-            controller.rendered_action_cache = true
-            set_content_type!(controller, cache_path.extension)
-            options = { :text => cache }
-            options.merge!(:layout => true) if cache_layout?
-            controller.__send__(:render, options)
-            false
           else
-            controller.action_cache_path = cache_path
+            yield
+            controller._save_fragment(cache_path.path, @layout == false, @store_options)
           end
         end
-
-        def after(controller)
-          return if controller.rendered_action_cache || !caching_allowed(controller)
-          action_content = cache_layout? ? content_for_layout(controller) : controller.response.body
-          controller.write_fragment(controller.action_cache_path.path, action_content, @options[:store_options])
-        end
-
-        private
-          def set_content_type!(controller, extension)
-            controller.response.content_type = Mime::Type.lookup_by_extension(extension).to_s if extension
-          end
-
-          def path_options_for(controller, options)
-            ((path_options = options[:cache_path]).respond_to?(:call) ? path_options.call(controller) : path_options) || {}
-          end
-
-          def caching_allowed(controller)
-            controller.request.get? && controller.response.status.to_i == 200
-          end
-
-          def cache_layout?
-            @options[:layout] == false
-          end
-
-          def content_for_layout(controller)
-            template = controller.view_context
-            template.layout && template.instance_variable_get('@cached_content_for_layout')
-          end
       end
 
       class ActionCachePath
@@ -142,10 +129,11 @@ module ActionController #:nodoc:
             new(controller, options, infer_extension).path
           end
         end
-        
+
-        # If +infer_extension+ is true, the cache path extension is looked up from the request's path & format.
+        # If +infer_extension+ is true, the cache path extension is looked up from the request's
-        # This is desirable when reading and writing the cache, but not when expiring the cache -
+        # path & format. This is desirable when reading and writing the cache, but not when
-        # expire_action should expire the same files regardless of the request format.
+        # expiring the cache - expire_action should expire the same files regardless of the
+        # request format.
         def initialize(controller, options = {}, infer_extension = true)
           if infer_extension
             extract_extension(controller.request)
@@ -158,20 +146,20 @@ module ActionController #:nodoc:
           @path = URI.unescape(path)
         end
 
-        private
+      private
-          def normalize!(path)
+        def normalize!(path)
-            path << 'index' if path[-1] == ?/
+          path << 'index' if path[-1] == ?/
-          end
+        end
 
-          def add_extension!(path, extension)
+        def add_extension!(path, extension)
-            path << ".#{extension}" if extension and !path.ends_with?(extension)
+          path << ".#{extension}" if extension and !path.ends_with?(extension)
-          end
+        end
-          
+
-          def extract_extension(request)
+        def extract_extension(request)
-            # Don't want just what comes after the last '.' to accommodate multi part extensions
+          # Don't want just what comes after the last '.' to accommodate multi part extensions
-            # such as tar.gz.
+          # such as tar.gz.
-            @extension = request.path[/^[^.]+\.(.+)$/, 1] || request.cache_format
+          @extension = request.path[/^[^.]+\.(.+)$/, 1] || request.cache_format
-          end
+        end
       end
     end
   end

actionpack/lib/action_view/render/rendering.rb

@@ -123,7 +123,6 @@ module ActionView
         template.render(self, locals)
       end
 
-      @cached_content_for_layout = content
       @_content_for[:layout] = content
 
       if layout

activesupport/lib/active_support/core_ext/hash/slice.rb

@@ -29,4 +29,10 @@ class Hash
     replace(hash)
     omit
   end
+
+  def extract!(*keys)
+    result = {}
+    keys.each {|key| result[key] = delete(key) }
+    result
+  end
 end