AS::Callbacks: rip out per_key option.

activesupport/CHANGELOG.md

@@ -1,5 +1,7 @@
 ## Rails 4.0.0 (unreleased) ##
 
+*    AS::Callbacks: `:per_key` option is no longer supported
+
 *    `AS::Callbacks#define_callbacks`: add `:skip_after_callbacks_if_terminated` option.
 
 *    Add html_escape_once to ERB::Util, and delegate escape_once tag helper to it. *Carlos Antonio da Silva*

activesupport/lib/active_support/callbacks.rb

@@ -95,6 +95,7 @@ module ActiveSupport
 
       def initialize(chain, filter, kind, options, klass)
         @chain, @kind, @klass = chain, kind, klass
+        deprecate_per_key_option(options)
         normalize_options!(options)
 
         @raw_filter, @options = filter, options
@@ -103,6 +104,12 @@ module ActiveSupport
         @callback_id          = next_id
       end
 
+      def deprecate_per_key_option(options)
+        if options[:per_key]
+          raise NotImplementedError, ":per_key option is no longer supported. Use generic :if and :unless options instead."
+        end
+      end
+
       def clone(chain, klass)
         obj                  = super()
         obj.chain            = chain
@@ -116,11 +123,6 @@ module ActiveSupport
       def normalize_options!(options)
         options[:if] = Array(options[:if])
         options[:unless] = Array(options[:unless])
-
-        options[:per_key] ||= {}
-
-        options[:if] += Array(options[:per_key][:if])
-        options[:unless] += Array(options[:per_key][:unless])
       end
 
       def name
@@ -140,9 +142,9 @@ module ActiveSupport
         filter_options[:unless].concat(Array(new_options[:if])) if new_options.key?(:if)
       end
 
-      def recompile!(_options, _per_key)
+      def recompile!(_options)
+        deprecate_per_key_option(_options)
         _update_filter(self.options, _options)
-        _update_filter(self.options, _per_key)
 
         @callback_id      = next_id
         @filter           = _compile_filter(@raw_filter)
@@ -352,9 +354,9 @@ module ActiveSupport
 
     module ClassMethods
 
-      # This method calls the callback method for the given key.
-      # If this called first time it creates a new callback method for the key, 
-      # calculating which callbacks can be omitted because of per_key conditions.
+      # This method runs callback chain for the given key.
+      # If this called first time it creates a new callback method for the key.
+      # This generated method plays caching role.
       #
       def __run_callbacks(key, kind, object, &blk) #:nodoc:
         name = __callback_runner_name(kind)
@@ -427,29 +429,6 @@ module ActiveSupport
       #   will be called only when it returns a false value.
       # * <tt>:prepend</tt> - If true, the callback will be prepended to the existing
       #   chain rather than appended.
-      # * <tt>:per_key</tt> - A hash with <tt>:if</tt> and <tt>:unless</tt> options;
-      #   see "Per-key conditions" below.
-      #
-      # ===== Per-key conditions
-      #
-      # When creating or skipping callbacks, you can specify conditions that
-      # are always the same for a given key. For instance, in Action Pack,
-      # we convert :only and :except conditions into per-key conditions.
-      #
-      #   before_filter :authenticate, :except => "index"
-      #
-      # becomes
-      #
-      #   set_callback :process_action, :before, :authenticate, :per_key => {:unless => proc {|c| c.action_name == "index"}}
-      #
-      # Per-key conditions are evaluated only once per use of a given key.
-      # In the case of the above example, you would do:
-      #
-      #   run_callbacks(:process_action, action_name) { ... dispatch stuff ... }
-      #
-      # In that case, each action_name would get its own compiled callback
-      # method that took into consideration the per_key conditions. This
-      # is a speed improvement for ActionPack.
       #
       def set_callback(name, *filter_list, &block)
         mapped = nil
@@ -484,7 +463,7 @@ module ActiveSupport
             if filter && options.any?
               new_filter = filter.clone(chain, self)
               chain.insert(chain.index(filter), new_filter)
-              new_filter.recompile!(options, options[:per_key] || {})
+              new_filter.recompile!(options)
             end
 
             chain.delete(filter)

activesupport/test/callback_inheritance_test.rb

@@ -9,8 +9,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, :if => proc {|c| c.action_name == "index" || c.action_name == "update" }
+  set_callback :dispatch, :after, :after1, :after2, :if => proc {|c| c.action_name == "update" || c.action_name == "delete" }
 
   def before1
     @log << "before1"
@@ -37,12 +37,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, :unless => proc {|c| c.action_name == "update" }
+  skip_callback :dispatch, :after, :after2, :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, :unless => proc {|c| c.action_name == "update" }, :if => :state_open?
 
   def state_open?
     @state == :open
@@ -112,15 +112,15 @@ class BasicCallbacksTest < ActiveSupport::TestCase
     @unknown  = GrandParent.new("unknown").dispatch
   end
 
-  def test_basic_per_key1
+  def test_basic_conditional_callback1
     assert_equal %w(before1 before2 index), @index.log
   end
 
-  def test_basic_per_key2
+  def test_basic_conditional_callback2
     assert_equal %w(before1 before2 update after2 after1), @update.log
   end
 
-  def test_basic_per_key3
+  def test_basic_conditional_callback3
     assert_equal %w(delete after2 after1), @delete.log
   end
 end

activesupport/test/callbacks_test.rb

@@ -95,7 +95,7 @@ module CallbacksTest
 
     define_callbacks :dispatch
 
-    set_callback :dispatch, :before, :log, :per_key => {:unless => proc {|c| c.action_name == :index || c.action_name == :show }}
+    set_callback :dispatch, :before, :log, :unless => proc {|c| c.action_name == :index || c.action_name == :show }
     set_callback :dispatch, :after, :log2
 
     attr_reader :action_name, :logger
@@ -120,7 +120,7 @@ module CallbacksTest
   end
 
   class Child < ParentController
-    skip_callback :dispatch, :before, :log, :per_key => {:if => proc {|c| c.action_name == :update} }
+    skip_callback :dispatch, :before, :log, :if => proc {|c| c.action_name == :update} 
     skip_callback :dispatch, :after, :log2
   end
 
@@ -131,10 +131,10 @@ module CallbacksTest
       super
     end
 
-    before_save Proc.new {|r| r.history << [:before_save, :starts_true, :if] }, :per_key => {:if => :starts_true}
-    before_save Proc.new {|r| r.history << [:before_save, :starts_false, :if] }, :per_key => {:if => :starts_false}
-    before_save Proc.new {|r| r.history << [:before_save, :starts_true, :unless] }, :per_key => {:unless => :starts_true}
-    before_save Proc.new {|r| r.history << [:before_save, :starts_false, :unless] }, :per_key => {:unless => :starts_false}
+    before_save Proc.new {|r| r.history << [:before_save, :starts_true, :if] }, :if => :starts_true
+    before_save Proc.new {|r| r.history << [:before_save, :starts_false, :if] }, :if => :starts_false
+    before_save Proc.new {|r| r.history << [:before_save, :starts_true, :unless] }, :unless => :starts_true
+    before_save Proc.new {|r| r.history << [:before_save, :starts_false, :unless] }, :unless => :starts_false
 
     def starts_true
       if @@starts_true
@@ -329,7 +329,7 @@ module CallbacksTest
     define_callbacks :save
     attr_reader :stuff
 
-    set_callback :save, :before, :action, :per_key => {:if => :yes}
+    set_callback :save, :before, :action, :if => :yes
 
     def yes() true end
 
@@ -700,5 +700,21 @@ module CallbacksTest
       assert_equal [1, 2, 3], model.recorder
     end
   end
+
+  class PerKeyOptionDeprecationTest < ActiveSupport::TestCase
+
+    def test_per_key_option_deprecaton
+      assert_raise NotImplementedError do
+        Phone.class_eval do
+          set_callback :save, :before, :before_save1, :per_key => {:if => "true"}
+        end
+      end
+      assert_raise NotImplementedError do
+        Phone.class_eval do
+          skip_callback :save, :before, :before_save1, :per_key => {:if => "true"}
+        end
+      end
+    end
+  end
  
 end