Replace all calls to db_delete, which is deprecated

The patch seems OK according to standards.

Good work on this so far! I think that this patch takes a good approach and scope to fixing these.

I have queued test runs with Postgres and SQLite to be safe (always advisable for patches for the DB API or Views).

When I apply the patch, there are still a few usages we are missing, as well as documentation references:

[ibnsina:drupal | Wed 18:43:32] $ grep -r "db_delete" *
core/includes/database.inc: * be handled via db_insert(), db_update() and db_delete() respectively.
core/includes/database.inc:function db_delete($table, array $options = array()) {
core/lib/Drupal/Core/Database/database.api.php: * db_update(), and db_delete() to obtain a base query on your table, and then
core/lib/Drupal/Core/Database/Driver/mysql/Connection.php:      // We know we are using MySQL here, no need for the slower db_delete().
core/modules/aggregator/src/Tests/ImportOpmlTest.php:    db_delete('aggregator_feed')->execute();
core/modules/dblog/src/Tests/DbLogTest.php:    db_delete('watchdog')->execute();
core/modules/tracker/src/Tests/TrackerTest.php:    db_delete('tracker_node')
core/modules/tracker/src/Tests/TrackerTest.php:    db_delete('tracker_user')
core/tests/Drupal/KernelTests/Core/Config/Storage/DatabaseStorageTest.php:    db_delete('config')->condition('name', $name)->execute();
core/tests/Drupal/KernelTests/Core/Database/DeleteTruncateTest.php:    $delete = db_delete('test_task')
core/tests/Drupal/KernelTests/Core/Database/DeleteTruncateTest.php:    $num_deleted = db_delete('test')
core/tests/Drupal/KernelTests/Core/Database/DeleteTruncateTest.php:    $num_deleted = db_delete('test_special_columns')

Let's fix those in the same way as we did for this patch so far. The documentation references will need a different approach. (They will need to reference the method on the fully-namespaced interface used for the service.) That can be done in a followup to make things easier.

This:

core/includes/database.inc: * be handled via db_insert(), db_update() and db_delete() respectively.

... makes me think some of these will have to be fixed one-by-one.

On #2848161: [meta] Replace calls to deprecated db_*() wrappers I have recommended replacing all code usages first, and then having possibly a single followup to remove documentation references for things like #6.

Replaced db_delete calls in tests.

inject the dependency in ShortcutSetStorage.php file.

I applied the 'replace_all_calls_to_db_delete-2850033-10.patch' patch and evaluated how many 'db_delete' calls there still exist.

- Before
find . -type f \( -name '*.inc' -or -name '*.module' -or -name '*.php' \) -exec grep db_delete\( {} \+ | wc -l
37

- After
find . -type f \( -name '*.inc' -or -name '*.module' -or -name '*.php' \) -exec grep db_delete\( {} \+ | wc -l
12

The remaining calls are these files:

find . -type f \( -name '*.inc' -or -name '*.module' -or -name '*.php' \) -exec grep db_delete\( {} \+ | cut -f1 -d':' | uniq
./core/tests/Drupal/KernelTests/Core/Database/DeleteTruncateTest.php
./core/tests/Drupal/KernelTests/Core/Config/Storage/DatabaseStorageTest.php
./core/modules/aggregator/tests/src/Functional/ImportOpmlTest.php
./core/modules/tracker/tests/src/Functional/TrackerTest.php
./core/modules/dblog/src/Tests/DbLogTest.php
./core/includes/database.inc
./core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
./core/lib/Drupal/Core/Database/database.api.php

Keeping in
./core/tests/Drupal/KernelTests/Core/Database/DeleteTruncateTest.php
./core/tests/Drupal/KernelTests/Core/Config/Storage/DatabaseStorageTest.php
as these looks to self test to me.
Reset of remaining occurrences are documentation, which will be updated separately.

Updated.

Previous patch #13 failed to apply on 8.6.x branch because file path change from core/modules/dblog/src/Tests/DbLogTest.php to core/modules/dblog/tests/src/Functional/DbLogTest.php

Reverting wrongly change for db_insert instance.

Updated naming of a parameter.

1. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
   @@ -286,7 +286,7 @@ public function nextIdDelete() {
   -      // We know we are using MySQL here, no need for the slower db_delete().
   +      // We know we are using MySQL here, no need for the slower \Drupal::database()->delete().
   

   needs wrap on 80 chars

2. +++ b/core/modules/tracker/tracker.module
   @@ -381,10 +381,10 @@ function _tracker_remove($nid, $uid = NULL, $changed = NULL) {
        // If the node doesn't exist, remove everything.
   -    db_delete('tracker_node')
   +    \Drupal::database()->delete('tracker_node')
          ->condition('nid', $nid)
          ->execute();
   -    db_delete('tracker_user')
   +    \Drupal::database()->delete('tracker_user')
   

   use variable for `\Drupal::database()`

Rerolled and updated patch

one more fix

reroll after #2848821: Replace all calls to db_close, which is deprecated

+++ b/core/modules/aggregator/tests/src/Functional/ImportOpmlTest.php
@@ -70,6 +70,7 @@ public function validateImportFormFields() {
+    $connection = \Drupal::database();

+++ b/core/modules/dblog/tests/src/Functional/DbLogTest.php
@@ -554,7 +554,7 @@ public function testFilter() {
+    \Drupal::database()->delete('watchdog')->execute();

+++ b/core/modules/tracker/tests/src/Functional/TrackerTest.php
@@ -373,9 +373,10 @@ public function testTrackerCronIndexing() {
+    $connection = \Drupal::database();

+++ b/core/tests/Drupal/KernelTests/Core/Config/Storage/DatabaseStorageTest.php
@@ -38,7 +38,7 @@ protected function update($name, $data) {
+    \Drupal::database()->delete('config')->condition('name', $name)->execute();

here we need to get a final decision how to properly get the database connection in test contexts, #2991337: Document the recommended ways to obtain the database connection object

+++ b/core/tests/Drupal/KernelTests/Core/Database/DeleteTruncateTest.php
@@ -21,13 +21,14 @@ class DeleteTruncateTest extends DatabaseTestBase {
    * Confirms that we can use a subselect in a delete successfully.
    */
   public function testSubselectDelete() {
+    $connection = \Drupal::database();
     $num_records_before = db_query('SELECT COUNT(*) FROM {test_task}')->fetchField();
     $pid_to_delete = db_query("SELECT * FROM {test_task} WHERE task = 'sleep'")->fetchField();
 
     $subquery = db_select('test', 't')
       ->fields('t', ['id'])
       ->condition('t.id', [$pid_to_delete], 'IN');
-    $delete = db_delete('test_task')
+    $delete = $connection->delete('test_task')
       ->condition('task', 'sleep')
       ->condition('pid', $subquery, 'IN');
 
@@ -42,9 +43,10 @@ public function testSubselectDelete() {

@@ -42,9 +43,10 @@ public function testSubselectDelete() {
    * Confirms that we can delete a single record successfully.
    */
   public function testSimpleDelete() {
+    $connection = \Drupal::database();
     $num_records_before = db_query('SELECT COUNT(*) FROM {test}')->fetchField();
 
-    $num_deleted = db_delete('test')
+    $num_deleted = $connection->delete('test')
       ->condition('id', 1)
       ->execute();
     $this->assertIdentical($num_deleted, 1, 'Deleted 1 record.');
@@ -148,9 +150,10 @@ public function testTruncateTransactionRollback() {

@@ -148,9 +150,10 @@ public function testTruncateTransactionRollback() {
    * Confirms that we can delete a single special column name record successfully.
    */
   public function testSpecialColumnDelete() {
+    $connection = \Drupal::database();
     $num_records_before = db_query('SELECT COUNT(*) FROM {test_special_columns}')->fetchField();
 
-    $num_deleted = db_delete('test_special_columns')
+    $num_deleted = $connection->delete('test_special_columns')
       ->condition('id', 1)
       ->execute();
     $this->assertIdentical($num_deleted, 1, 'Deleted 1 special column record.');
 

here you can directly use $this->connection as the class is extending from DatabaseTestBase. See testTruncateInTransaction and testTruncateTransactionRollback which are already doing that.

Updated patch
- Using $this->connection in DeleteTruncateTest as this inherited from DatabaseTestBase
- DatabaseStorageTest.php is Kernel test which everywhere using Database::getConnection()

Functional tests still depends on #2991337: Document the recommended ways to obtain the database connection object

Fixed functional tests - I think better to make test connections running in each testcase are independent from container
That's because no matter on container rebuild testing mocked database connection will remain the same

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -610,7 +610,8 @@ public function nextIdDelete() {
+      // We know we are using MySQL here, no need for the slower
+      // \Drupal::database()->delete().

Nit: \Drupal::database()->delete() can just be ::delete() here, we're in the Connection object and it's a method of the class

+++ b/core/tests/Drupal/KernelTests/Core/Database/DatabaseLegacyTest.php
@@ -206,4 +207,13 @@ public function testDbMerge() {
 
+  /**
+   * Tests deprecation of the db_delete() function.
+   *
+   * @expectedDeprecation db_delete is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Instead, get a database connection injected into your service from the container and call delete() on it. For example, $injected_database->delete($table, $options). See https://www.drupal.org/node/2993033
+   */
+  public function testDbDelete() {
+    $this->assertInstanceOf(Delete::class, db_delete('test'));
+  }
+

Can we test a real delete here instead, for instance c/p from DeleteTruncateTest::testSimpleDelete?

I think this is more than ready otherwise... just we do not have a conclusion on #2991337: Document the recommended ways to obtain the database connection object yet. Will set that one as a blocker as we're getting stuck on the more complicated db_* deprecation issues without that. Tagging for Framework Manager review, also.

Can we test a real delete here instead, for instance c/p from DeleteTruncateTest::testSimpleDelete?

No, we should test result of the function which is Delete object - this test intended for test that legacy function works

fixed docs

ok then

Re-roll after #2848137: Replace all calls to db_update, which is deprecated

Committed 84bde9d and pushed to 8.7.x. Thanks!