Fix errors with around_filters which do not yield, restore 1.1 behaviour with...

Fix errors with around_filters which do not yield, restore 1.1 behaviour with after filters. Closes #8891 [skaes]


git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@7177 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

actionpack/CHANGELOG

 *SVN*
 
+* Fix errors with around_filters which do not yield, restore 1.1 behaviour with after filters. Closes #8891 [skaes]
+
+  After filters will *no longer* be run if an around_filter fails to yield, users relying on
+  this behaviour are advised to put the code in question after a yield statement in an around filter.
+  
+
 * Allow you to delete cookies with options. Closes #3685 [josh, Chris Wanstrath]
 
 * Allow you to render views with periods in the name.  Closes #8076 [norbert]

actionpack/lib/action_controller/filters.rb

@@ -214,9 +214,10 @@ def self.included(base)
     # == Filter Chain Halting
     #
     # <tt>before_filter</tt> and <tt>around_filter</tt> may halt the request
-    # before controller action is run. This is useful, for example, to deny
+    # before a controller action is run. This is useful, for example, to deny
     # access to unauthenticated users or to redirect from http to https.
     # Simply return false from the filter or call render or redirect.
+    # After filters will not be executed if the filter chain is halted.
     #
     # Around filters halt the request unless the action block is called.
     # Given these filters
@@ -238,12 +239,12 @@ def self.included(base)
     #   .  .  /
     #   .  #around (code after yield)
     #   . /
-    #   #after (actual filter code is run)
+    #   #after (actual filter code is run, unless the around filter does not yield)
     #
-    # If #around returns before yielding, only #after will be run. The #before
-    # filter and controller action will not be run.  If #before returns false,
-    # the second half of #around and all of #after will still run but the
-    # action will not.
+    # If #around returns before yielding, #after will still not be run. The #before
+    # filter and controller action will not be run. If #before returns false,
+    # the second half of #around and will still run but #after and the
+    # action will not. If #around fails to yield, #after will not be run.
     module ClassMethods
       # The passed <tt>filters</tt> will be appended to the filter_chain and
       # will execute before the action on this controller is performed.
@@ -439,7 +440,7 @@ def type
         def run(controller)
           # only filters returning false are halted.
           if false == @filter.call(controller)
-            controller.halt_filter_chain(@filter)
+            controller.send :halt_filter_chain, @filter, :returned_false
           end
         end
 
@@ -528,7 +529,7 @@ def update_filter_chain(filters, filter_type, pos, &block)
 
         def find_filter_append_position(filters, filter_type)
           # appending an after filter puts it at the end of the call chain
-          # before and around filters goe before the first after filter in the chain
+          # before and around filters go before the first after filter in the chain
           unless filter_type == :after
             filter_chain.each_with_index do |f,i|
               return i if f.after?
@@ -655,7 +656,7 @@ def proxy_before_and_after_filter(filter) #:nodoc:
           return filter unless filter_responds_to_before_and_after(filter)
           Proc.new do |controller, action|
             if filter.before(controller) == false
-              controller.send :halt_filter_chain, filter
+              controller.send :halt_filter_chain, filter, :returned_false
             else
               begin
                 action.call
@@ -675,8 +676,25 @@ def self.included(base)
         end
       end
 
-      def filter_chain
-        self.class.filter_chain
+      protected
+
+      def process_with_filters(request, response, method = :perform_action, *arguments) #:nodoc:
+        @before_filter_chain_aborted = false
+        process_without_filters(request, response, method, *arguments)
+      end
+
+      def perform_action_with_filters
+        call_filters(self.class.filter_chain, 0, 0)
+      end
+
+      private
+
+      def call_filters(chain, index, nesting)
+        index = run_before_filters(chain, index, nesting)
+        aborted = @before_filter_chain_aborted
+        perform_action_without_filters unless performed? || aborted
+        return index if nesting != 0 || aborted
+        run_after_filters(chain, index)
       end
 
       def skip_excluded_filters(chain, index)
@@ -686,72 +704,54 @@ def skip_excluded_filters(chain, index)
         [filter, index]
       end
 
-      def call_filters(chain, index, nesting)
-        # run before filters until we find an after filter or around filter
+      def run_before_filters(chain, index, nesting)
         while chain[index]
           filter, index = skip_excluded_filters(chain, index)
           break unless filter # end of call chain reached
           case filter.type
           when :before
-            # invoke before filter
-            filter.run(self)
+            filter.run(self)  # invoke before filter
             index = index.next
             break if @before_filter_chain_aborted
           when :around
+            yielded = false
             filter.call(self) do
+              yielded = true
               # all remaining before and around filters will be run in this call
               index = call_filters(chain, index.next, nesting.next)
             end
+            halt_filter_chain(filter, :did_not_yield) unless yielded
             break
           else
-            # no before or around filters left
-            break
+            break  # no before or around filters left
           end
         end
+        index
+      end
 
-        aborted = @before_filter_chain_aborted
-        perform_action_without_filters unless performed? || aborted
-        return index if aborted || nesting != 0
-
-        # if an around filter catches an exception during rendering and handles it, e.g.
-        # by rendering an error page, we might have left over around filters in the filter
-        # chain. so skip over these.
-        index = index.next while (filter = chain[index]) && filter.type == :around
-
-        # run after filters, if any
+      def run_after_filters(chain, index)
+        seen_after_filter = false
         while chain[index]
           filter, index = skip_excluded_filters(chain, index)
-          break unless filter
+          break unless filter # end of call chain reached
           case filter.type
           when :after
-            filter.run(self)
-            index = index.next
+            seen_after_filter = true
+            filter.run(self)  # invoke after filter
           else
-            raise ActionControllerError, "filter #{filter.inspect} was in the wrong place!"
+            # implementation error or someone has mucked with the filter chain
+            raise ActionControllerError, "filter #{filter.inspect} was in the wrong place!" if seen_after_filter
           end
+          index = index.next
         end
-
         index.next
       end
 
-      def halt_filter_chain(filter)
-        logger.info "Filter chain halted as [#{filter.inspect}] returned false." if logger
+      def halt_filter_chain(filter, reason)
         @before_filter_chain_aborted = true
+        logger.info "Filter chain halted as [#{filter.inspect}] #{reason}." if logger
         false
       end
-
-      protected
-
-        def process_with_filters(request, response, method = :perform_action, *arguments) #:nodoc:
-          @before_filter_chain_aborted = false
-          process_without_filters(request, response, method, *arguments)
-        end
-
-      private
-        def perform_action_with_filters
-          call_filters(self.class.filter_chain, 0, 0)
-        end
-
     end
   end
 end

actionpack/test/controller/filters_test.rb

@@ -324,9 +324,9 @@ def show
       render :text => 'hello'
     end
   end
-  
+
   class ErrorToRescue < Exception; end
-  
+
   class RescuingAroundFilterWithBlock
     def filter(controller)
       begin
@@ -336,20 +336,86 @@ def filter(controller)
       end
     end
   end
-  
+
   class RescuedController < ActionController::Base
     around_filter RescuingAroundFilterWithBlock.new
-    
+
     def show
       raise ErrorToRescue.new("Something made the bad noise.")
     end
-    
+
   private
     def rescue_action(exception)
       raise exception
     end
   end
 
+  class NonYieldingAroundFilterController < ActionController::Base
+
+    before_filter :filter_one
+    around_filter :non_yielding_filter
+    before_filter :filter_two
+    after_filter :filter_three
+
+    def index
+      render :inline => "index"
+    end
+
+    #make sure the controller complains
+    def rescue_action(e); raise e; end
+
+    private
+
+      def filter_one
+        @filters  ||= []
+        @filters  << "filter_one"
+      end
+
+      def filter_two
+        @filters  << "filter_two"
+      end
+
+      def non_yielding_filter
+        @filters  << "zomg it didn't yield"
+        @filter_return_value
+      end
+
+      def filter_three
+        @filters  << "filter_three"
+      end
+
+  end
+
+  def test_non_yielding_around_filters_not_returning_false_do_not_raise
+    controller = NonYieldingAroundFilterController.new
+    controller.instance_variable_set "@filter_return_value", true
+    assert_nothing_raised do
+      test_process(controller, "index")
+    end
+  end
+
+  def test_non_yielding_around_filters_returning_false_do_not_raise
+    controller = NonYieldingAroundFilterController.new
+    controller.instance_variable_set "@filter_return_value", false
+    assert_nothing_raised do
+      test_process(controller, "index")
+    end
+  end
+
+  def test_after_filters_are_not_run_if_around_filter_returns_false
+    controller = NonYieldingAroundFilterController.new
+    controller.instance_variable_set "@filter_return_value", false
+    test_process(controller, "index")
+    assert_equal ["filter_one", "zomg it didn't yield"], controller.assigns['filters']
+  end
+
+  def test_after_filters_are_not_run_if_around_filter_does_not_yield
+    controller = NonYieldingAroundFilterController.new
+    controller.instance_variable_set "@filter_return_value", true
+    test_process(controller, "index")
+    assert_equal ["filter_one", "zomg it didn't yield"], controller.assigns['filters']
+  end
+
   def test_empty_filter_chain
     assert_equal 0, EmptyFilterChainController.filter_chain.size
     assert test_process(EmptyFilterChainController).template.assigns['action_executed']
@@ -516,13 +582,13 @@ def test_condition_skipping_of_filters_when_siblings_also_have_conditions
   def test_changing_the_requirements
     assert_equal nil, test_process(ChangingTheRequirementsController, "go_wild").template.assigns['ran_filter']
   end
-  
+
   def test_a_rescuing_around_filter
     response = nil
     assert_nothing_raised do
       response = test_process(RescuedController)
     end
-    
+
     assert response.success?
     assert_equal("I rescued this: #<FilterTest::ErrorToRescue: Something made the bad noise.>", response.body)
   end