Replace all calls to db_drop_table, which is deprecated.

Thank you! Link the parent issue when you make these, please. They are tasks, not bugs.

2 instances of "db_drop_table()" found in following:

core/includes/schema.inc:138:      db_drop_table($table['name']);
core/modules/simpletest/simpletest.module:672:      db_drop_table($matches[0]);

patch #2 covers both of them. Looks good!

Thanks !

+++ b/core/modules/simpletest/simpletest.module
@@ -669,7 +669,7 @@ function simpletest_clean_database() {
-      db_drop_table($matches[0]);
+     \Drupal::database()->schema()->dropTable($matches[0]);

nitpick - the line indentation is wrong.

@marvin_B8: You forgot to replace some instances in Drupal\KernelTests\Core\Database\SchemaTest

Updated patch as per comment #7 & also added interdiff.

Same patch with added tests against 8.4.x branch.

All code changes are good.
All instances of the usage of db_drop_table() are replaced.
The patch looks good to me.

+++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
@@ -117,7 +117,7 @@ function testSchema() {
-    db_drop_table('test_table2');
+    \Drupal::database()->schema()->dropTable('test_table2');

At least this is the self-test of the function. The whole thing is the test for the schema. So I think we should leave the usages here and discuss what to do with it in phase 2 of this cleanup initiative.

Thanks!

Thanks @xjm, as per your comment #11 i have updated the patch & added a interdiff also.

This is what @xjm wants. Back to RTBC.

This is what @xjm wants. Back to RTBC.

lol, that in itself is not the criterion for RTBC. :) I am often wrong. If you disagree it's always worth adding why.

However, what I was getting at in #11 is that it feels awkward and wrong to use \Drupal::database() in SchemaTest (that's the "whole thing" I was referring to; sorry for being ambiguous). We shouldn't be going to static methods on \Drupal to the container to the service to the class in order to test the class. This applies to numerous of these patches. So, my thought is that it would be better to create a single followup for updating the DB tests (especially SchemaTest) and discuss the best approach.

Again, though, I could be wrong. :)

I have fixed the 'replace_all_calls_to-2848479-12.patch' patch because it was not applying any more because 'array()' notation has already been applied in the files affected by the patch.

Reroll for 8.6.x
Patch is pretty small, so it could be ready to go.
+1 RTBC

Patch cleanup

Reroll
Reverted changes in self-test.
Used $schema instance in separate variable because we have another functions to replace that will use $schema instance.

Upload the correct interdiff

Used $schema instance in separate variable because we have another functions to replace that will use $schema instance

Nice!

Looks good to me, RTBC.

Can we add a deprecation trigger_error() - see db_set_active() for an example.

Wrt to SchemaTest we can replace the calls to drop_table() with Database::getConnection()->schema()->dropTable() -we're already using Database::getConnection() in that test and its fine since refactoring the Database static is really not in scope of this work or any other of the remove usages of db_* functions.

Added change notice https://www.drupal.org/node/2987737
Added Regression test.
Added replacements in the Schema tests.

+++ b/core/tests/Drupal/KernelTests/Core/Database/RegressionTest.php
@@ -71,4 +71,18 @@ public function testDBIsActive() {
+  /**
+   * Tests the db_create_table() and db_drop_table() function.
+   *
+   * @group legacy
+   *
+   * @expectedDeprecation db_drop_table() is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Use \Drupal\Core\Database\Database::getConnection()->schema()->dropTable() instead. See https://www.drupal.org/node/2987737
+   */
+  public function testDBCreateDropTable() {
+    $table = 'temp_test_table';
+    db_create_table($table, []);
+    $this->assertTrue(db_drop_table($table));
+    $this->assertFalse(db_drop_table($table));
+  }
+

I think it would be enough just to test db_drop_table on a non-existing table - we are just testing that the deprecation exception is thrown here, and dropTable implementations take care of returning FALSE if the table is missing.

Also, in the SchemaTest class I think it would make sense to have the $connection and the $schema variables as class properties, initialised at setup, instead of figuring out in each method. Only the testFindTables test is playing around with the connection in this test class, and it could be managed as a special case. AFAICS all the other tests just use the connection set up for the Kernel test. But out of this issue's scope.

Thank's for the review @mondrake

1. +++ b/core/tests/Drupal/KernelTests/Core/Database/RegressionTest.php
   @@ -71,4 +71,15 @@ public function testDBIsActive() {
   +   * Tests the db_create_table() and db_drop_table() function.
   

   db_create_table() is no longer tested.

2. +++ b/core/tests/Drupal/KernelTests/Core/Database/RegressionTest.php
   @@ -71,4 +71,15 @@ public function testDBIsActive() {
   +   * @expectedDeprecation db_drop_table() is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Use \Drupal\Core\Database\Database::getConnection()->schema()->dropTable() instead. See https://www.drupal.org/node/2987737
   +   */
   +  public function testDBCreateDropTable() {
   +    $this->assertFalse(db_drop_table('temp_test_table'));
   +  }
   

   +1

3. +++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
   @@ -23,6 +23,27 @@ class SchemaTest extends KernelTestBase {
   +
   +  protected function setUp() {
   +    parent::setUp();
   +    $this->connection = $this->container->get('database');
   +    $this->schema = $this->connection->schema();
   +  }
   

   I like it +1

   But needs a {@inheritdoc}

+++ b/core/tests/Drupal/KernelTests/Core/Database/RegressionTest.php
@@ -71,4 +71,15 @@ public function testDBIsActive() {
+  /**
+   * Tests the db_create_table() and db_drop_table() function.
+   *
+   * @group legacy
+   *
+   * @expectedDeprecation db_drop_table() is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Use \Drupal\Core\Database\Database::getConnection()->schema()->dropTable() instead. See https://www.drupal.org/node/2987737
+   */
+  public function testDBCreateDropTable() {
+    $this->assertFalse(db_drop_table('temp_test_table'));

Both docblock and test method name need to be updated as table is no longer created.

@voleger interdiff in #32 is incorrect

Also, nitpick - testDBCreateDropTable(): according to CS should be 'testDb*' with lower 'b'. So in this case just testDbDropTable.

+++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
@@ -23,6 +23,27 @@ class SchemaTest extends KernelTestBase {
+  /**
+   * Database schema instance.
+   *
+   * @var \Drupal\Core\Database\Schema
+   */
+  protected $schema;
+
+  protected function setUp() {
+    parent::setUp();
+    $this->connection = $this->container->get('database');
+    $this->schema = $this->connection->schema();
+  }
+
+

There's an extra white line before next method. Also, I suggest to use

    $this->connection = Database::getConnection();

instead of the container - that's how DatabaseTestBase initialises the connection property, and how it is setup in KernelTestBase.

(Sorry for feedback in bits and pieces)

@mondrake np, it's ok. I really happy to see that the db_* replacement issues has some attention from reviewers.

Addressed #33, #34, #35, #36

+++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
@@ -23,6 +23,30 @@ class SchemaTest extends KernelTestBase {
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp() {
+    parent::setUp();
+    $this->connection = Database::getConnection();
+    $this->schema = $this->connection->schema();
+  }
+
+

There is still a double newline after the setUp method, but I think it's RTBC now, it could be fixed on commit.

Can someone file a quick issue to change just SchemaTest to use the new protected properties as that will make the rest of go much quicker. Thanks!

Committed and pushed ead36d31b8 to 8.7.x and ab302cc34c to 8.6.x. Thanks!

Created followup: #2987856: Followup: Update Schema test

I had tried to pull the latest changes but not saw changes in SchemaTest.
Wondering, would those ead36d31b8 and ab302cc34c commits be available in the near future?

@voleger oops pushed now. Sorry.