Association validation does not belong in a before_save callback: move it into a validation method. Restores the expected model.valid? == model.save. Add tests for cancelling save by returning false from a before_save callback. Remove assumption that before_destroy's return value indicates whether the record was destroyed.

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

3 files changed, 47 insertions, 44 deletions

diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index b1bc5f9a70..075d3d8c44 100755
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -308,8 +308,8 @@ module ActiveRecord
         end
 
         add_multiple_associated_save_callbacks(association_name)
-				add_association_callbacks(association_name, options)
-				
+        add_association_callbacks(association_name, options)
+
         collection_accessor_methods(association_name, association_class_name, association_class_primary_key_name, options, HasManyAssociation)
         
         # deprecated api
@@ -686,39 +686,39 @@ module ActiveRecord
         end
 
         def add_multiple_associated_save_callbacks(association_name)
-          module_eval do
-            before_save <<-end_eval
-              @new_record_before_save = new_record?
-              association = instance_variable_get("@#{association_name}")
-              if association.respond_to?(:loaded?)
-                if new_record?
-                  records_to_save = association
-                else
-                  records_to_save = association.select{ |record| record.new_record? }
-                end
-                records_to_save.all? { |record| record.valid? }
+          method_name = "validate_associated_records_for_#{association_name}".to_sym
+          define_method(method_name) do
+            @new_record_before_save = new_record?
+            association = instance_variable_get("@#{association_name}")
+            if association.respond_to?(:loaded?)
+              if new_record?
+                association
+              else
+                association.select { |record| record.new_record? }
+              end.each do |record|
+                errors.add "#{association_name}" unless record.valid?
               end
-            end_eval
+            end
           end
 
-          module_eval do
-            after_callback = <<-end_eval
-              association = instance_variable_get("@#{association_name}")
-              if association.respond_to?(:loaded?)
-                if @new_record_before_save
-                  records_to_save = association
-                else
-                  records_to_save = association.select{ |record| record.new_record? }
-                end
-                records_to_save.each{ |record| association.send(:insert_record, record) }
-                association.send(:construct_sql)   # reconstruct the SQL queries now that we know the owner's id
+          validate method_name
+
+          after_callback = <<-end_eval
+            association = instance_variable_get("@#{association_name}")
+            if association.respond_to?(:loaded?)
+              if @new_record_before_save
+                records_to_save = association
+              else
+                records_to_save = association.select { |record| record.new_record? }
               end
-            end_eval
+              records_to_save.each { |record| association.send(:insert_record, record) }
+              association.send(:construct_sql)   # reconstruct the SQL queries now that we know the owner's id
+            end
+          end_eval
 
-            # Doesn't use after_save as that would save associations added in after_create/after_update twice
-            after_create(after_callback)
-            after_update(after_callback)
-          end
+          # Doesn't use after_save as that would save associations added in after_create/after_update twice
+          after_create(after_callback)
+          after_update(after_callback)
         end
 
         def association_constructor_method(constructor, association_name, association_class_name, association_class_primary_key_name, options, association_proxy_class)
@@ -894,16 +894,15 @@ module ActiveRecord
         end
 
         def add_association_callbacks(association_name, options)
-        	callbacks = %w(before_add after_add before_remove after_remove)
-        	callbacks.each do |callback_name|
-        	  full_callback_name = "#{callback_name.to_s}_for_#{association_name.to_s}"
-        	  defined_callbacks = options[callback_name.to_sym]
-        	  if options.has_key?(callback_name.to_sym)
-        	    callback_array = defined_callbacks.kind_of?(Array) ? defined_callbacks : [defined_callbacks]
-        	    class_inheritable_reader full_callback_name.to_sym
-              write_inheritable_array(full_callback_name.to_sym, callback_array)
-        	  end
-        	end
+          callbacks = %w(before_add after_add before_remove after_remove)
+          callbacks.each do |callback_name|
+            full_callback_name = "#{callback_name.to_s}_for_#{association_name.to_s}"
+            defined_callbacks = options[callback_name.to_sym]
+            if options.has_key?(callback_name.to_sym)
+              class_inheritable_reader full_callback_name.to_sym
+              write_inheritable_array(full_callback_name.to_sym, [defined_callbacks].flatten)
+            end
+          end
         end
 
         def extract_record(schema_abbreviations, table_name, row)
@@ -915,5 +914,6 @@ module ActiveRecord
           return record
         end        
     end
+
   end
 end
diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index e8b92da941..0e1c28e330 100755
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -1206,7 +1206,6 @@ module ActiveRecord #:nodoc:
     private
       def create_or_update
         if new_record? then create else update end
-        true
       end
 
       # Updates the associated record with values matching those of the instant attributes.
diff --git a/activerecord/lib/active_record/validations.rb b/activerecord/lib/active_record/validations.rb
index 4e5fc16a10..ca50cef5a2 100755
--- a/activerecord/lib/active_record/validations.rb
+++ b/activerecord/lib/active_record/validations.rb
@@ -657,7 +657,12 @@ 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_with_validation(perform_validation = true)
-      if perform_validation && valid? || !perform_validation then save_without_validation else false end
+      if perform_validation && valid? || !perform_validation
+        save_without_validation
+        true
+      else
+        false
+      end
     end
 
     # Attempts to save the record just like Base.save but will raise a RecordInvalid exception instead of returning false
@@ -694,8 +699,7 @@ module ActiveRecord
 
     # Returns the Errors object that holds all information about attribute error messages.
     def errors
-      @errors = Errors.new(self) if @errors.nil?
-      @errors
+      @errors ||= Errors.new(self)
     end
 
     protected