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

activerecord/CHANGELOG.md

+*   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.
 

activerecord/lib/active_record/relation.rb

@@ -418,7 +418,7 @@ def update(id = :all, attributes)
       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 @@ def update(id = :all, attributes)
     # 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 @@ def destroy(id)
       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 @@ def delete_all(conditions = nil)
       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

activerecord/test/cases/persistence_test.rb

@@ -126,7 +126,7 @@ def test_destroy_all
     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

activerecord/test/cases/relations_test.rb

@@ -931,6 +931,12 @@ def test_destroy_all
     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 @@ def test_delete_all
     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')
 

activerecord/test/models/topic.rb

@@ -86,7 +86,7 @@ def default_written_on
     end
 
     def destroy_children
-      self.class.delete_all "parent_id = #{id}"
+      self.class.where("parent_id = #{id}").delete_all
     end
 
     def set_email_address