Refactor filters to use Active Support callbacks. Closes #11235.

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

actionpack/CHANGELOG

 *SVN*
 
+* Refactor filters to use Active Support callbacks.  #11235 [Josh Peek]
+
 * Fixed that polymorphic routes would modify the input array #11363 [thomas.lee]
 
 * Added :format option to NumberHelper#number_to_currency to enable better localization support #11149 [lylo]

actionpack/lib/action_controller/dispatcher.rb

@@ -20,17 +20,9 @@ def dispatch(cgi = nil, session_options = CgiRequest::DEFAULT_SESSION_OPTIONS, o
       # existing callback. Passing an identifier is a suggested practice if the
       # code adding a preparation block may be reloaded.
       def to_prepare(identifier = nil, &block)
-        @prepare_dispatch_callbacks ||= []
+        @prepare_dispatch_callbacks ||= ActiveSupport::Callbacks::CallbackChain.new
         callback = ActiveSupport::Callbacks::Callback.new(:prepare_dispatch, block, :identifier => identifier)
-
-        # Already registered: update the existing callback
-        # TODO: Ruby one liner for Array#find returning index
-        if identifier && callback_for_identifier = @prepare_dispatch_callbacks.find { |c| c.identifier == identifier }
-          index = @prepare_dispatch_callbacks.index(callback_for_identifier)
-          @prepare_dispatch_callbacks[index] = callback
-        else
-          @prepare_dispatch_callbacks.concat([callback])
-        end
+        @prepare_dispatch_callbacks.replace_or_append_callback(callback)
       end
 
       # If the block raises, send status code as a last-ditch response.

actionpack/test/controller/dispatcher_test.rb

@@ -11,7 +11,7 @@ def setup
     @output = StringIO.new
     ENV['REQUEST_METHOD'] = 'GET'
 
-    Dispatcher.instance_variable_set("@prepare_dispatch_callbacks", [])
+    Dispatcher.instance_variable_set("@prepare_dispatch_callbacks", ActiveSupport::Callbacks::CallbackChain.new)
     @dispatcher = Dispatcher.new(@output)
   end
 

actionpack/test/controller/filters_test.rb

@@ -134,6 +134,11 @@ class AnomolousYetValidConditionController < ConditionalFilterController
     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 ConditionalOptionsFilter < ConditionalFilterController
+    before_filter :ensure_login, :if => Proc.new { |c| true }
+    before_filter :clean_up_tmp, :if => Proc.new { |c| false }
+  end
+
   class EmptyFilterChainController < TestController
     self.filter_chain.clear
     def show
@@ -466,6 +471,11 @@ def test_running_anomolous_yet_valid_condition_filters
     assert !response.template.assigns["ran_proc_filter2"]
   end
 
+  def test_running_conditional_options
+    response = test_process(ConditionalOptionsFilter)
+    assert_equal %w( ensure_login ), response.template.assigns["ran_filter"]
+  end
+
   def test_running_collection_condition_filters
     assert_equal %w( ensure_login ), test_process(ConditionalCollectionFilterController).template.assigns["ran_filter"]
     assert_equal nil, test_process(ConditionalCollectionFilterController, "show_without_filter").template.assigns["ran_filter"]
@@ -499,13 +509,6 @@ def test_running_before_and_after_condition_filters
     assert_equal nil, test_process(BeforeAndAfterConditionController, "show_without_filter").template.assigns["ran_filter"]
   end
 
-  def test_bad_filter
-    bad_filter_controller = Class.new(ActionController::Base)
-    assert_raises(ActionController::ActionControllerError) do
-      bad_filter_controller.before_filter 2
-    end
-  end
-
   def test_around_filter
     controller = test_process(AroundFilterController)
     assert controller.template.assigns["before_ran"]
@@ -746,14 +749,6 @@ def test_filters_registering
     assert_equal 4, ControllerWithAllTypesOfFilters.filter_chain.size
   end
 
-  def test_wrong_filter_type
-    assert_raise ArgumentError do
-      Class.new PostsController do
-        around_filter lambda { yield }
-      end
-    end
-  end
-
   def test_base
     controller = PostsController
     assert_nothing_raised { test_process(controller,'no_raise') }

activerecord/lib/active_record/validations.rb

@@ -282,19 +282,20 @@ def self.included(base) # :nodoc:
 
       base.send :include, ActiveSupport::Callbacks
 
-      # TODO: Use helper ActiveSupport::Callbacks#define_callbacks instead
-      %w( validate validate_on_create validate_on_update ).each do |validation_method|
+      VALIDATIONS.each do |validation_method|
         base.class_eval <<-"end_eval"
           def self.#{validation_method}(*methods, &block)
-            options = methods.extract_options!
-            methods << block if block_given?
-            methods.map! { |method| Callback.new(:#{validation_method}, method, options) }
-            existing_methods = read_inheritable_attribute(:#{validation_method}) || []
-            write_inheritable_attribute(:#{validation_method}, existing_methods | methods)
+            methods = CallbackChain.build(:#{validation_method}, *methods, &block)
+            self.#{validation_method}_callback_chain.replace(#{validation_method}_callback_chain | methods)
           end
 
           def self.#{validation_method}_callback_chain
-            read_inheritable_attribute(:#{validation_method}) || []
+            if chain = read_inheritable_attribute(:#{validation_method})
+              return chain
+            else
+              write_inheritable_attribute(:#{validation_method}, CallbackChain.new)
+              return #{validation_method}_callback_chain
+            end
           end
         end_eval
       end

activesupport/lib/active_support/callbacks.rb

@@ -76,20 +76,53 @@ module ActiveSupport
   #   - save
   #   saved
   module Callbacks
-    class Callback
-      def self.run(callbacks, object, options = {}, &terminator)
-        enumerator  = options[:enumerator] || :each
+    class CallbackChain < Array
+      def self.build(kind, *methods, &block)
+        methods, options = extract_options(*methods, &block)
+        methods.map! { |method| Callback.new(kind, method, options) }
+        new(methods)
+      end
+
+      def run(object, options = {}, &terminator)
+        enumerator = options[:enumerator] || :each
 
         unless block_given?
-          callbacks.send(enumerator) { |callback| callback.call(object) }
+          send(enumerator) { |callback| callback.call(object) }
         else
-          callbacks.send(enumerator) do |callback|
+          send(enumerator) do |callback|
             result = callback.call(object)
             break result if terminator.call(result, object)
           end
         end
       end
 
+      def find_callback(callback, &block)
+        select { |c| c == callback && (!block_given? || yield(c)) }.first
+      end
+
+      def replace_or_append_callback(callback)
+        if found_callback = find_callback(callback)
+          index = index(found_callback)
+          self[index] = callback
+        else
+          self << callback
+        end
+      end
+
+      private
+        def self.extract_options(*methods, &block)
+          methods.flatten!
+          options = methods.extract_options!
+          methods << block if block_given?
+          return methods, options
+        end
+
+        def extract_options(*methods, &block)
+          self.class.extract_options(*methods, &block)
+        end
+    end
+
+    class Callback
       attr_reader :kind, :method, :identifier, :options
 
       def initialize(kind, method, options = {})
@@ -99,22 +132,50 @@ def initialize(kind, method, options = {})
         @options    = options
       end
 
-      def call(object)
-        evaluate_method(method, object) if should_run_callback?(object)
+      def ==(other)
+        case other
+        when Callback
+          (self.identifier && self.identifier == other.identifier) || self.method == other.method
+        else
+          (self.identifier && self.identifier == other) || self.method == other
+        end
+      end
+
+      def eql?(other)
+        self == other
+      end
+
+      def dup
+        self.class.new(@kind, @method, @options.dup)
+      end
+
+      def call(object, &block)
+        evaluate_method(method, object, &block) if should_run_callback?(object)
+      rescue LocalJumpError
+        raise ArgumentError,
+          "Cannot yield from a Proc type filter. The Proc must take two " +
+          "arguments and execute #call on the second argument."
       end
 
       private
-        def evaluate_method(method, object)
+        def evaluate_method(method, object, &block)
           case method
             when Symbol
-              object.send(method)
+              object.send(method, &block)
             when String
               eval(method, object.instance_eval { binding })
             when Proc, Method
-              method.call(object)
+              case method.arity
+                when -1, 1
+                  method.call(object, &block)
+                when 2
+                  method.call(object, block)
+                else
+                  raise ArgumentError, 'Callback blocks must take one or two arguments.'
+              end
             else
               if method.respond_to?(kind)
-                method.send(kind, object)
+                method.send(kind, object, &block)
               else
                 raise ArgumentError,
                   "Callbacks must be a symbol denoting the method to call, a string to be evaluated, " +
@@ -143,17 +204,15 @@ def define_callbacks(*callbacks)
         callbacks.each do |callback|
           class_eval <<-"end_eval"
             def self.#{callback}(*methods, &block)
-              options = methods.extract_options!
-              methods << block if block_given?
-              callbacks = methods.map { |method| Callback.new(:#{callback}, method, options) }
-              (@#{callback}_callbacks ||= []).concat callbacks
+              callbacks = CallbackChain.build(:#{callback}, *methods, &block)
+              (@#{callback}_callbacks ||= CallbackChain.new).concat callbacks
             end
 
             def self.#{callback}_callback_chain
-              @#{callback}_callbacks ||= []
+              @#{callback}_callbacks ||= CallbackChain.new
 
               if superclass.respond_to?(:#{callback}_callback_chain)
-                superclass.#{callback}_callback_chain + @#{callback}_callbacks
+                CallbackChain.new(superclass.#{callback}_callback_chain + @#{callback}_callbacks)
               else
                 @#{callback}_callbacks
               end
@@ -208,7 +267,7 @@ def self.#{callback}_callback_chain
     #   pass
     #   stop
     def run_callbacks(kind, options = {}, &block)
-      Callback.run(self.class.send("#{kind}_callback_chain"), self, options, &block)
+      self.class.send("#{kind}_callback_chain").run(self, options, &block)
     end
   end
 end

activesupport/test/callbacks_test.rb

@@ -94,3 +94,24 @@ def test_save_conditional_person
     ], person.history
   end
 end
+
+class CallbackTest < Test::Unit::TestCase
+  def test_eql
+    callback = Callback.new(:before, :save, :identifier => :lifesaver)
+    assert callback.eql?(Callback.new(:before, :save, :identifier => :lifesaver))
+    assert callback.eql?(Callback.new(:before, :save))
+    assert callback.eql?(:lifesaver)
+    assert callback.eql?(:save)
+    assert !callback.eql?(Callback.new(:before, :destroy))
+    assert !callback.eql?(:destroy)
+  end
+
+  def test_dup
+    a = Callback.new(:before, :save)
+    assert_equal({}, a.options)
+    b = a.dup
+    b.options[:unless] = :pigs_fly
+    assert_equal({:unless => :pigs_fly}, b.options)
+    assert_equal({}, a.options)
+  end
+end