Merge pull request #21505 from morgoth/deprecate-passing-conditions-to-destroy_all-and-delete_all

Deprecate passing conditions to AR::Relation destroy_all and delete_all methods

5 files changed, 29 insertions, 15 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index ec3ec06c2b..5593e84e9c 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,8 @@
+*   Deprecate passing conditions to `ActiveRecord::Relation#delete_all`
+    and `ActiveRecord::Relation#destroy_all`
+
+    *Wojciech Wnętrzak*
+
 *   PostgreSQL, `create_schema`, `drop_schema` and `rename_table` now quote
     schema names.
 
diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb
index e47b7b1ed9..bf08cdbbf3 100644
--- a/activerecord/lib/active_record/relation.rb
+++ b/activerecord/lib/active_record/relation.rb
@@ -418,7 +418,7 @@ module ActiveRecord
       end
     end
 
-    # Destroys the records matching +conditions+ by instantiating each
+    # Destroys the records by instantiating each
     # record and calling its +destroy+ method. Each object's callbacks are
     # executed (including <tt>:dependent</tt> association options). Returns the
     # collection of objects that were destroyed; each will be frozen, to
@@ -431,20 +431,15 @@ module ActiveRecord
     # rows quickly, without concern for their associations or callbacks, use
     # +delete_all+ instead.
     #
-    # ==== Parameters
-    #
-    # * +conditions+ - A string, array, or hash that specifies which records
-    #   to destroy. If omitted, all records are destroyed. See the
-    #   Conditions section in the introduction to ActiveRecord::Base for
-    #   more information.
-    #
     # ==== Examples
     #
-    #   Person.destroy_all("last_login < '2004-04-04'")
-    #   Person.destroy_all(status: "inactive")
     #   Person.where(age: 0..18).destroy_all
     def destroy_all(conditions = nil)
       if conditions
+        ActiveSupport::Deprecation.warn(<<-MESSAGE.squish)
+          Passing conditions to destroy_all is deprecated and will be removed in Rails 5.1.
+          To achieve the same use where(conditions).destroy_all
+        MESSAGE
         where(conditions).destroy_all
       else
         to_a.each(&:destroy).tap { reset }
@@ -478,15 +473,13 @@ module ActiveRecord
       end
     end
 
-    # Deletes the records matching +conditions+ without instantiating the records
+    # Deletes the records without instantiating the records
     # first, and hence not calling the +destroy+ method nor invoking callbacks. This
     # is a single SQL DELETE statement that goes straight to the database, much more
     # efficient than +destroy_all+. Be careful with relations though, in particular
     # <tt>:dependent</tt> rules defined on associations are not honored. Returns the
     # number of rows affected.
     #
-    #   Post.delete_all("person_id = 5 AND (category = 'Something' OR category = 'Else')")
-    #   Post.delete_all(["person_id = ? AND (category = ? OR category = ?)", 5, 'Something', 'Else'])
     #   Post.where(person_id: 5).where(category: ['Something', 'Else']).delete_all
     #
     # Both calls delete the affected posts all at once with a single DELETE statement.
@@ -512,6 +505,10 @@ module ActiveRecord
       end
 
       if conditions
+        ActiveSupport::Deprecation.warn(<<-MESSAGE.squish)
+          Passing conditions to delete_all is deprecated and will be removed in Rails 5.1.
+          To achieve the same use where(conditions).delete_all
+        MESSAGE
         where(conditions).delete_all
       else
         stmt = Arel::DeleteManager.new
diff --git a/activerecord/test/cases/persistence_test.rb b/activerecord/test/cases/persistence_test.rb
index cdf63957f4..7f14082a9a 100644
--- a/activerecord/test/cases/persistence_test.rb
+++ b/activerecord/test/cases/persistence_test.rb
@@ -126,7 +126,7 @@ class PersistenceTest < ActiveRecord::TestCase
     assert ! topics_by_mary.empty?
 
     assert_difference('Topic.count', -topics_by_mary.size) do
-      destroyed = Topic.destroy_all(conditions).sort_by(&:id)
+      destroyed = Topic.where(conditions).destroy_all.sort_by(&:id)
       assert_equal topics_by_mary, destroyed
       assert destroyed.all?(&:frozen?), "destroyed topics should be frozen"
     end
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index 5f48c2b40f..8256762f96 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -931,6 +931,12 @@ class RelationTest < ActiveRecord::TestCase
     assert davids.loaded?
   end
 
+  def test_destroy_all_with_conditions_is_deprecated
+    assert_deprecated do
+      assert_difference('Author.count', -1) { Author.destroy_all(name: 'David') }
+    end
+  end
+
   def test_delete_all
     davids = Author.where(:name => 'David')
 
@@ -938,6 +944,12 @@ class RelationTest < ActiveRecord::TestCase
     assert ! davids.loaded?
   end
 
+  def test_delete_all_with_conditions_is_deprecated
+    assert_deprecated do
+      assert_difference('Author.count', -1) { Author.delete_all(name: 'David') }
+    end
+  end
+
   def test_delete_all_loaded
     davids = Author.where(:name => 'David')
 
diff --git a/activerecord/test/models/topic.rb b/activerecord/test/models/topic.rb
index d17270021a..176bc79dc7 100644
--- a/activerecord/test/models/topic.rb
+++ b/activerecord/test/models/topic.rb
@@ -86,7 +86,7 @@ class Topic < ActiveRecord::Base
     end
 
     def destroy_children
-      self.class.delete_all "parent_id = #{id}"
+      self.class.where("parent_id = #{id}").delete_all
     end
 
     def set_email_address