Make sure valid? preceives the context as in ActiveModel API (ht: Carlos Antonio)

4 files changed, 45 insertions, 26 deletions

diff --git a/activerecord/lib/active_record/attribute_methods/primary_key.rb b/activerecord/lib/active_record/attribute_methods/primary_key.rb
index 411330dda2..82d94b848a 100644
--- a/activerecord/lib/active_record/attribute_methods/primary_key.rb
+++ b/activerecord/lib/active_record/attribute_methods/primary_key.rb
@@ -6,7 +6,7 @@ module ActiveRecord
       # Returns this record's primary key value wrapped in an Array
       # or nil if the record is a new_record?
       def to_key
-        new_record? ? nil : [ send(self.class.primary_key) ]
+        new_record? ? nil : [ id ]
       end
 
       module ClassMethods
diff --git a/activerecord/lib/active_record/validations.rb b/activerecord/lib/active_record/validations.rb
index 55c4236874..58391168a9 100644
--- a/activerecord/lib/active_record/validations.rb
+++ b/activerecord/lib/active_record/validations.rb
@@ -36,8 +36,8 @@ module ActiveRecord
 
     # The validation process on save can be skipped by passing false. The regular Base#save method is
     # replaced with this when the validations module is mixed in, which it is by default.
-    def save(options=nil)
-      return super if valid?(options)
+    def save(options={})
+      return super if perform_validations(options)
       false
     end
 
@@ -47,38 +47,35 @@ module ActiveRecord
 
     # Attempts to save the record just like Base#save but will raise a RecordInvalid exception instead of returning false
     # if the record is not valid.
-    def save!(options = nil)
-      return super if valid?(options)
+    def save!(options={})
+      return super if perform_validations(options)
       raise RecordInvalid.new(self)
     end
 
     # Runs all the specified validations and returns true if no errors were added otherwise false.
-    def valid?(options = nil)
-      perform_validation = case options
-        when NilClass
-          true
-        when Hash
-          options[:validate] != false
-        else
-          ActiveSupport::Deprecation.warn "save(#{options}) is deprecated, please give save(:validate => #{options}) instead", caller
-          options
-      end
+    def valid?(context = nil)
+      context ||= (new_record? ? :create : :update)
+      super(context)
 
-      if perform_validation
-        errors.clear
+      deprecated_callback_method(:validate)
+      deprecated_callback_method(:"validate_on_#{context}")
 
-        self.validation_context = new_record? ? :create : :update
-        _run_validate_callbacks
+      errors.empty?
+    end
 
-        deprecated_callback_method(:validate)
+  protected
 
-        if new_record?
-          deprecated_callback_method(:validate_on_create)
-        else
-          deprecated_callback_method(:validate_on_update)
-        end
+    def perform_validations(options={})
+      perform_validation = case options
+      when Hash
+        options[:validate] != false
+      else
+        ActiveSupport::Deprecation.warn "save(#{options}) is deprecated, please give save(:validate => #{options}) instead", caller
+        options
+      end
 
-        errors.empty?
+      if perform_validation
+        valid?(options.is_a?(Hash) ? options[:context] : nil)
       else
         true
       end
diff --git a/activerecord/test/cases/validations_test.rb b/activerecord/test/cases/validations_test.rb
index e1fb911cc9..b873babc36 100644
--- a/activerecord/test/cases/validations_test.rb
+++ b/activerecord/test/cases/validations_test.rb
@@ -62,6 +62,23 @@ class ValidationsTest < ActiveRecord::TestCase
     assert_equal ["is Wrong Update"], r.errors[:title], "A reply with a bad content should contain an error"
   end
 
+  def test_error_on_given_context
+    r = WrongReply.new
+    assert !r.valid?(:special_case)
+    assert "Invalid", r.errors[:title].join
+
+    r.title = "secret"
+    r.content = "Good"
+    assert r.valid?(:special_case)
+
+    r.title = nil
+    assert !r.save(:context => :special_case)
+    assert "Invalid", r.errors[:title].join
+
+    r.title = "secret"
+    assert r.save(:context => :special_case)
+  end
+
   def test_invalid_record_exception
     assert_raise(ActiveRecord::RecordInvalid) { WrongReply.create! }
     assert_raise(ActiveRecord::RecordInvalid) { WrongReply.new.save! }
diff --git a/activerecord/test/models/reply.rb b/activerecord/test/models/reply.rb
index 264a49b465..6cc9ee038a 100644
--- a/activerecord/test/models/reply.rb
+++ b/activerecord/test/models/reply.rb
@@ -17,6 +17,7 @@ class WrongReply < Reply
   validate :check_empty_title
   validate :check_content_mismatch, :on => :create
   validate :check_wrong_update, :on => :update
+  validate :check_title_is_secret, :on => :special_case
 
   def check_empty_title
     errors[:title] << "Empty" unless attribute_present?("title")
@@ -39,6 +40,10 @@ class WrongReply < Reply
   def check_wrong_update
     errors[:title] << "is Wrong Update" if attribute_present?("title") && title == "Wrong Update"
   end
+
+  def check_title_is_secret
+    errors[:title] << "Invalid" unless title == "secret"
+  end
 end
 
 class SillyReply < Reply