Bug fix for the filter implementation

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

2 files changed, 52 insertions, 3 deletions

diff --git a/actionpack/lib/action_controller/filters.rb b/actionpack/lib/action_controller/filters.rb
index 3543c80ec9..d87df2577c 100644
--- a/actionpack/lib/action_controller/filters.rb
+++ b/actionpack/lib/action_controller/filters.rb
@@ -527,6 +527,8 @@ module ActionController #:nodoc:
         end
 
         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
           unless filter_type == :after
             filter_chain.each_with_index do |f,i|
               return i if f.after?
@@ -536,10 +538,13 @@ module ActionController #:nodoc:
         end
 
         def find_filter_prepend_position(filters, filter_type)
+          # prepending a before or around filter puts it at the front of the call chain
+          # after filters go before the first after filter in the chain
           if filter_type == :after
             filter_chain.each_with_index do |f,i|
               return i if f.after?
             end
+            return -1
           end
           return 0
         end
@@ -684,9 +689,9 @@ module ActionController #:nodoc:
 
       def call_filters(chain, index, nesting)
         # run before filters until we find an after filter or around filter
-        while true
+        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 :before
             # invoke before filter
@@ -710,8 +715,9 @@ module ActionController #:nodoc:
         return index if aborted || nesting != 0
 
         # run after filters, if any
-        while filter = chain[index]
+        while chain[index]
           filter, index = skip_excluded_filters(chain, index)
+          break unless filter
           case filter.type
           when :after
             filter.run(self)
diff --git a/actionpack/test/controller/filters_test.rb b/actionpack/test/controller/filters_test.rb
index f250b24f00..e84661b688 100644
--- a/actionpack/test/controller/filters_test.rb
+++ b/actionpack/test/controller/filters_test.rb
@@ -131,6 +131,14 @@ class FilterTest < Test::Unit::TestCase
     before_filter(ConditionalClassFilter, :ensure_login, Proc.new {|c| c.assigns["ran_proc_filter1"] = true }, :except => :show_without_filter) { |c| c.assigns["ran_proc_filter2"] = true}
   end
 
+  class EmptyFilterChainController < TestController
+    self.filter_chain.clear
+    def show
+      @action_executed = true
+      render :text => "yawp!"
+    end
+  end
+
   class PrependingController < TestController
     prepend_before_filter :wonderful_life
     # skip_before_filter :fire_flash
@@ -293,6 +301,35 @@ class FilterTest < Test::Unit::TestCase
       end
   end
 
+  class PrependingBeforeAndAfterController < ActionController::Base
+    prepend_before_filter :before_all
+    prepend_after_filter :after_all
+    before_filter :between_before_all_and_after_all
+
+    def before_all
+      @ran_filter ||= []
+      @ran_filter << 'before_all'
+    end
+
+    def after_all
+      @ran_filter ||= []
+      @ran_filter << 'after_all'
+    end
+
+    def between_before_all_and_after_all
+      @ran_filter ||= []
+      @ran_filter << 'between_before_all_and_after_all'
+    end
+    def show
+      render :text => 'hello'
+    end
+  end
+
+  def test_empty_filter_chain
+    assert_equal 0, EmptyFilterChainController.filter_chain.size
+    assert test_process(EmptyFilterChainController).template.assigns['action_executed']
+  end
+
   def test_added_filter_to_inheritance_graph
     assert_equal [ :ensure_login ], TestController.before_filters
   end
@@ -426,6 +463,12 @@ class FilterTest < Test::Unit::TestCase
     end
   end
 
+  def test_running_prepended_before_and_after_filter
+    assert_equal 3, PrependingBeforeAndAfterController.filter_chain.length
+    response = test_process(PrependingBeforeAndAfterController)
+    assert_equal %w( before_all between_before_all_and_after_all after_all ), response.template.assigns["ran_filter"]
+  end
+
   def test_conditional_skipping_of_filters
     assert_nil test_process(ConditionalSkippingController, "login").template.assigns["ran_filter"]
     assert_equal %w( ensure_login find_user ), test_process(ConditionalSkippingController, "change_password").template.assigns["ran_filter"]