Further cleaning up new callbacks

5 files changed, 97 insertions, 117 deletions

diff --git a/actionpack/lib/action_controller/abstract/callbacks.rb b/actionpack/lib/action_controller/abstract/callbacks.rb
index dd4213e847..fd7e5ebfda 100644
--- a/actionpack/lib/action_controller/abstract/callbacks.rb
+++ b/actionpack/lib/action_controller/abstract/callbacks.rb
@@ -45,19 +45,19 @@ module AbstractController
         class_eval <<-RUBY_EVAL, __FILE__, __LINE__ + 1
           def #{filter}_filter(*names, &blk)
             _insert_callbacks(names, blk) do |name, options|
-              _set_callback(:process_action, :#{filter}, name, options)
+              set_callback(:process_action, :#{filter}, name, options)
             end
           end
 
           def prepend_#{filter}_filter(*names, &blk)
             _insert_callbacks(names, blk) do |name, options|
-              _set_callback(:process_action, :#{filter}, name, options.merge(:prepend => true))
+              set_callback(:process_action, :#{filter}, name, options.merge(:prepend => true))
             end
           end
 
           def skip_#{filter}_filter(*names, &blk)
             _insert_callbacks(names, blk) do |name, options|
-              _skip_callback(:process_action, :#{filter}, name, options)
+              skip_callback(:process_action, :#{filter}, name, options)
             end
           end
 
diff --git a/actionpack/test/abstract_controller/callbacks_test.rb b/actionpack/test/abstract_controller/callbacks_test.rb
index 826e76892e..32450257f0 100644
--- a/actionpack/test/abstract_controller/callbacks_test.rb
+++ b/actionpack/test/abstract_controller/callbacks_test.rb
@@ -8,7 +8,7 @@ module AbstractController
     end
     
     class Callback1 < ControllerWithCallbacks
-      _set_callback :process_action, :before, :first
+      set_callback :process_action, :before, :first
       
       def first
         @text = "Hello world"
diff --git a/activesupport/lib/active_support/new_callbacks.rb b/activesupport/lib/active_support/new_callbacks.rb
index 86cf5c01a8..6023fc1170 100644
--- a/activesupport/lib/active_support/new_callbacks.rb
+++ b/activesupport/lib/active_support/new_callbacks.rb
@@ -91,9 +91,8 @@ module ActiveSupport
       @@_callback_sequence = 0
       
       attr_accessor :filter, :kind, :name, :options, :per_key, :klass
-      def initialize(filter, kind, options, klass, name)
+      def initialize(filter, kind, options, klass)
         @kind, @klass = kind, klass
-        @name         = name
         
         normalize_options!(options)
 
@@ -131,9 +130,8 @@ module ActiveSupport
         @@_callback_sequence += 1
       end
       
-      def matches?(_kind, _name, _filter)
+      def matches?(_kind, _filter)
         @kind   == _kind &&
-        @name   == _name &&
         @filter == _filter
       end
 
@@ -310,40 +308,35 @@ module ActiveSupport
           RUBY_EVAL
           method_name
         else
-          kind, name = @kind, @name
+          kind = @kind
           @klass.send(:define_method, "#{method_name}_object") { filter }
 
-          if kind == :around
-            @klass.class_eval <<-RUBY_EVAL, __FILE__, __LINE__ + 1
-              def #{method_name}(&blk)
-                if :#{kind} == :around && #{method_name}_object.respond_to?(:filter)
-                  #{method_name}_object.send("filter", self, &blk)
-                # TODO: Deprecate this
-                elsif #{method_name}_object.respond_to?(:before) && #{method_name}_object.respond_to?(:after)
-                  should_continue = #{method_name}_object.before(self)
-                  yield if should_continue
-                  #{method_name}_object.after(self)
-                else
-                  #{method_name}_object.send("#{kind}_#{name}", self, &blk)
-                end
-              end
-            RUBY_EVAL
-          else
-            @klass.class_eval <<-RUBY_EVAL, __FILE__, __LINE__ + 1
-              def #{method_name}(&blk)
-                if #{method_name}_object.respond_to?(:#{kind})
-                  #{method_name}_object.#{kind}(self, &blk)
-                elsif #{method_name}_object.respond_to?(:filter)
-                  #{method_name}_object.send("filter", self, &blk)
-                else
-                  #{method_name}_object.send("#{kind}_#{name}", self, &blk)
-                end
-              end
-            RUBY_EVAL
-          end
+          _normalize_legacy_filter(kind, filter)
+
+          @klass.class_eval <<-RUBY_EVAL, __FILE__, __LINE__ + 1
+            def #{method_name}(&blk)
+              #{method_name}_object.send(:#{kind}, self, &blk)
+            end
+          RUBY_EVAL
+
           method_name
         end
       end
+
+      def _normalize_legacy_filter(kind, filter)
+        if !filter.respond_to?(kind) && filter.respond_to?(:filter)
+          filter.metaclass.class_eval(
+            "def #{kind}(context, &block) filter(context, &block) end",
+            __FILE__, __LINE__ - 1)
+        elsif filter.respond_to?(:before) && filter.respond_to?(:after) && kind == :around
+          def filter.around(context)
+            should_continue = before(context)
+            yield if should_continue
+            after(context)
+          end
+        end
+      end
+
     end
 
     # An Array with a compile method
@@ -385,22 +378,19 @@ module ActiveSupport
       # The _run_save_callbacks method can optionally take a key, which
       # will be used to compile an optimized callback method for each
       # key. See #define_callbacks for more information.
-      def _define_runner(symbol, callbacks, options)
+      def _define_runner(symbol, callbacks)
         body = callbacks.compile(nil, :terminator => send("_#{symbol}_terminator"))
 
-        body = <<-RUBY_EVAL
-          def _run_#{symbol}_callbacks(key = nil)
+        body, line = <<-RUBY_EVAL, __LINE__
+          def _run_#{symbol}_callbacks(key = nil, &blk)
             if key
-              key = key.hash.to_s.gsub(/-/, '_')
-              name = "_run__\#{self.class.name.split("::").last}__#{symbol}__\#{key}__callbacks"
+              name = "_run__\#{self.class.name.hash.abs}__#{symbol}__\#{key.hash.abs}__callbacks"
               
-              if respond_to?(name)
-                send(name) { yield if block_given? }
-              else
-                self.class._create_and_run_keyed_callback(
-                  self.class.name.split("::").last,
-                  :#{symbol}, key, self) { yield if block_given? }
+              unless respond_to?(name)
+                self.class._create_keyed_callback(name, :#{symbol}, self, &blk)
               end
+
+              send(name, &blk)
             else
               #{body}
             end
@@ -408,30 +398,22 @@ module ActiveSupport
         RUBY_EVAL
   
         undef_method "_run_#{symbol}_callbacks" if method_defined?("_run_#{symbol}_callbacks")
-        class_eval body, __FILE__, __LINE__
-        
-        before_name, around_name, after_name = 
-          options.values_at(:before, :after, :around)
+        class_eval body, __FILE__, line
       end
       
       # This is called the first time a callback is called with a particular
       # key. It creates a new callback method for the key, calculating
       # which callbacks can be omitted because of per_key conditions.
-      def _create_and_run_keyed_callback(klass, kind, key, obj, &blk)
+      def _create_keyed_callback(name, kind, obj, &blk)
         @_keyed_callbacks ||= {}
-        @_keyed_callbacks[[kind, key]] ||= begin
-          str = self.send("_#{kind}_callbacks").compile(key, :object => obj, :terminator => self.send("_#{kind}_terminator"))
+        @_keyed_callbacks[name] ||= begin
+          str = send("_#{kind}_callbacks").
+            compile(name, :object => obj, :terminator => send("_#{kind}_terminator"))
 
-          self.class_eval <<-RUBY_EVAL, __FILE__, __LINE__ + 1
-            def _run__#{klass.split("::").last}__#{kind}__#{key}__callbacks
-              #{str}
-            end
-          RUBY_EVAL
+          class_eval "def #{name}() #{str} end", __FILE__, __LINE__
                     
           true
         end
-                                  
-        obj.send("_run__#{klass.split("::").last}__#{kind}__#{key}__callbacks", &blk)
       end
       
       # Define callbacks.
@@ -473,32 +455,30 @@ module ActiveSupport
         callbacks = send("_#{name}_callbacks")
         yield callbacks, type, filters, options if block_given?
 
-        _define_runner(name, callbacks, options)
+        _define_runner(name, callbacks)
       end
 
-      def _set_callback(name, *filters, &block)
+      def set_callback(name, *filters, &block)
         _update_callbacks(name, filters, block) do |callbacks, type, filters, options|        
           filters.map! do |filter|
             # overrides parent class
-            callbacks.delete_if do |c|
-              c.matches?(type, name, filter)
-            end
-            Callback.new(filter, type, options.dup, self, name)
+            callbacks.delete_if {|c| c.matches?(type, filter) }
+            Callback.new(filter, type, options.dup, self)
           end
 
           options[:prepend] ? callbacks.unshift(*filters) : callbacks.push(*filters)
         end
       end
 
-      def _skip_callback(name, *filters, &block)
+      def skip_callback(name, *filters, &block)
         _update_callbacks(name, filters, block) do |callbacks, type, filters, options|
           filters.each do |filter|
             callbacks = send("_#{name}_callbacks=", callbacks.clone(self))
 
-            filter = callbacks.find {|c| c.matches?(type, name, filter) }
-            per_key = options[:per_key] || {}
+            filter = callbacks.find {|c| c.matches?(type, filter) }
+
             if filter && options.any?
-              filter.recompile!(options, per_key)
+              filter.recompile!(options, options[:per_key] || {})
             else
               callbacks.delete(filter)
             end
@@ -520,7 +500,7 @@ module ActiveSupport
               _update_callbacks(:#{symbol})
             end
             
-            self._set_callback(:#{symbol}, :before)
+            self.set_callback(:#{symbol}, :before)
           RUBY_EVAL
         end
       end
diff --git a/activesupport/test/new_callback_inheritance_test.rb b/activesupport/test/new_callback_inheritance_test.rb
index 9a1882b9d3..da0c19eaea 100644
--- a/activesupport/test/new_callback_inheritance_test.rb
+++ b/activesupport/test/new_callback_inheritance_test.rb
@@ -11,8 +11,8 @@ class GrandParent
   end
   
   define_callbacks :dispatch
-  _set_callback :dispatch, :before, :before1, :before2, :per_key => {:if => proc {|c| c.action_name == "index" || c.action_name == "update" }}
-  _set_callback :dispatch, :after, :after1, :after2, :per_key => {:if => proc {|c| c.action_name == "update" || c.action_name == "delete" }}  
+  set_callback :dispatch, :before, :before1, :before2, :per_key => {:if => proc {|c| c.action_name == "index" || c.action_name == "update" }}
+  set_callback :dispatch, :after, :after1, :after2, :per_key => {:if => proc {|c| c.action_name == "update" || c.action_name == "delete" }}  
   
   def before1
     @log << "before1"
@@ -39,12 +39,12 @@ class GrandParent
 end
 
 class Parent < GrandParent
-  _skip_callback :dispatch, :before, :before2, :per_key => {:unless => proc {|c| c.action_name == "update" }}
-  _skip_callback :dispatch, :after, :after2, :per_key => {:unless => proc {|c| c.action_name == "delete" }}
+  skip_callback :dispatch, :before, :before2, :per_key => {:unless => proc {|c| c.action_name == "update" }}
+  skip_callback :dispatch, :after, :after2, :per_key => {:unless => proc {|c| c.action_name == "delete" }}
 end
 
 class Child < GrandParent
-  _skip_callback :dispatch, :before, :before2, :per_key => {:unless => proc {|c| c.action_name == "update" }}, :if => :state_open?
+  skip_callback :dispatch, :before, :before2, :per_key => {:unless => proc {|c| c.action_name == "update" }}, :if => :state_open?
   
   def state_open?
     @state == :open
diff --git a/activesupport/test/new_callbacks_test.rb b/activesupport/test/new_callbacks_test.rb
index de501e9f7b..7e092b5f63 100644
--- a/activesupport/test/new_callbacks_test.rb
+++ b/activesupport/test/new_callbacks_test.rb
@@ -10,11 +10,11 @@ module NewCallbacksTest
     define_callbacks :save
   
     def self.before_save(*filters, &blk)
-      _set_callback(:save, :before, *filters, &blk)
+      set_callback(:save, :before, *filters, &blk)
     end
   
     def self.after_save(*filters, &blk)
-      _set_callback(:save, :after, *filters, &blk)
+      set_callback(:save, :after, *filters, &blk)
     end
 
     class << self
@@ -37,7 +37,7 @@ module NewCallbacksTest
       def callback_object(callback_method)
         klass = Class.new
         klass.send(:define_method, callback_method) do |model|
-          model.history << [callback_method, :object]
+          model.history << [:"#{callback_method}_save", :object]
         end
         klass.new
       end
@@ -54,7 +54,7 @@ module NewCallbacksTest
       send(callback_method, callback_symbol(callback_method_sym))
       send(callback_method, callback_string(callback_method_sym))
       send(callback_method, callback_proc(callback_method_sym))
-      send(callback_method, callback_object(callback_method_sym))
+      send(callback_method, callback_object(callback_method_sym.to_s.gsub(/_save/, '')))
       send(callback_method) { |model| model.history << [callback_method_sym, :block] }
     end
 
@@ -64,10 +64,10 @@ module NewCallbacksTest
   end
 
   class PersonSkipper < Person
-    _skip_callback :save, :before, :before_save_method, :if => :yes
-    _skip_callback :save, :after, :before_save_method, :unless => :yes
-    _skip_callback :save, :after, :before_save_method, :if => :no
-    _skip_callback :save, :before, :before_save_method, :unless => :no
+    skip_callback :save, :before, :before_save_method, :if => :yes
+    skip_callback :save, :after, :before_save_method, :unless => :yes
+    skip_callback :save, :after, :before_save_method, :if => :no
+    skip_callback :save, :before, :before_save_method, :unless => :no
     def yes; true; end
     def no; false; end
   end
@@ -77,8 +77,8 @@ module NewCallbacksTest
 
     define_callbacks :dispatch
     
-    _set_callback :dispatch, :before, :log, :per_key => {:unless => proc {|c| c.action_name == :index || c.action_name == :show }}
-    _set_callback :dispatch, :after, :log2
+    set_callback :dispatch, :before, :log, :per_key => {:unless => proc {|c| c.action_name == :index || c.action_name == :show }}
+    set_callback :dispatch, :after, :log2
   
     attr_reader :action_name, :logger
     def initialize(action_name)
@@ -102,8 +102,8 @@ module NewCallbacksTest
   end
 
   class Child < ParentController
-    _skip_callback :dispatch, :before, :log, :per_key => {:if => proc {|c| c.action_name == :update} }
-    _skip_callback :dispatch, :after, :log2
+    skip_callback :dispatch, :before, :log, :per_key => {:if => proc {|c| c.action_name == :update} }
+    skip_callback :dispatch, :after, :log2
   end
 
   class OneTimeCompile < Record
@@ -188,19 +188,19 @@ module NewCallbacksTest
   class AroundPerson < MySuper
     attr_reader :history
   
-    _set_callback :save, :before, :nope,           :if =>     :no
-    _set_callback :save, :before, :nope,           :unless => :yes
-    _set_callback :save, :after,  :tweedle
-    _set_callback :save, :before, "tweedle_dee"
-    _set_callback :save, :before, proc {|m| m.history << "yup" }
-    _set_callback :save, :before, :nope,           :if =>     proc { false }
-    _set_callback :save, :before, :nope,           :unless => proc { true }
-    _set_callback :save, :before, :yup,            :if =>     proc { true }
-    _set_callback :save, :before, :yup,            :unless => proc { false }
-    _set_callback :save, :around, :tweedle_dum
-    _set_callback :save, :around, :w0tyes,         :if =>     :yes
-    _set_callback :save, :around, :w0tno,          :if =>     :no
-    _set_callback :save, :around, :tweedle_deedle
+    set_callback :save, :before, :nope,           :if =>     :no
+    set_callback :save, :before, :nope,           :unless => :yes
+    set_callback :save, :after,  :tweedle
+    set_callback :save, :before, "tweedle_dee"
+    set_callback :save, :before, proc {|m| m.history << "yup" }
+    set_callback :save, :before, :nope,           :if =>     proc { false }
+    set_callback :save, :before, :nope,           :unless => proc { true }
+    set_callback :save, :before, :yup,            :if =>     proc { true }
+    set_callback :save, :before, :yup,            :unless => proc { false }
+    set_callback :save, :around, :tweedle_dum
+    set_callback :save, :around, :w0tyes,         :if =>     :yes
+    set_callback :save, :around, :w0tno,          :if =>     :no
+    set_callback :save, :around, :tweedle_deedle
   
     def no; false; end
     def yes; true; end
@@ -260,7 +260,7 @@ module NewCallbacksTest
     define_callbacks :save
     attr_reader :stuff
     
-    _set_callback :save, :before, :omg, :per_key => {:if => :yes}
+    set_callback :save, :before, :omg, :per_key => {:if => :yes}
     
     def yes() true end
       
@@ -354,15 +354,15 @@ module NewCallbacksTest
   
     define_callbacks :save, "result == :halt"
   
-    _set_callback :save, :before, :first
-    _set_callback :save, :before, :second
-    _set_callback :save, :around, :around_it  
-    _set_callback :save, :before, :third
-    _set_callback :save, :after, :first
-    _set_callback :save, :around, :around_it
-    _set_callback :save, :after, :second
-    _set_callback :save, :around, :around_it
-    _set_callback :save, :after, :third
+    set_callback :save, :before, :first
+    set_callback :save, :before, :second
+    set_callback :save, :around, :around_it  
+    set_callback :save, :before, :third
+    set_callback :save, :after, :first
+    set_callback :save, :around, :around_it
+    set_callback :save, :after, :second
+    set_callback :save, :around, :around_it
+    set_callback :save, :after, :third
 
   
     attr_reader :history, :saved
@@ -397,11 +397,11 @@ module NewCallbacksTest
   end
 
   class CallbackObject
-    def before_save(caller)
+    def before(caller)
       caller.record << "before"
     end
     
-    def around_save(caller)
+    def around(caller)
       caller.record << "around before"
       yield
       caller.record << "around after"
@@ -412,7 +412,7 @@ module NewCallbacksTest
     include ActiveSupport::NewCallbacks
     
     define_callbacks :save
-    _set_callback :save, :before, CallbackObject.new
+    set_callback :save, :before, CallbackObject.new
     
     attr_accessor :record
     def initialize
@@ -430,7 +430,7 @@ module NewCallbacksTest
     include ActiveSupport::NewCallbacks
     
     define_callbacks :save
-    _set_callback :save, :around, CallbackObject.new
+    set_callback :save, :around, CallbackObject.new
     
     attr_accessor :record
     def initialize