See #2848161: [meta] Replace calls to deprecated db_*() wrappers

Marking as Novice as all these functions have no usage in core anymore, and writing a single patch similar to #2994556: Properly deprecate db_and, db_condition, db_or, db_xor will do the job here.

Find existing references in code to these functions:
find . -type f \( -name '*.inc' -or -name '*.module' -or -name '*.php' \) -exec grep -E "db_driver|db_escape_field|db_escape_table|db_rename_table|db_drop_index|db_drop_unique_key|db_add_unique_key|db_drop_primary_key|db_add_primary_key" {} \+

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

mondrake’s picture

Issue summary: View changes

Using the grep statement in IS, this is the current situation:

./core/includes/database.inc:function db_escape_table($table) {
./core/includes/database.inc:function db_escape_field($field) {
./core/includes/database.inc:function db_driver() {
./core/includes/database.inc:function db_rename_table($table, $new_name) {
./core/includes/database.inc:function db_add_primary_key($table, $fields) {
./core/includes/database.inc:function db_drop_primary_key($table) {
./core/includes/database.inc:function db_add_unique_key($table, $name, $fields) {
./core/includes/database.inc:function db_drop_unique_key($table, $name) {
./core/includes/database.inc:function db_drop_index($table, $name) {
./core/includes/database.inc: * db_drop_primary_key('foo');
./core/lib/Drupal/Core/Database/Schema.php:   * db_drop_primary_key('foo');
./core/lib/Drupal/Core/Database/Query/TableSortExtender.php:      // Based on code from db_escape_table(), but this can also contain a dot.

So there are a couple of comments in Schema.php and TableSortExtender.php to be fixed, too.

hardikpandya’s picture

Assigned: Unassigned » hardikpandya
hardikpandya’s picture

Assigned: hardikpandya » Unassigned
Status: Active » Needs review
FileSize
7.58 KB
longwave’s picture

Status: Needs review » Needs work

This looks pretty good, but I think each deprecation needs tests adding in DatabaseLegacyTest.

+++ b/core/lib/Drupal/Core/Database/Query/TableSortExtender.php
@@ -38,7 +38,8 @@ public function orderByHeader(array $header) {
+      // Based on code from \Drupal::database()->schema()->escapeTable($table),
+      // but this can also contain a dot.

This is not right: escapeTable() belongs to the database and not the schema, but as this is just a documentation reference rather than a code example, it is probably better overall to say \Drupal\Core\Database\Connection::escapeTable() here.

Hardik Rawal’s picture

Assigned: Unassigned » Hardik Rawal
Hardik Rawal’s picture

Assigned: Hardik Rawal » Unassigned
Status: Needs work » Needs review
FileSize
712 bytes
mondrake’s picture

Status: Needs review » Needs work
mondrake’s picture

Issue tags: +Needs tests

NW to add tests as per #5.

mitrpaka’s picture

Tests added.

mondrake’s picture

Status: Needs review » Needs work
  1. +++ b/core/includes/database.inc
    @@ -364,6 +364,7 @@ function db_set_active($key = 'default') {
    +  @trigger_error('db_escape_table() 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 escapeTable() on it. For example, $injected_database->escapeTable($table). See https://www.drupal.org/node/2993033.', E_USER_DEPRECATED);
    

    Pls remove the final dot from the message string, see #2848137-66: Replace all calls to db_update, which is deprecated, also in the other methods and the corresponding tests,

  2. +++ b/core/tests/Drupal/KernelTests/Core/Database/DatabaseLegacyTest.php
    @@ -206,4 +206,90 @@ public function testDbMerge() {
    +  public function testDbDriver() {
    +    $this->assertTrue(db_driver());
    +  }
    

    assertNotNull, the function returns a string

  3. +++ b/core/tests/Drupal/KernelTests/Core/Database/DatabaseLegacyTest.php
    @@ -206,4 +206,90 @@ public function testDbMerge() {
    +  public function testDbEscapeField() {
    +    $this->assertTrue(db_escape_field('test'));
    +  }
    

    same

  4. +++ b/core/tests/Drupal/KernelTests/Core/Database/DatabaseLegacyTest.php
    @@ -206,4 +206,90 @@ public function testDbMerge() {
    +  public function testDbEscapeTable() {
    +    $this->assertTrue(db_escape_table('test'));
    +  }
    

    same

  5. +++ b/core/tests/Drupal/KernelTests/Core/Database/DatabaseLegacyTest.php
    @@ -206,4 +206,90 @@ public function testDbMerge() {
    +  public function testDbDropIndex() {
    +    $schema = drupal_get_module_schema('database_test', 'test');
    +    $this->connection->schema()->addIndex('test', 'job', ['job'], $schema);
    +    $this->assertTrue(db_drop_index('test', 'job'));
    +  }
    

    it's sufficient to assertFalse on dropping a non-existent index

Vidushi Mehta’s picture

Status: Needs work » Needs review
FileSize
13.41 KB

Rerolled #10 as the patch does not apply and with the changes mentioned by #11.

mondrake’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests
+++ b/core/tests/Drupal/KernelTests/Core/Database/DatabaseLegacyTest.php
@@ -208,6 +208,92 @@ public function testDbMerge() {
+  public function testDbDropIndex() {
+    $schema = drupal_get_module_schema('database_test', 'test');
+    $this->connection->schema()->addIndex('test', 'job', ['job'], $schema);
+    $this->assertFalse(db_drop_index('test', 'job'));
+  }

just:

  public function testDbDropIndex() {
    $this->assertFalse(db_drop_index('test', 'no_such_index'));
  }

Please add interdiffs when posting new patches on top of previous ones.

mitrpaka’s picture

Status: Needs work » Needs review
FileSize
13.08 KB
8.37 KB
mondrake’s picture

Status: Needs review » Reviewed & tested by the community

LGTM, thank you

  • catch committed 617d5b7 on 8.7.x
    Issue #2994694 by mitrpaka, hardik.p, Vidushi Mehta, Hardik Rawal,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

The patch was changing permissions on the files, but fixed this on commit. Thanks!

Checking changed files...
git pre-commit check failed: file core/includes/database.inc should be 644 not 755
PHPCS: core/includes/database.inc passed
git pre-commit check failed: file core/lib/Drupal/Core/Database/Query/TableSortExtender.php should be 644 not 755
PHPCS: core/lib/Drupal/Core/Database/Query/TableSortExtender.php passed
git pre-commit check failed: file core/lib/Drupal/Core/Database/Schema.php should be 644 not 755
PHPCS: core/lib/Drupal/Core/Database/Schema.php passed
git pre-commit check failed: file core/tests/Drupal/KernelTests/Core/Database/DatabaseLegacyTest.php should be 644 not 755
PHPCS: core/tests/Drupal/KernelTests/Core/Database/DatabaseLegacyTest.php passed
andypost’s picture

Filed follow-up #2996436: Follow-up to fix DatabaseLegacyTest::testDbRenameTable to fix broken tests on sqlite/pgsql

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.