Ticket #8891: fix_for_around_filters_returning_false.3.patch

actionpack/lib/action_controller/filters.rb

@@ -214 +214 @@
-    # == Filter Chain Halting
-    #
-    # <tt>before_filter</tt> and <tt>around_filter</tt> may halt the request
-
+a 
-    # 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 no be executed if the filter chain is halted.
-    #
-    # Around filters halt the request unless the action block is called.
-    # Given these filters
@@ -238 +239 @@
-    #   .  .  /
-    #   .  #around (code after yield)
-    #   . /
-
+, unless around filter returns false or does not yield
-    #
-only #after will
- 
-all of #after will still run but
-
+#after will still not
+
+will still run but #after and
+ If #around returns false, #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 +440 @@
-        def run(controller)
-          # only filters returning false are halted.
-          if false == @filter.call(controller)
-halt_filter_chain(@filter)
+send :halt_filter_chain, @filter, :returned_false
-          end
-        end
-
@@ -528 +529 @@
-
-        def find_filter_append_position(filters, filter_type)
-          # appending an after filter puts it at the end of the call chain
-e
+
-          unless filter_type == :after
-            filter_chain.each_with_index do |f,i|
-              return i if f.after?
@@ -655 +656 @@
-          return filter unless filter_responds_to_before_and_after(filter)
-          Proc.new do |controller, action|
-            if filter.before(controller) == false
-
+, :returned_false
-            else
-              begin
-                action.call
@@ -675 +676 @@
-        end
-      end
-
-
-
+
+
+
+
+
-      end
-
+      private
+
+      def perform_action_with_filters
+        call_filters(self.class.filter_chain, 0, 0)
+      end
+
+      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)
-        while (filter = chain[index]) && self.class.filter_excluded_from_action?(filter, action_name)
-          index = index.next
@@ -686 +704 @@
-        [filter, index]
-      end
-
-
-
+
-        while chain[index]
-          filter, index = skip_excluded_filters(chain, index)
-          break unless filter # end of call chain reached
-          case filter.type
-          when :before
-
-
+
-            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
-
-
+
-          end
-        end
+        index
+      end
-
-
-
-
-
-
-
-
-
-
-
+
+
-        while chain[index]
-          filter, index = skip_excluded_filters(chain, index)
-
+ # 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
-
+
+
-          end
+          index = index.next
-        end
-
-        index.next
-      end
-
-
-
+
-        @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 +324 @@
-      render :text => 'hello'
-    end
-  end
-
+
-  class ErrorToRescue < Exception; end
-
+
-  class RescuingAroundFilterWithBlock
-    def filter(controller)
-      begin
@@ -336 +336 @@
-      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 +582 @@
-  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