Remove the dependent_restrict_raises option.

It's not really a good idea to have this as a global config option. We
should allow people to specify the behaviour per association.

There will now be two new values:

* :dependent => :restrict_with_exception implements the current
  behaviour of :restrict. :restrict itself is deprecated in favour of
  :restrict_with_exception.
* :dependent => :restrict_with_error implements the new behaviour - it
  adds an error to the owner if there are dependent records present

See #4727 for the original discussion of this.

10 files changed, 87 insertions, 131 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index a99d7fdde8..e6237ef437 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -486,24 +486,13 @@
 *   Added the `ActiveRecord::NullRelation` class implementing the null
     object pattern for the Relation class. *Juanjo Bazán*
 
-*   Added deprecation for the `:dependent => :restrict` association option.
+*   Added new `:dependent => :restrict_with_error` option. This will add
+    an error to the model, rather than raising an exception.
 
-    Please note:
-
-      * Up until now `has_many` and `has_one`, `:dependent => :restrict`
-        option raised a `DeleteRestrictionError` at the time of destroying
-        the object. Instead, it will add an error on the model.
-
-      * To fix this warning, make sure your code isn't relying on a
-        `DeleteRestrictionError` and then add
-        `config.active_record.dependent_restrict_raises = false` to your
-        application config.
-
-      * New rails application would be generated with the
-        `config.active_record.dependent_restrict_raises = false` in the
-        application config.
+    The `:restrict` option is renamed to `:restrict_with_exception` to
+    make this distinction explicit.
 
-    *Manoj Kumar*
+    *Manoj Kumar & Jon Leighton*
 
 *   Added `create_join_table` migration helper to create HABTM join tables
 
diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index f5ee4f3ebe..d9857b8fbd 100644
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -1094,12 +1094,14 @@ module ActiveRecord
       # [:primary_key]
       #   Specify the method that returns the primary key used for the association. By default this is +id+.
       # [:dependent]
-      #   If set to <tt>:destroy</tt> all the associated objects are destroyed
-      #   alongside this object by calling their +destroy+ method. If set to <tt>:delete_all</tt> all associated
-      #   objects are deleted *without* calling their +destroy+ method. If set to <tt>:nullify</tt> all associated
-      #   objects' foreign keys are set to +NULL+ *without* calling their +save+ callbacks. If set to
-      #   <tt>:restrict</tt> an error will be added to the object, preventing its deletion, if any associated 
-      #   objects are present.
+      #   Controls what happens to the associated objects when
+      #   their owner is destroyed:
+      #
+      #   * <tt>:destroy</tt> causes all the associated objects to also be destroyed
+      #   * <tt>:delete_all</tt> causes all the asssociated objects to be deleted directly from the database (so callbacks will not execute)
+      #   * <tt>:nullify</tt> causes the foreign keys to be set to +NULL+. Callbacks are not executed.
+      #   * <tt>:restrict_with_exception</tt> causes an exception to be raised if there are any associated records
+      #   * <tt>:restrict_with_error</tt> causes an error to be added to the owner if there are any associated objects
       #
       #   If using with the <tt>:through</tt> option, the association on the join model must be
       #   a +belongs_to+, and the records which get deleted are the join records, rather than
@@ -1203,11 +1205,14 @@ module ActiveRecord
       #   from the association name. So <tt>has_one :manager</tt> will by default be linked to the Manager class, but
       #   if the real class name is Person, you'll have to specify it with this option.
       # [:dependent]
-      #   If set to <tt>:destroy</tt>, the associated object is destroyed when this object is. If set to
-      #   <tt>:delete</tt>, the associated object is deleted *without* calling its destroy method.
-      #   If set to <tt>:nullify</tt>, the associated object's foreign key is set to +NULL+.
-      #   If set to <tt>:restrict</tt>, an error will be added to the object, preventing its deletion, if an
-      #   associated object is present.
+      #   Controls what happens to the associated objects when
+      #   their owner is destroyed:
+      #
+      #   * <tt>:destroy</tt> causes all the associated objects to also be destroyed
+      #   * <tt>:delete</tt> causes all the asssociated objects to be deleted directly from the database (so callbacks will not execute)
+      #   * <tt>:nullify</tt> causes the foreign keys to be set to +NULL+. Callbacks are not executed.
+      #   * <tt>:restrict_with_exception</tt> causes an exception to be raised if there are any associated records
+      #   * <tt>:restrict_with_error</tt> causes an error to be added to the owner if there are any associated objects
       # [:foreign_key]
       #   Specify the foreign key used for the association. By default this is guessed to be the name
       #   of this class in lower-case and "_id" suffixed. So a Person class that makes a +has_one+ association
diff --git a/activerecord/lib/active_record/associations/builder/association.rb b/activerecord/lib/active_record/associations/builder/association.rb
index c3f32b5ed9..47f06421ee 100644
--- a/activerecord/lib/active_record/associations/builder/association.rb
+++ b/activerecord/lib/active_record/associations/builder/association.rb
@@ -85,35 +85,35 @@ module ActiveRecord::Associations::Builder
           raise ArgumentError, "The :dependent option expects either " \
             "#{valid_options_message} (#{dependent.inspect})"
         end
-      end
 
-      def dependent_restrict_raises?
-        ActiveRecord::Base.dependent_restrict_raises == true
+        if dependent == :restrict
+          ActiveSupport::Deprecation.warn(
+            "The :restrict option is deprecated. Please use :restrict_with_exception instead, which " \
+            "provides the same functionality."
+          )
+        end
       end
 
-      def dependent_restrict_deprecation_warning
-        if dependent_restrict_raises?
-          msg = "In the next release, `:dependent => :restrict` will not raise a `DeleteRestrictionError`. "\
-                "Instead, it will add an error on the model. To fix this warning, make sure your code " \
-                "isn't relying on a `DeleteRestrictionError` and then add " \
-                "`config.active_record.dependent_restrict_raises = false` to your application config."
-          ActiveSupport::Deprecation.warn msg
+      def define_restrict_with_exception_dependency_method
+        name = self.name
+        mixin.redefine_method(dependency_method_name) do
+          has_one_macro = association(name).reflection.macro == :has_one
+          if has_one_macro ? !send(name).nil? : send(name).exists?
+            raise ActiveRecord::DeleteRestrictionError.new(name)
+          end
         end
       end
+      alias define_restrict_dependency_method define_restrict_with_exception_dependency_method
 
-      def define_restrict_dependency_method
+      def define_restrict_with_error_dependency_method
         name = self.name
         mixin.redefine_method(dependency_method_name) do
           has_one_macro = association(name).reflection.macro == :has_one
           if has_one_macro ? !send(name).nil? : send(name).exists?
-            if dependent_restrict_raises?
-              raise ActiveRecord::DeleteRestrictionError.new(name)
-            else
-              key  = has_one_macro ? "one" : "many"
-              errors.add(:base, :"restrict_dependent_destroy.#{key}",
-                         :record => self.class.human_attribute_name(name).downcase)
-              return false
-            end
+            key  = has_one_macro ? "one" : "many"
+            errors.add(:base, :"restrict_dependent_destroy.#{key}",
+                       :record => self.class.human_attribute_name(name).downcase)
+            false
           end
         end
       end
diff --git a/activerecord/lib/active_record/associations/builder/has_many.rb b/activerecord/lib/active_record/associations/builder/has_many.rb
index 9e60dbc30b..63278c8cfd 100644
--- a/activerecord/lib/active_record/associations/builder/has_many.rb
+++ b/activerecord/lib/active_record/associations/builder/has_many.rb
@@ -19,8 +19,8 @@ module ActiveRecord::Associations::Builder
 
       def configure_dependency
         if dependent = options[:dependent]
-          check_valid_dependent! dependent, [:destroy, :delete_all, :nullify, :restrict]
-          dependent_restrict_deprecation_warning if dependent == :restrict
+          check_valid_dependent! dependent, [:destroy, :delete_all, :nullify, :restrict,
+                                             :restrict_with_error, :restrict_with_exception]
 
           send("define_#{dependent}_dependency_method")
           model.before_destroy dependency_method_name
diff --git a/activerecord/lib/active_record/associations/builder/has_one.rb b/activerecord/lib/active_record/associations/builder/has_one.rb
index 9c84f1913a..82c5905caf 100644
--- a/activerecord/lib/active_record/associations/builder/has_one.rb
+++ b/activerecord/lib/active_record/associations/builder/has_one.rb
@@ -25,8 +25,7 @@ module ActiveRecord::Associations::Builder
 
       def configure_dependency
         if dependent = options[:dependent]
-          check_valid_dependent! dependent, [:destroy, :delete, :nullify, :restrict]
-          dependent_restrict_deprecation_warning if dependent == :restrict
+          check_valid_dependent! dependent, [:destroy, :delete, :nullify, :restrict, :restrict_with_error, :restrict_with_exception]
 
           send("define_#{dependent}_dependency_method")
           model.before_destroy dependency_method_name
diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb
index 1145d2138c..0fddfdf0cb 100644
--- a/activerecord/lib/active_record/core.rb
+++ b/activerecord/lib/active_record/core.rb
@@ -82,15 +82,6 @@ module ActiveRecord
       # The connection handler
       config_attribute :connection_handler
 
-      ##
-      # :singleton-method:
-      # Specifies whether or not has_many or has_one association option
-      # :dependent => :restrict raises an exception. If set to true, the
-      # ActiveRecord::DeleteRestrictionError exception will be raised
-      # along with a DEPRECATION WARNING. If set to false, an error would
-      # be added to the model instead.
-      config_attribute :dependent_restrict_raises
-
       %w(logger configurations default_timezone schema_format timestamped_migrations).each do |name|
         config_attribute name, global: true
       end
diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb
index 43440c1146..04714f42e9 100644
--- a/activerecord/test/cases/associations/has_many_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_associations_test.rb
@@ -1091,9 +1091,6 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
   end
 
   def test_restrict
-    option_before = ActiveRecord::Base.dependent_restrict_raises
-    ActiveRecord::Base.dependent_restrict_raises = true
-
     firm = RestrictedFirm.create!(:name => 'restrict')
     firm.companies.create(:name => 'child')
 
@@ -1101,15 +1098,25 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
     assert_raise(ActiveRecord::DeleteRestrictionError) { firm.destroy }
     assert RestrictedFirm.exists?(:name => 'restrict')
     assert firm.companies.exists?(:name => 'child')
-  ensure
-    ActiveRecord::Base.dependent_restrict_raises = option_before
   end
 
-  def test_restrict_when_dependent_restrict_raises_config_set_to_false
-    option_before = ActiveRecord::Base.dependent_restrict_raises
-    ActiveRecord::Base.dependent_restrict_raises = false
+  def test_restrict_is_deprecated
+    klass = Class.new(ActiveRecord::Base)
+    assert_deprecated { klass.has_many :posts, dependent: :restrict }
+  end
 
-    firm = RestrictedFirm.create!(:name => 'restrict')
+  def test_restrict_with_exception
+    firm = RestrictedWithExceptionFirm.create!(:name => 'restrict')
+    firm.companies.create(:name => 'child')
+
+    assert !firm.companies.empty?
+    assert_raise(ActiveRecord::DeleteRestrictionError) { firm.destroy }
+    assert RestrictedWithExceptionFirm.exists?(:name => 'restrict')
+    assert firm.companies.exists?(:name => 'child')
+  end
+
+  def test_restrict_with_error
+    firm = RestrictedWithErrorFirm.create!(:name => 'restrict')
     firm.companies.create(:name => 'child')
 
     assert !firm.companies.empty?
@@ -1119,10 +1126,8 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
     assert !firm.errors.empty?
 
     assert_equal "Cannot delete record because dependent companies exist", firm.errors[:base].first
-    assert RestrictedFirm.exists?(:name => 'restrict')
+    assert RestrictedWithErrorFirm.exists?(:name => 'restrict')
     assert firm.companies.exists?(:name => 'child')
-  ensure
-    ActiveRecord::Base.dependent_restrict_raises = option_before
   end
 
   def test_included_in_collection
@@ -1602,18 +1607,6 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
     assert_equal [bulb1, bulb3], result
   end
 
-  def test_building_has_many_association_with_restrict_dependency
-    option_before = ActiveRecord::Base.dependent_restrict_raises
-    ActiveRecord::Base.dependent_restrict_raises = true
-
-    klass = Class.new(ActiveRecord::Base)
-
-    assert_deprecated     { klass.has_many :companies, :dependent => :restrict }
-    assert_not_deprecated { klass.has_many :companies }
-  ensure
-    ActiveRecord::Base.dependent_restrict_raises = option_before
-  end
-
   def test_collection_association_with_private_kernel_method
     firm = companies(:first_firm)
     assert_equal [accounts(:signals37)], firm.accounts.open
diff --git a/activerecord/test/cases/associations/has_one_associations_test.rb b/activerecord/test/cases/associations/has_one_associations_test.rb
index 752c33b1db..8bc633f2b5 100644
--- a/activerecord/test/cases/associations/has_one_associations_test.rb
+++ b/activerecord/test/cases/associations/has_one_associations_test.rb
@@ -156,10 +156,7 @@ class HasOneAssociationsTest < ActiveRecord::TestCase
     assert_nothing_raised { firm.destroy }
   end
 
-  def test_dependence_with_restrict
-    option_before = ActiveRecord::Base.dependent_restrict_raises
-    ActiveRecord::Base.dependent_restrict_raises = true
-
+  def test_restrict
     firm = RestrictedFirm.create!(:name => 'restrict')
     firm.create_account(:credit_limit => 10)
 
@@ -168,38 +165,26 @@ class HasOneAssociationsTest < ActiveRecord::TestCase
     assert_raise(ActiveRecord::DeleteRestrictionError) { firm.destroy }
     assert RestrictedFirm.exists?(:name => 'restrict')
     assert firm.account.present?
-  ensure
-    ActiveRecord::Base.dependent_restrict_raises = option_before
   end
 
-  def test_dependence_with_restrict_with_dependent_restrict_raises_config_set_to_false
-    option_before = ActiveRecord::Base.dependent_restrict_raises
-    ActiveRecord::Base.dependent_restrict_raises = false
+  def test_restrict_is_deprecated
+    klass = Class.new(ActiveRecord::Base)
+    assert_deprecated { klass.has_one :post, dependent: :restrict }
+  end
 
-    firm = RestrictedFirm.create!(:name => 'restrict')
+  def test_restrict_with_exception
+    firm = RestrictedWithExceptionFirm.create!(:name => 'restrict')
     firm.create_account(:credit_limit => 10)
 
     assert_not_nil firm.account
 
-    firm.destroy
-
-    assert !firm.errors.empty?
-    assert_equal "Cannot delete record because a dependent account exists", firm.errors[:base].first
-    assert RestrictedFirm.exists?(:name => 'restrict')
+    assert_raise(ActiveRecord::DeleteRestrictionError) { firm.destroy }
+    assert RestrictedWithExceptionFirm.exists?(:name => 'restrict')
     assert firm.account.present?
-  ensure
-    ActiveRecord::Base.dependent_restrict_raises = option_before
   end
 
-  def test_dependence_with_restrict_with_dependent_restrict_raises_config_set_to_false_and_attribute_name
-    old_backend = I18n.backend
-    I18n.backend = I18n::Backend::Simple.new
-    I18n.backend.store_translations 'en', :activerecord => {:attributes => {:restricted_firm => {:account => "account model"}}}
-
-    option_before = ActiveRecord::Base.dependent_restrict_raises
-    ActiveRecord::Base.dependent_restrict_raises = false
-
-    firm = RestrictedFirm.create!(:name => 'restrict')
+  def test_restrict_with_error
+    firm = RestrictedWithErrorFirm.create!(:name => 'restrict')
     firm.create_account(:credit_limit => 10)
 
     assert_not_nil firm.account
@@ -207,12 +192,9 @@ class HasOneAssociationsTest < ActiveRecord::TestCase
     firm.destroy
 
     assert !firm.errors.empty?
-    assert_equal "Cannot delete record because a dependent account model exists", firm.errors[:base].first
-    assert RestrictedFirm.exists?(:name => 'restrict')
+    assert_equal "Cannot delete record because a dependent account exists", firm.errors[:base].first
+    assert RestrictedWithErrorFirm.exists?(:name => 'restrict')
     assert firm.account.present?
-  ensure
-    ActiveRecord::Base.dependent_restrict_raises = option_before
-    I18n.backend = old_backend
   end
 
   def test_successful_build_association
@@ -524,18 +506,6 @@ class HasOneAssociationsTest < ActiveRecord::TestCase
     assert_equal car.id, bulb.attributes_after_initialize['car_id']
   end
 
-  def test_building_has_one_association_with_dependent_restrict
-    option_before = ActiveRecord::Base.dependent_restrict_raises
-    ActiveRecord::Base.dependent_restrict_raises = true
-
-    klass = Class.new(ActiveRecord::Base)
-
-    assert_deprecated     { klass.has_one :account, :dependent => :restrict }
-    assert_not_deprecated { klass.has_one :account }
-  ensure
-    ActiveRecord::Base.dependent_restrict_raises = option_before
-  end
-
   def test_has_one_transaction
     company = companies(:first_firm)
     account = Account.find(1)
diff --git a/activerecord/test/cases/helper.rb b/activerecord/test/cases/helper.rb
index 018064233a..4c6d4666ed 100644
--- a/activerecord/test/cases/helper.rb
+++ b/activerecord/test/cases/helper.rb
@@ -19,9 +19,6 @@ require 'support/connection'
 # Show backtraces for deprecated behavior for quicker cleanup.
 ActiveSupport::Deprecation.debug = true
 
-# Avoid deprecation warning setting dependent_restrict_raises to false. The default is true
-ActiveRecord::Base.dependent_restrict_raises = false
-
 # Connect to the database
 ARTest.connect
 
diff --git a/activerecord/test/models/company.rb b/activerecord/test/models/company.rb
index 5bfbb5e855..75f38d275c 100644
--- a/activerecord/test/models/company.rb
+++ b/activerecord/test/models/company.rb
@@ -115,8 +115,20 @@ class DependentFirm < Company
 end
 
 class RestrictedFirm < Company
-  has_one :account, -> { order("id") }, :foreign_key => "firm_id", :dependent => :restrict
-  has_many :companies, -> { order("id") }, :foreign_key => 'client_of', :dependent => :restrict
+  ActiveSupport::Deprecation.silence do
+    has_one :account, -> { order("id") }, :foreign_key => "firm_id", :dependent => :restrict
+    has_many :companies, -> { order("id") }, :foreign_key => 'client_of', :dependent => :restrict
+  end
+end
+
+class RestrictedWithExceptionFirm < Company
+  has_one :account, -> { order("id") }, :foreign_key => "firm_id", :dependent => :restrict_with_exception
+  has_many :companies, -> { order("id") }, :foreign_key => 'client_of', :dependent => :restrict_with_exception
+end
+
+class RestrictedWithErrorFirm < Company
+  has_one :account, -> { order("id") }, :foreign_key => "firm_id", :dependent => :restrict_with_error
+  has_many :companies, -> { order("id") }, :foreign_key => 'client_of', :dependent => :restrict_with_error
 end
 
 class Client < Company