diff options
| -rw-r--r-- | actionpack/CHANGELOG | 6 | ||||
| -rw-r--r-- | actionpack/lib/action_controller/filters.rb | 94 | ||||
| -rw-r--r-- | actionpack/test/controller/filters_test.rb | 80 |
3 files changed, 126 insertions, 54 deletions
diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG index 83e4a492ed..88832a6f2a 100644 --- a/actionpack/CHANGELOG +++ b/actionpack/CHANGELOG @@ -1,5 +1,11 @@ *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] diff --git a/actionpack/lib/action_controller/filters.rb b/actionpack/lib/action_controller/filters.rb index 5705960eb5..c2f2c2a432 100644 --- a/actionpack/lib/action_controller/filters.rb +++ b/actionpack/lib/action_controller/filters.rb @@ -214,9 +214,10 @@ module ActionController #:nodoc: # == 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 @@ module ActionController #:nodoc: # . . / # . #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 @@ module ActionController #:nodoc: 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 @@ module ActionController #:nodoc: 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 @@ module ActionController #: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 @@ module ActionController #:nodoc: 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 @@ module ActionController #:nodoc: [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 diff --git a/actionpack/test/controller/filters_test.rb b/actionpack/test/controller/filters_test.rb index a38e55fb52..8adee4c12f 100644 --- a/actionpack/test/controller/filters_test.rb +++ b/actionpack/test/controller/filters_test.rb @@ -324,9 +324,9 @@ class FilterTest < Test::Unit::TestCase render :text => 'hello' end end - + class ErrorToRescue < Exception; end - + class RescuingAroundFilterWithBlock def filter(controller) begin @@ -336,20 +336,86 @@ class FilterTest < Test::Unit::TestCase 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 @@ class FilterTest < Test::Unit::TestCase 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 |