Refactor association create and build so before & after callbacks behave consistently. Closes #8854.

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

3 files changed, 44 insertions, 38 deletions

diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb
index 5598d6daa7..5e2e2e0189 100644
--- a/activerecord/lib/active_record/associations/association_collection.rb
+++ b/activerecord/lib/active_record/associations/association_collection.rb
@@ -85,19 +85,15 @@ module ActiveRecord
       end
       
       def create(attrs = {})
-        ensure_owner_is_not_new
-        record = @reflection.klass.send(:with_scope, :create => construct_scope[:create]) { @reflection.klass.create(attrs) }                
-        @target ||= [] unless loaded?
-        @target << record 
-        record
+        if attrs.is_a?(Array)
+          attrs.collect { |attr| create(attr) }
+        else
+          create_record(attrs) { |record| record.save }
+        end
       end
 
       def create!(attrs = {})
-        ensure_owner_is_not_new
-        record = @reflection.klass.send(:with_scope, :create => construct_scope[:create]) { @reflection.klass.create!(attrs) }                
-        @target ||= [] unless loaded?
-        @target << record 
-        record
+        create_record(attrs) { |record| record.save! }
       end
 
       # Returns the size of the collection by executing a SELECT COUNT(*) query if the collection hasn't been loaded and
@@ -189,6 +185,27 @@ module ActiveRecord
         end
 
       private
+
+        def create_record(attrs, &block)
+          ensure_owner_is_not_new
+          record = @reflection.klass.send(:with_scope, :create => construct_scope[:create]) { @reflection.klass.new(attrs) }
+          add_record_to_target_with_callbacks(record, &block)
+        end
+
+        def build_record(attrs, &block)
+          record = @reflection.klass.new(attrs)
+          add_record_to_target_with_callbacks(record, &block)
+        end
+
+        def add_record_to_target_with_callbacks(record)
+          callback(:before_add, record)
+          yield(record) if block_given?
+          @target ||= [] unless loaded?
+          @target << record
+          callback(:after_add, record)
+          record
+        end
+
         def callback(method, record)
           callbacks_for(method).each do |callback|
             case callback
diff --git a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
index d67207fa4d..13c99455c0 100644
--- a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
@@ -8,33 +8,15 @@ module ActiveRecord
 
       def build(attributes = {})
         load_target
-        record = @reflection.klass.new(attributes)
-        @target << record
-        record
+        build_record(attributes)
       end
 
       def create(attributes = {})
-        # Can't use Base.create because the foreign key may be a protected attribute.
-        ensure_owner_is_not_new
-        if attributes.is_a?(Array)
-          attributes.collect { |attr| create(attr) }
-        else
-          record = build(attributes)
-          insert_record(record) unless @owner.new_record?
-          record
-        end
+        create_record(attributes) { |record| insert_record(record) }
       end
       
       def create!(attributes = {})
-        # Can't use Base.create! because the foreign key may be a protected attribute.
-        ensure_owner_is_not_new
-        if attributes.is_a?(Array)
-          attributes.collect { |attr| create(attr) }
-        else
-          record = build(attributes)
-          insert_record(record, true) unless @owner.new_record?
-          record
-        end        
+        create_record(attributes) { |record| insert_record(record, true) }
       end
 
       def find_first
@@ -160,6 +142,19 @@ module ActiveRecord
         def finding_with_ambiguous_select?(select_clause)
           !select_clause && @owner.connection.columns(@reflection.options[:join_table], "Join Table Columns").size != 2
         end
+
+      private
+        def create_record(attributes)
+          # Can't use Base.create because the foreign key may be a protected attribute.
+          ensure_owner_is_not_new
+          if attributes.is_a?(Array)
+            attributes.collect { |attr| create(attr) }
+          else
+            record = build(attributes)
+            yield(record)
+            record
+          end
+        end
     end
   end
 end
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index 282b43a2b7..b1b9895832 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -10,13 +10,7 @@ module ActiveRecord
         if attributes.is_a?(Array)
           attributes.collect { |attr| build(attr) }
         else
-          record = @reflection.klass.new(attributes)
-          set_belongs_to_association_for(record)
-          
-          @target ||= [] unless loaded?
-          @target << record
-          
-          record
+          build_record(attributes) { |record| set_belongs_to_association_for(record) }
         end
       end