Merge commit 'josevalim/inheritance'

10 files changed, 125 insertions, 68 deletions

diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb
index ebb717812d..730d9d8df7 100755
--- a/activerecord/test/cases/base_test.rb
+++ b/activerecord/test/cases/base_test.rb
@@ -201,7 +201,7 @@ class BasicsTest < ActiveRecord::TestCase
     topic = Topic.new(:title => "New Topic")
     assert topic.save!
 
-    reply = Reply.new
+    reply = WrongReply.new
     assert_raise(ActiveRecord::RecordInvalid) { reply.save! }
   end
 
@@ -959,6 +959,7 @@ class BasicsTest < ActiveRecord::TestCase
   end
 
   def test_update_attributes!
+    Reply.validates_presence_of(:title)
     reply = Reply.find(2)
     assert_equal "The Second Topic of the day", reply.title
     assert_equal "Have a nice day", reply.content
@@ -974,6 +975,8 @@ class BasicsTest < ActiveRecord::TestCase
     assert_equal "Have a nice day", reply.content
 
     assert_raise(ActiveRecord::RecordInvalid) { reply.update_attributes!(:title => nil, :content => "Have a nice evening") }
+  ensure
+    Reply.reset_callbacks(:validate)
   end
 
   def test_mass_assignment_should_raise_exception_if_accessible_and_protected_attribute_writers_are_both_used
diff --git a/activerecord/test/cases/validations/association_validation_test.rb b/activerecord/test/cases/validations/association_validation_test.rb
index 278a7a6a06..5ed997356b 100644
--- a/activerecord/test/cases/validations/association_validation_test.rb
+++ b/activerecord/test/cases/validations/association_validation_test.rb
@@ -10,7 +10,7 @@ require 'models/interest'
 class AssociationValidationTest < ActiveRecord::TestCase
   fixtures :topics, :owners
 
-  repair_validations(Topic)
+  repair_validations(Topic, Reply)
 
   def test_validates_size_of_association
     repair_validations(Owner) do
@@ -40,7 +40,8 @@ class AssociationValidationTest < ActiveRecord::TestCase
   end
 
   def test_validates_associated_many
-    Topic.validates_associated( :replies )
+    Topic.validates_associated(:replies)
+    Reply.validates_presence_of(:content)
     t = Topic.create("title" => "uhohuhoh", "content" => "whatever")
     t.replies << [r = Reply.new("title" => "A reply"), r2 = Reply.new("title" => "Another reply", "content" => "non-empty"), r3 = Reply.new("title" => "Yet another reply"), r4 = Reply.new("title" => "The last reply", "content" => "non-empty")]
     assert !t.valid?
diff --git a/activerecord/test/cases/validations/i18n_validation_test.rb b/activerecord/test/cases/validations/i18n_validation_test.rb
index 532de67d99..f017f24048 100644
--- a/activerecord/test/cases/validations/i18n_validation_test.rb
+++ b/activerecord/test/cases/validations/i18n_validation_test.rb
@@ -3,8 +3,9 @@ require 'models/topic'
 require 'models/reply'
 
 class I18nValidationTest < ActiveRecord::TestCase
+  repair_validations(Topic, Reply)
   def setup
-    Topic.reset_callbacks(:validate)
+    Reply.validates_presence_of(:title)
     @topic = Topic.new
     @old_load_path, @old_backend = I18n.load_path, I18n.backend
     I18n.load_path.clear
@@ -13,7 +14,6 @@ class I18nValidationTest < ActiveRecord::TestCase
   end
 
   def teardown
-    Topic.reset_callbacks(:validate)
     I18n.load_path.replace @old_load_path
     I18n.backend = @old_backend
   end
diff --git a/activerecord/test/cases/validations/uniqueness_validation_test.rb b/activerecord/test/cases/validations/uniqueness_validation_test.rb
index db633339f3..8f84841fe6 100644
--- a/activerecord/test/cases/validations/uniqueness_validation_test.rb
+++ b/activerecord/test/cases/validations/uniqueness_validation_test.rb
@@ -5,7 +5,6 @@ require 'models/reply'
 require 'models/warehouse_thing'
 require 'models/guid'
 require 'models/event'
-require 'models/developer'
 
 # The following methods in Topic are used in test_conditional_validation_*
 class Topic
@@ -276,14 +275,4 @@ class UniquenessValidationTest < ActiveRecord::TestCase
     assert w6.errors[:city].any?, "Should have errors for city"
     assert_equal ["has already been taken"], w6.errors[:city], "Should have uniqueness message for city"
   end
-
-  def test_validates_uniqueness_of_with_custom_message_using_quotes
-    repair_validations(Developer) do
-      Developer.validates_uniqueness_of :name, :message=> "This string contains 'single' and \"double\" quotes"
-      d = Developer.new
-      d.name = "David"
-      assert !d.valid?
-      assert_equal ["This string contains 'single' and \"double\" quotes"], d.errors[:name]
-    end
-  end
 end
diff --git a/activerecord/test/cases/validations_repair_helper.rb b/activerecord/test/cases/validations_repair_helper.rb
index e04738d209..11912ca1cc 100644
--- a/activerecord/test/cases/validations_repair_helper.rb
+++ b/activerecord/test/cases/validations_repair_helper.rb
@@ -4,31 +4,19 @@ module ActiveRecord
 
     module ClassMethods
       def repair_validations(*model_classes)
-        setup do
-          @_stored_callbacks = {}
-          model_classes.each do |k|
-            @_stored_callbacks[k] = k._validate_callbacks.dup
-          end
-        end
         teardown do
           model_classes.each do |k|
-            k._validate_callbacks = @_stored_callbacks[k]
-            k.__update_callbacks(:validate)
+            k.reset_callbacks(:validate)
           end
         end
       end
     end
 
-    def repair_validations(*model_classes, &block)
-      @__stored_callbacks = {}
-      model_classes.each do |k|
-        @__stored_callbacks[k] = k._validate_callbacks.dup
-      end
-      return block.call
+    def repair_validations(*model_classes)
+      yield
     ensure
       model_classes.each do |k|
-        k._validate_callbacks = @__stored_callbacks[k]
-        k.__update_callbacks(:validate)
+        k.reset_callbacks(:validate)
       end
     end
   end
diff --git a/activerecord/test/cases/validations_test.rb b/activerecord/test/cases/validations_test.rb
index 7462d944e0..3a1d5ae212 100644
--- a/activerecord/test/cases/validations_test.rb
+++ b/activerecord/test/cases/validations_test.rb
@@ -42,7 +42,7 @@ class ValidationsTest < ActiveRecord::TestCase
   repair_validations(Topic)
 
   def test_error_on_create
-    r = Reply.new
+    r = WrongReply.new
     r.title = "Wrong Create"
     assert !r.valid?
     assert r.errors[:title].any?, "A reply with a bad title should mark that attribute as invalid"
@@ -50,7 +50,7 @@ class ValidationsTest < ActiveRecord::TestCase
   end
 
   def test_error_on_update
-    r = Reply.new
+    r = WrongReply.new
     r.title = "Bad"
     r.content = "Good"
     assert r.save, "First save should be successful"
@@ -63,11 +63,11 @@ class ValidationsTest < ActiveRecord::TestCase
   end
 
   def test_invalid_record_exception
-    assert_raise(ActiveRecord::RecordInvalid) { Reply.create! }
-    assert_raise(ActiveRecord::RecordInvalid) { Reply.new.save! }
+    assert_raise(ActiveRecord::RecordInvalid) { WrongReply.create! }
+    assert_raise(ActiveRecord::RecordInvalid) { WrongReply.new.save! }
 
     begin
-      r = Reply.new
+      r = WrongReply.new
       r.save!
       flunk
     rescue ActiveRecord::RecordInvalid => invalid
@@ -77,13 +77,13 @@ class ValidationsTest < ActiveRecord::TestCase
 
   def test_exception_on_create_bang_many
     assert_raise(ActiveRecord::RecordInvalid) do
-      Reply.create!([ { "title" => "OK" }, { "title" => "Wrong Create" }])
+      WrongReply.create!([ { "title" => "OK" }, { "title" => "Wrong Create" }])
     end
   end
 
   def test_exception_on_create_bang_with_block
     assert_raise(ActiveRecord::RecordInvalid) do
-      Reply.create!({ "title" => "OK" }) do |r|
+      WrongReply.create!({ "title" => "OK" }) do |r|
         r.content = nil
       end
     end
@@ -91,15 +91,15 @@ class ValidationsTest < ActiveRecord::TestCase
 
   def test_exception_on_create_bang_many_with_block
     assert_raise(ActiveRecord::RecordInvalid) do
-      Reply.create!([{ "title" => "OK" }, { "title" => "Wrong Create" }]) do |r|
+      WrongReply.create!([{ "title" => "OK" }, { "title" => "Wrong Create" }]) do |r|
         r.content = nil
       end
     end
   end
 
   def test_scoped_create_without_attributes
-    Reply.send(:with_scope, :create => {}) do
-      assert_raise(ActiveRecord::RecordInvalid) { Reply.create! }
+    WrongReply.send(:with_scope, :create => {}) do
+      assert_raise(ActiveRecord::RecordInvalid) { WrongReply.create! }
     end
   end
 
@@ -122,15 +122,15 @@ class ValidationsTest < ActiveRecord::TestCase
   end
 
   def test_create_without_validation
-    reply = Reply.new
+    reply = WrongReply.new
     assert !reply.save
     assert reply.save(false)
   end
 
   def test_create_without_validation_bang
-    count = Reply.count
-    assert_nothing_raised { Reply.new.save_without_validation! }
-    assert count+1, Reply.count
+    count = WrongReply.count
+    assert_nothing_raised { WrongReply.new.save_without_validation! }
+    assert count+1, WrongReply.count
   end
 
   def test_validates_acceptance_of_with_non_existant_table
diff --git a/activerecord/test/models/reply.rb b/activerecord/test/models/reply.rb
index ba5a1d1d01..f1ba45b528 100644
--- a/activerecord/test/models/reply.rb
+++ b/activerecord/test/models/reply.rb
@@ -7,11 +7,13 @@ class Reply < Topic
   belongs_to :topic_with_primary_key, :class_name => "Topic", :primary_key => "title", :foreign_key => "parent_title", :counter_cache => "replies_count"
   has_many :replies, :class_name => "SillyReply", :dependent => :destroy, :foreign_key => "parent_id"
 
+  attr_accessible :title, :author_name, :author_email_address, :written_on, :content, :last_read, :parent_title
+end
+
+class WrongReply < Reply
   validate :errors_on_empty_content
   validate :title_is_wrong_create, :on => :create
 
-  attr_accessible :title, :author_name, :author_email_address, :written_on, :content, :last_read, :parent_title
-
   validate :check_empty_title
   validate :check_content_mismatch, :on => :create
   validate :check_wrong_update, :on => :update
diff --git a/activesupport/lib/active_support/callbacks.rb b/activesupport/lib/active_support/callbacks.rb
index 05bc453dbf..47e90df200 100644
--- a/activesupport/lib/active_support/callbacks.rb
+++ b/activesupport/lib/active_support/callbacks.rb
@@ -367,12 +367,6 @@ module ActiveSupport
         method << "halted ? false : (block_given? ? value : true)"
         method.compact.join("\n")
       end
-
-      def clone(klass)
-        chain = CallbackChain.new(@name, @config.dup)
-        callbacks = map { |c| c.clone(chain, klass) }
-        chain.push(*callbacks)
-      end
     end
 
     module ClassMethods
@@ -389,10 +383,16 @@ module ActiveSupport
       # key. See #define_callbacks for more information.
       #
       def __define_runner(symbol) #:nodoc:
+        send("_update_#{symbol}_superclass_callbacks")
         body = send("_#{symbol}_callbacks").compile(nil)
 
         body, line = <<-RUBY_EVAL, __LINE__
           def _run_#{symbol}_callbacks(key = nil, &blk)
+            if self.class.send("_update_#{symbol}_superclass_callbacks")
+              self.class.__define_runner(#{symbol.inspect})
+              return _run_#{symbol}_callbacks(key, &blk)
+            end
+
             if key
               name = "_run__\#{self.class.name.hash.abs}__#{symbol}__\#{key.hash.abs}__callbacks"
 
@@ -431,6 +431,8 @@ module ActiveSupport
       # CallbackChain.
       #
       def __update_callbacks(name, filters = [], block = nil) #:nodoc:
+        send("_update_#{name}_superclass_callbacks")
+
         type = [:before, :after, :around].include?(filters.first) ? filters.shift : :before
         options = filters.last.is_a?(Hash) ? filters.pop : {}
         filters.unshift(block) if block
@@ -470,7 +472,8 @@ module ActiveSupport
       def set_callback(name, *filter_list, &block)
         __update_callbacks(name, filter_list, block) do |chain, type, filters, options|
           filters.map! do |filter|
-            chain.delete_if {|c| c.matches?(type, filter) }
+            removed = chain.delete_if {|c| c.matches?(type, filter) } 
+            send("_removed_#{name}_callbacks").push(*removed)
             Callback.new(chain, filter, type, options.dup, self)
           end
 
@@ -482,16 +485,17 @@ module ActiveSupport
       #
       def skip_callback(name, *filter_list, &block)
         __update_callbacks(name, filter_list, block) do |chain, type, filters, options|
-          chain = send("_#{name}_callbacks=", chain.clone(self))
-
           filters.each do |filter|
             filter = chain.find {|c| c.matches?(type, filter) }
 
             if filter && options.any?
-              filter.recompile!(options, options[:per_key] || {})
-            else
-              chain.delete(filter)
+              new_filter = filter.clone(chain, self)
+              chain.insert(chain.index(filter), new_filter)
+              new_filter.recompile!(options, options[:per_key] || {})
             end
+
+            chain.delete(filter)
+            send("_removed_#{name}_callbacks") << filter
           end
         end
       end
@@ -499,7 +503,9 @@ module ActiveSupport
       # Reset callbacks for a given type.
       #
       def reset_callbacks(symbol)
-        send("_#{symbol}_callbacks").clear
+        callbacks = send("_#{symbol}_callbacks")
+        callbacks.clear
+        send("_removed_#{symbol}_callbacks").concat(callbacks)
         __define_runner(symbol)
       end
 
@@ -546,14 +552,46 @@ module ActiveSupport
       #
       # Defaults to :kind.
       #
-      def define_callbacks(*symbols)
-        config = symbols.last.is_a?(Hash) ? symbols.pop : {}
-        symbols.each do |symbol|
-          extlib_inheritable_accessor("_#{symbol}_callbacks") do
-            CallbackChain.new(symbol, config)
+      def define_callbacks(*callbacks)
+        config = callbacks.last.is_a?(Hash) ? callbacks.pop : {}
+        callbacks.each do |callback|
+          extlib_inheritable_reader("_#{callback}_callbacks") do
+            CallbackChain.new(callback, config)
+          end
+
+          extlib_inheritable_reader("_removed_#{callback}_callbacks") do
+            []
           end
 
-          __define_runner(symbol)
+          class_eval <<-METHOD, __FILE__, __LINE__ + 1
+            def self._#{callback}_superclass_callbacks
+              if superclass.respond_to?(:_#{callback}_callbacks)
+                superclass._#{callback}_callbacks + superclass._#{callback}_superclass_callbacks
+              else
+                []
+              end
+            end
+
+            def self._update_#{callback}_superclass_callbacks
+              changed, index = false, 0
+
+              callbacks  = (_#{callback}_superclass_callbacks -
+                _#{callback}_callbacks) - _removed_#{callback}_callbacks
+
+              callbacks.each do |callback|
+                if new_index = _#{callback}_callbacks.index(callback)
+                  index = new_index + 1
+                else
+                  changed = true
+                  _#{callback}_callbacks.insert(index, callback)
+                  index = index + 1
+                end
+              end
+              changed
+            end
+          METHOD
+
+          __define_runner(callback)
         end
       end
     end
diff --git a/activesupport/lib/active_support/core_ext/class/inheritable_attributes.rb b/activesupport/lib/active_support/core_ext/class/inheritable_attributes.rb
index e4d22516c1..2b8e2b544f 100644
--- a/activesupport/lib/active_support/core_ext/class/inheritable_attributes.rb
+++ b/activesupport/lib/active_support/core_ext/class/inheritable_attributes.rb
@@ -159,7 +159,7 @@ class Class
   #   (error out or do the same as other methods above) instead of silently
   #   moving on). In particular, this makes the return value of this function
   #   less useful.
-  def extlib_inheritable_reader(*ivars)
+  def extlib_inheritable_reader(*ivars, &block)
     options = ivars.extract_options!
 
     ivars.each do |ivar|
@@ -178,6 +178,7 @@ class Class
           end
         RUBY
       end
+      instance_variable_set(:"@#{ivar}", yield) if block_given?
     end
   end
 
diff --git a/activesupport/test/callback_inheritance_test.rb b/activesupport/test/callback_inheritance_test.rb
index 18721eab19..2e978f01be 100644
--- a/activesupport/test/callback_inheritance_test.rb
+++ b/activesupport/test/callback_inheritance_test.rb
@@ -56,6 +56,31 @@ class Child < GrandParent
   end
 end
 
+class EmptyParent
+  include ActiveSupport::Callbacks
+
+  def performed?
+    @performed ||= false
+  end
+
+  define_callbacks :dispatch
+
+  def perform!
+    @performed = true
+  end
+
+  def dispatch
+    _run_dispatch_callbacks
+    self
+  end
+end
+
+class EmptyChild < EmptyParent
+  set_callback :dispatch, :before, :do_nothing
+
+  def do_nothing
+  end
+end
 
 class BasicCallbacksTest < Test::Unit::TestCase
   def setup
@@ -113,3 +138,13 @@ class InheritedCallbacksTest2 < Test::Unit::TestCase
     assert_equal %w(before1 before2 update after2 after1), @update2.log
   end
 end
+
+class DynamicInheritedCallbacks < Test::Unit::TestCase
+  def test_callbacks_looks_to_the_superclass_before_running
+    child = EmptyChild.new.dispatch
+    assert !child.performed?
+    EmptyParent.set_callback :dispatch, :before, :perform!
+    child = EmptyChild.new.dispatch
+    assert child.performed?
+  end
+end