Fixed that Base.save should always return false if the save didn't succeed, including if it has halted by before_save's (closes #1861, #2477) [DHH]

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

3 files changed, 23 insertions, 27 deletions

diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index 88baaecfd3..1fd247e47b 100755
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -21,6 +21,8 @@ module ActiveRecord #:nodoc:
   end
   class RecordNotFound < ActiveRecordError #:nodoc:
   end
+  class RecordNotSaved < ActiveRecordError #:nodoc:
+  end
   class StatementInvalid < ActiveRecordError #:nodoc:
   end
   class PreparedStatementInvalid < ActiveRecordError #:nodoc:
@@ -1285,9 +1287,15 @@ module ActiveRecord #:nodoc:
       # * No record exists: Creates a new record with values matching those of the object attributes.
       # * A record does exist: Updates the record with values matching those of the object attributes.
       def save
-        raise ActiveRecord::ReadOnlyRecord if readonly?
+        raise ReadOnlyRecord if readonly?
         create_or_update
       end
+      
+      # Attempts to save the record, but instead of just returning false if it couldn't happen, it raises a 
+      # RecordNotSaved exception
+      def save!
+        raise RecordNotSaved unless save
+      end
 
       # Deletes the record in the database and freezes this instance to reflect that no changes should
       # be made (since they can't be persisted).
@@ -1505,6 +1513,8 @@ module ActiveRecord #:nodoc:
           "WHERE #{self.class.primary_key} = #{quote(id)}",
           "#{self.class.name} Update"
         )
+        
+        return true
       end
 
       # Creates a new record with values matching those of the instance attributes.
@@ -1522,6 +1532,8 @@ module ActiveRecord #:nodoc:
         )
 
         @new_record = false
+        
+        return true
       end
 
       # Sets the attribute used for single table inheritance to this class name if this is not the ActiveRecord descendent.
diff --git a/activerecord/lib/active_record/validations.rb b/activerecord/lib/active_record/validations.rb
index 53c4f596fe..e206dfbbf1 100755
--- a/activerecord/lib/active_record/validations.rb
+++ b/activerecord/lib/active_record/validations.rb
@@ -216,6 +216,9 @@ module ActiveRecord
         alias_method :save_without_validation, :save
         alias_method :save, :save_with_validation
 
+        alias_method :save_without_validation!, :save!
+        alias_method :save!, :save_with_validation!
+
         alias_method :update_attribute_without_validation_skipping, :update_attribute
         alias_method :update_attribute, :update_attribute_with_validation_skipping
       end
@@ -719,7 +722,6 @@ module ActiveRecord
     def save_with_validation(perform_validation = true)
       if perform_validation && valid? || !perform_validation
         save_without_validation
-        true
       else
         false
       end
@@ -727,9 +729,9 @@ 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!
+    def save_with_validation!
       if valid?
-        save(false)
+        save_without_validation!
       else
         raise RecordInvalid.new(self)
       end
diff --git a/activerecord/test/callbacks_test.rb b/activerecord/test/callbacks_test.rb
index c1639c1795..3a4dac90cf 100644
--- a/activerecord/test/callbacks_test.rb
+++ b/activerecord/test/callbacks_test.rb
@@ -324,39 +324,21 @@ class CallbacksTest < Test::Unit::TestCase
   def test_before_save_returning_false
     david = ImmutableDeveloper.find(1)
     assert david.valid?
-    assert david.save
-    assert david.cancelled?
-
-    david = ImmutableDeveloper.find(1)
-    david.salary = 10_000_000
-    assert !david.valid?
     assert !david.save
-    assert !david.cancelled?
-
-    david = ImmutableMethodDeveloper.find(1)
-    assert david.valid?
-    assert david.save
-    assert david.cancelled?
+    assert_raises(ActiveRecord::RecordNotSaved) { david.save! }
 
-    david = ImmutableMethodDeveloper.find(1)
+    david = ImmutableDeveloper.find(1)
     david.salary = 10_000_000
     assert !david.valid?
     assert !david.save
-    assert !david.cancelled?
+    assert_raises(ActiveRecord::RecordInvalid) { david.save! }
   end
 
   def test_before_destroy_returning_false
     david = ImmutableDeveloper.find(1)
-    david.destroy
-    assert david.cancelled?
+    assert !david.destroy
     assert_not_nil ImmutableDeveloper.find_by_id(1)
-
-    david = ImmutableMethodDeveloper.find(1)
-    david.destroy
-    assert david.cancelled?
-    assert_not_nil ImmutableMethodDeveloper.find_by_id(1)
-  end
-
+  end  
 
   def test_zzz_callback_returning_false # must be run last since we modify CallbackDeveloper
     david = CallbackDeveloper.find(1)