Replace all calls to db_table_exists, which is deprecated.

#3 Looks good.
1 "db_table_exists()" call found in core/includes/schema.inc:137. Rest calls are in test files.

Thanks !

Read the meta issue carefully about test replacements.

I'm sending a patch to replace the tests cases that use db_table_exists.

Fixing my previous patch. Removing a wrong regression test.

+++ b/core/modules/simpletest/src/Tests/KernelTestBaseTest.php
@@ -62,7 +62,7 @@ public function testSetUp() {
-    $this->assertFalse(db_table_exists($table), "'$table' database table not found.");

@@ -114,7 +114,7 @@ public function testEnableModulesInstall() {
+    $this->assertFalse(\Drupal::database()->schema()->tableExists($table), "'$table' database table not found.");

Instead use "Database::getConnection()->schema()->tableExists()" method,
check db_table_exists() implementation for details.

@Sharique
I do not agree with you. "\Drupal::database()->schema()->tableExists($table)" will also work.

To understand my point have a look at the implementation of db_table_exists here
https://api.drupal.org/api/drupal/core%21includes%21database.inc/functio...

@hgunicamp's patch in #10 no longer applied to 8.5.x. A reroll with no further modifications is attached.

Changing status to review after #15.

Patch #15 still applies. Tested on 8.6.x
Looks good

This patch should add an @trigger_error() to db_table_exists() so people can discover they need to replace the calls. See https://www.drupal.org/core/deprecation
This will also prove we've replaced all the usages in core.

Will also need to add @group legacy and @expectedDeprecation ... to \Drupal\KernelTests\Core\Database\RegressionTest::testDBTableExists()

#20 #21 - added

+++ b/core/includes/database.inc
--- a/core/includes/schema.inc
+++ b/core/includes/schema.inc

+++ b/core/includes/schema.inc
@@ -134,7 +134,7 @@ function drupal_uninstall_schema($module) {
   foreach ($schema as $table) {
-    if (db_table_exists($table['name'])) {
+    if (\Drupal::database()->schema()->tableExists($table['name'])) {
       db_drop_table($table['name']);
     }
   }

Instead of doing this we could do:

diff --git a/core/includes/schema.inc b/core/includes/schema.inc
index 443cbf1934..06864db44a 100644
--- a/core/includes/schema.inc
+++ b/core/includes/schema.inc
@@ -5,6 +5,8 @@
  * Schema API handling functions.
  */
 
+use Drupal\Core\Database\Database;
+
 /**
  * @addtogroup schemaapi
  * @{
@@ -133,8 +135,9 @@ function drupal_uninstall_schema($module) {
   $schema = drupal_get_module_schema($module);
   _drupal_schema_initialize($schema, $module, FALSE);
 
+  $database_schema = Database::getConnection()->schema();
   foreach ($schema as $table) {
-    if (db_table_exists($table['name'])) {
+    if ($database_schema->tableExists($table['name'])) {
       db_drop_table($table['name']);
     }
   }

But also weirdly the whole table exists check is not necessary. Every single db_drop_table() implementation does a table exists check so we are duplicating the query unnecessarily. This weird. I don't think db_drop_table should check existence. But doing the query twice is just wasteful. And the non-failure for table not existing is part of the API for drop table.

  /**
   * Drop a table.
   *
   * @param $table
   *   The table to be dropped.
   *
   * @return
   *   TRUE if the table was successfully dropped, FALSE if there was no table
   *   by that name to begin with.
   */
  abstract public function dropTable($table);

So tldr; let's just remove the check.

#24: Removed additional check.

Tests are green. So, back to RTBC?

Reroll + Patch moves service fetching out of loops, I bet now patch ready to go

PS: Looking at RegressionTest.php I gonna file follow-up to refactor it or maybe there's one already...

1. +++ b/core/includes/schema.inc
   @@ -134,9 +134,7 @@ function drupal_uninstall_schema($module) {
   -    if (db_table_exists($table['name'])) {
   -      db_drop_table($table['name']);
   -    }
   +    db_drop_table($table['name']);
   

   cos patch does change that line makes sense to clean-up it as #24 said

2. +++ b/core/modules/simpletest/src/Tests/KernelTestBaseTest.php
   @@ -156,7 +156,7 @@ public function testInstallSchema() {
   +    $this->assertTrue(\Drupal::database()->schema()->tableExists($table), "'$table' database table found.");
   
   @@ -171,7 +171,7 @@ public function testInstallSchema() {
   +    $this->assertFalse(\Drupal::database()->schema()->tableExists($table), "'$table' database table not found.");
   
   @@ -185,14 +185,14 @@ public function testInstallSchema() {
   +    $this->assertFalse(\Drupal::database()->schema()->tableExists($table), "'$table' database table not found.");
   ...
   +    $this->assertTrue(\Drupal::database()->schema()->tableExists($table), "'$table' database table found.");
   
   @@ -206,7 +206,7 @@ public function testInstallEntitySchema() {
   +    $this->assertTrue(\Drupal::database()->schema()->tableExists($entity), "'$entity' database table found.");
   

   all ones inside one function in kernel test, I thought they needs common method or variable as @alexpott pointed for old function

   But then it's clear that modules [un]install rebuilds container so we can't store nether schema no Database service so fine

And better to have this comment instead of check

Oh this functions were deprecated on 8.0.x so revert version

annotation better to stick per method to prevent collision with other patches

+  @trigger_error(
+    "Deprecated as of Drupal 8.0.x, will be removed in Drupal 9.0.0. Instead, get a database connection injected into your service from the container, get its schema driver, and call tableExists() on it. For example, \$injected_database->schema()->tableExists(\$table);",

The trigger message should be similar to the following one (https://www.drupal.org/core/deprecation).

@trigger_error('baz() is deprecated in Drupal 8.3.0 and will be removed before Drupal 9.0.0. Use \Drupal\Foo\Bar::baz() instead. See http://drupal.org/node/the-change-notice-nid.');

• The message should report the function that is deprecated
• The Drupal version should be 8.6.0, not 8.0.x
• The message should report the full-qualified name of the class to use
• Every other details should be given in a change record the message links

This functions were deprecated before 8.0, so we just add triggering error here

#33 +1
See #1894396: Mark db_*() wrappers in database.inc as @deprecated for 9.x

It would be great is there was a CR to link to be there does not appear to be. Re #32 is correct that the format of the deprecation message is not quite right. See the other deprecation in this file:
@trigger_error('db_set_active() is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Use \Drupal\Core\Database\Database::setActiveConnection() instead. See https://www.drupal.org/node/2944084.', E_USER_DEPRECATED);
So let's take the opportunity to repurpose the linked CR to be about all the deprecated methods in database.inc and then link it from this @trigger_error. That will help all the related issues too.

Added CR
https://www.drupal.org/node/2947929

Changed deprecation message.

Sure it needs broader change record about all db_* functions cos it's missing since 8.0 release

I filed #2947946: Create change record for all deprecated db_* functions to identify main troubles on a way to iterate with db layer, actually 2 blockers - missing CR & performance impact from additional @trigger_error, bonus is "default target" #2947775: Move setting default target out of db_merge() and other deprecated db_* functions

Reverted changes in SchemaTest. See #2987856: Followup: Update Schema test.
Rerolled.

Looks great to go, but needs one more pair of eye on it

Second pair of eyes to confirm.

I tested this with $ ag "db_table_exists" and the only remaining mentions are in database.inc and in the RegressionTest.

Giving issue credit to all comments that helped move this issue along.

Also committed to 8.6.x as there is no risk in the single runtime change and I think maintaining out-of-sync tests for the 8.6.x / 8.7.x cycle is more costly.

Committed and pushed 27bc1f74cb to 8.7.x and 6173d84367 to 8.6.x. Thanks!

I find confusing to use the RegressionTest class for deprecation tests. Here the testDbTableExist method has been changed to track deprecation, which means AFAICS that it will be dropped in 9.0 when the wrappers will be remived frin code. But that test is pre-existing and should not be dropped.

I suggest adding a LegacyTest class for deprecation tests only, and move the tests already coded there.

@mondrake nice spot - can you create a issue to do that?

@mondrake actually only the methods marked with @legacy will be dropped and I think that is okay since \Drupal\KernelTests\Core\Database\SchemaTest tests the actual methods. I agree we need to be careful not to remove \Drupal\KernelTests\Core\Database\RegressionTest::testRegression_310447 but that is not marked with @legacy. So moving the other methods to a legacy test is probably a good idea but what's happened here is not wrong - it just can be better.

Looks like IS of #2848161: [meta] Replace calls to deprecated db_*() wrappers should be updated

#51/#52, opened issue #2991542: Introduce a DatabaseLegacyTest class for deprecation testing.

publish change record