Changeset 7177

Timestamp:

07/11/07 02:29:25 (1 year ago)

Author:

nzkoz

Message:

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

Files:

• trunk/actionpack/CHANGELOG (modified) (1 diff)
• trunk/actionpack/lib/action_controller/filters.rb (modified) (8 diffs)
• trunk/actionpack/test/controller/filters_test.rb (modified) (4 diffs)

trunk/actionpack/CHANGELOG

@@ -1 +1 @@
-*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]

trunk/actionpack/lib/action_controller/filters.rb

@@ -215 +215 @@
-    #
-    # <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 not be executed if the filter chain is halted.
-    #
-    # Around filters halt the request unless the action block is called.
@@ -239 +240 @@
-    #   .  #around (code after yield)
-    #   . /
-
-
-only #after will
- 
-all of #after will still run but
-
+, unless the around filter does not yield
+
+#after will still not
+
+will still run but #after and
+ 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
@@ -440 +441 @@
-          # only filters returning false are halted.
-          if false == @filter.call(controller)
-halt_filter_chain(@filter)
+send :halt_filter_chain, @filter, :returned_false
-          end
-        end
@@ -529 +530 @@
-        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|
@@ -656 +657 @@
-          Proc.new do |controller, action|
-            if filter.before(controller) == false
-
+, :returned_false
-            else
-              begin
@@ -676 +677 @@
-      end
-
-
-
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
-      end
-
@@ -687 +705 @@
-      end
-
-
-
+
-        while chain[index]
-          filter, index = skip_excluded_filters(chain, index)
@@ -694 +711 @@
-          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
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
+
+
+
+
+
+
+
+
-        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
-
-
-
-
+
+
+
+
+
-        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

trunk/actionpack/test/controller/filters_test.rb

@@ -325 +325 @@
-    end
-  end
-
+
-  class ErrorToRescue < Exception; end
-
+
-  class RescuingAroundFilterWithBlock
-    def filter(controller)
@@ -337 +337 @@
-    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
-
@@ -517 +583 @@
-    assert_equal nil, test_process(ChangingTheRequirementsController, "go_wild").template.assigns['ran_filter']
-  end
-
+
-  def test_a_rescuing_around_filter
-    response = nil
@@ -523 +589 @@
-      response = test_process(RescuedController)
-    end
-
+
-    assert response.success?
-    assert_equal("I rescued this: #<FilterTest::ErrorToRescue: Something made the bad noise.>", response.body)