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

Replace all usages of db_insert in core, except in code that is testing db_insert.

CommentFileSizeAuthor
#88 2853118-88.patch35.25 KBlongwave
#85 2853118-85.patch35.2 KBvoleger
#82 2853118-82.patch35.2 KBvoleger
#78 interdiff-2853118-72-78.patch489 bytesvoleger
#78 2853118-78.patch35.21 KBvoleger
#72 interdiff-2853118-69-72.txt1.15 KBjofitz
#72 2853118-72.patch35 KBjofitz
#69 interdiff-2853118-68-69.txt761 bytesvoleger
#69 2853118-69.patch35.22 KBvoleger
#68 interdiff-64-68.txt1.13 KBjofitz
#68 2853118-68.patch34.83 KBjofitz
#64 interdiff-2853118-63-64.txt5.22 KBvoleger
#64 2853118-64.patch35.29 KBvoleger
#63 2853118-63.patch34.22 KBandypost
#63 interdiff.txt15.16 KBandypost
#61 2853118-61.patch32.28 KBjofitz
#61 interdiff-2853118-60-61.txt1.69 KBjofitz
#60 2853118-60.patch33.97 KBjofitz
#54 interdiff-2853118-52-54.txt2.52 KBvoleger
#54 2853118-54.patch35.78 KBvoleger
#52 interdiff-2853118-50-52.txt667 bytesvoleger
#52 2853118-52.patch36.57 KBvoleger
#50 2853118-50.patch36.57 KBvoleger
#50 interdiff-2853118-48-50.txt741 bytesvoleger
#48 interdiff-2853118-43-48.txt2.75 KBvoleger
#48 2853118-48.patch36.56 KBvoleger
#43 2853118-43.patch33.37 KBjofitz
#43 interdiff-2853118-40-43.txt16.55 KBjofitz
#40 2853118-40.patch34.03 KBjofitz
#40 interdiff-2853118-38-40.txt1.55 KBjofitz
#38 2853118-38.patch34 KBjofitz
#36 2853118-36.patch36.25 KBjofitz
#32 replace_all_db_insert_calls-2853118-32-8.6.x.patch38.5 KBvoleger
#29 insert.png65.81 KBpk188
#29 replace_all_calls_to-2853118-28.patch38.17 KBpk188
#27 replace_all_calls_to-2853118-27.patch39.19 KBpk188
#26 interdiff.txt1.03 KBjeetendrakumar
#26 replace_all_calls_to-2853118-26.patch34.28 KBjeetendrakumar
#25 replace_all_calls_to-2853118-25.patch33.27 KBhgunicamp
#25 interdiff-2853118-25.txt1.67 KBhgunicamp
#24 DrupalDb.png26.14 KBrenatog
#23 replace_all_calls_to-2853118-23.patch35.25 KBhgunicamp
#23 interdiff-2853118-23.txt1.36 KBhgunicamp
#19 interdiff-2853118-17-19.txt3.99 KByogeshmpawar
#19 replace_all_calls_to-2853118-19.patch34.7 KByogeshmpawar
#17 interdiff-2853118-13-17.txt669 bytesyogeshmpawar
#17 replace_all_calls_to-2853118-17.patch37.04 KByogeshmpawar
#14 interdiff1.txt563 bytesPavan B S
#9 drupal-replace-calls-to-db_insert-2853118-8.patch37.07 KBgaurav.kapoor
#6 interdiff.txt26.28 KBPavan B S
#6 db_insert-2853118-4.patch37.75 KBPavan B S
#4 db_insert-2853118-2.patch8.34 KBPavan B S
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dhruveshdtripathi created an issue. See original summary.

dhruveshdtripathi’s picture

Assigned: Unassigned » dhruveshdtripathi
Pavan B S’s picture

WIll be working on this issue.

Pavan B S’s picture

Assigned: Pavan B S » Unassigned
Status: Active » Needs review
FileSize
8.34 KB

Applying the patch, please Review

daffie’s picture

Status: Needs review » Needs work

You forgot to replace some instances. If I apply your patch and run the commend grep -r "db_insert" * I get the following result:

core/includes/database.inc:function db_insert($table, array $options = array()) {
core/modules/system/tests/modules/module_test/module_test.install:  db_insert('module_test')
core/modules/file/tests/src/Kernel/UsageTest.php:    db_insert('file_usage')
core/modules/file/tests/src/Kernel/UsageTest.php:    db_insert('file_usage')
core/modules/file/tests/src/Kernel/UsageTest.php:    db_insert('file_usage')
core/modules/search/src/Tests/SearchRankingTest.php:    db_insert('node_counter')
core/modules/history/src/Tests/Views/HistoryTimestampTest.php:    db_insert('history')
core/modules/history/src/Tests/Views/HistoryTimestampTest.php:    db_insert('history')
core/modules/simpletest/simpletest.module:  $test_id = db_insert('simpletest_test_id')
core/modules/contact/tests/drupal-7.contact.database.php:db_insert('contact')->fields(array(
core/modules/node/src/Tests/NodeQueryAlterTest.php:    db_insert('node_access')->fields($record)->execute();
core/modules/node/tests/src/Functional/NodeAccessGrantsCacheContextTest.php:   db_insert('node_access')->fields($record)->execute();
core/modules/views/src/Tests/ViewTestBase.php:    $query = db_insert('views_test_data')
core/modules/views/src/Tests/ViewKernelTestBase.php:    $query = db_insert('views_test_data')
core/modules/views/src/Tests/Plugin/StyleTableTest.php:    $query = db_insert('views_test_data')
core/modules/views/tests/src/Kernel/Plugin/CacheTest.php:    db_insert('views_test_data')->fields($record)->execute();
core/modules/views/tests/src/Kernel/Plugin/CacheTest.php:    db_insert('views_test_data')->fields($record)->execute();
core/modules/locale/src/Tests/LocaleUpdateBase.php:      db_insert('locale_file')->fields($file)->execute();
core/lib/Drupal/Core/Database/Query/Upsert.php:        $values[':db_insert_placeholder_' . $max_placeholder++] = $value;
core/lib/Drupal/Core/Database/Query/InsertTrait.php:          $placeholders[] = ':db_insert_placeholder_' . $i;
core/lib/Drupal/Core/Database/Driver/mysql/Insert.php:          $values[':db_insert_placeholder_' . $max_placeholder++] = $value;
core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php:          $stmt->bindParam(':db_insert_placeholder_' . $max_placeholder++, $blobs[$blob_count], \PDO::PARAM_LOB);
core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php:          $stmt->bindParam(':db_insert_placeholder_' . $max_placeholder++, $insert_values[$idx]);
core/lib/Drupal/Core/Database/Driver/pgsql/NativeUpsert.php:          $stmt->bindParam(':db_insert_placeholder_' . $max_placeholder++, $blobs[$blob_count], \PDO::PARAM_LOB);
core/lib/Drupal/Core/Database/Driver/pgsql/NativeUpsert.php:          $stmt->bindParam(':db_insert_placeholder_' . $max_placeholder++, $insert_values[$idx]);
core/tests/Drupal/KernelTests/Core/Database/SelectTest.php:      db_insert('test')->fields(array('name' => $this->randomMachineName()))->execute();
core/tests/Drupal/KernelTests/Core/Database/InsertLobTest.php:    $id = db_insert('test_one_blob')
core/tests/Drupal/KernelTests/Core/Database/InsertLobTest.php:    $id = db_insert('test_two_blobs')
core/tests/Drupal/KernelTests/Core/Database/CaseSensitivityTest.php:    db_insert('test')
core/tests/Drupal/KernelTests/Core/Database/UpdateLobTest.php:    $id = db_insert('test_one_blob')
core/tests/Drupal/KernelTests/Core/Database/UpdateLobTest.php:    $id = db_insert('test_two_blobs')
core/tests/Drupal/KernelTests/Core/Database/SelectSubqueryTest.php:    db_inser('test_people')
core/tests/Drupal/KernelTests/Core/Database/SelectSubqueryTest.php:    db_inser('test_people')
core/tests/Drupal/KernelTests/Core/Database/InvalidDataTest.php:      db_insert('test')
core/tests/Drupal/KernelTests/Core/Database/QueryTest.php:    db_insert('test')
core/tests/Drupal/KernelTests/Core/Database/QueryTest.php:    db_insert('TEST_UPPERCASE')
core/tests/Drupal/KernelTests/Core/Database/DatabaseTestBase.php:    db_insert('test_null')
core/tests/Drupal/KernelTests/Core/Database/DatabaseTestBase.php:    $john = db_insert('test')
core/tests/Drupal/KernelTests/Core/Database/DatabaseTestBase.php:    $george = db_insert('test')
core/tests/Drupal/KernelTests/Core/Database/DatabaseTestBase.php:    db_insert('test')
core/tests/Drupal/KernelTests/Core/Database/DatabaseTestBase.php:    $paul = db_insert('test')
core/tests/Drupal/KernelTests/Core/Database/DatabaseTestBase.php:    db_insert('test_people')
core/tests/Drupal/KernelTests/Core/Database/DatabaseTestBase.php:    db_insert('test_task')
core/tests/Drupal/KernelTests/Core/Database/DatabaseTestBase.php:    db_insert('test_special_columns')
core/tests/Drupal/KernelTests/Core/Database/InsertDefaultsTest.php:    $query =db_insert('test')->useDefaults(array('job'));
core/tests/Drupal/KernelTests/Core/Database/InsertDefaultsTest.php:      db_insert('test')->execute();
core/tests/Drupal/KernelTests/Core/Database/InsertDefaultsTest.php:    $query =db_insert('test')
core/tests/Drupal/KernelTests/Core/Database/BasicSyntaxTest.php:    db_insert('test')
core/tests/Drupal/KernelTests/Core/Database/BasicSyntaxTest.php:    db_insert('test')
core/tests/Drupal/KernelTests/Core/Database/RegressionTest.php:    db_insert('test')
core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php:      db_insert($table)
core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php:      db_insert($table_name)
core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php:      db_insert($table_name)
core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php:      $id = db_inser($table_name)
core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php:      $id = db_inser($table_name)
core/tests/Drupal/KernelTests/Core/Database/InsertTest.php:    $query = db_insert('test');
core/tests/Drupal/KernelTests/Core/Database/InsertTest.php:    $query = db_insert('test');
core/tests/Drupal/KernelTests/Core/Database/InsertTest.php:    $query = db_insert('test');
core/tests/Drupal/KernelTests/Core/Database/InsertTest.php:    db_insert('test')
core/tests/Drupal/KernelTests/Core/Database/InsertTest.php:    $id = db_insert('test')
core/tests/Drupal/KernelTests/Core/Database/InsertTest.php:    db_insert('test')
core/tests/Drupal/KernelTests/Core/Database/InsertTest.php:    db_insert('test_people_copy')
core/tests/Drupal/KernelTests/Core/Database/InsertTest.php:    $id = db_insert('test_special_columns')
core/tests/Drupal/KernelTests/Core/Database/TransactionTest.php:    db_insert('test')
core/tests/Drupal/KernelTests/Core/Database/TransactionTest.php:    db_insert('test')
core/tests/Drupal/KernelTests/Core/Database/TransactionTest.php:    db_insert('test')
core/tests/Drupal/KernelTests/Core/Entity/FieldSqlStorageTest.php:    $query = db_insert($this->revisionTable)->fields($columns);
core/tests/Drupal/KernelTests/Core/Entity/FieldSqlStorageTest.php:    $query = db_insert($this->table)->fields($columns);
core/tests/Drupal/KernelTests/Core/Entity/FieldSqlStorageTest.php:    db_insert($this->table)->fields($columns)->values($values)->execute();
core/tests/Drupal/KernelTests/Core/Entity/FieldSqlStorageTest.php:    db_insert($this->revisionTable)->fields($columns)->values($values)->execute();
core/tests/Drupal/KernelTests/Core/Config/Storage/DatabaseStorageTest.php:    db_insert('config')->fields(array('name' => $name, 'data' => $data))->execute();
Pavan B S’s picture

Status: Needs work » Needs review
FileSize
37.75 KB
26.28 KB

Updating the patch as per the comment #5.Please review the patch

Status: Needs review » Needs work

The last submitted patch, 6: db_insert-2853118-4.patch, failed testing.

Pavan B S’s picture

Status: Needs work » Needs review
FileSize
37.7 KB
675 bytes

Applying the patch, please review.

gaurav.kapoor’s picture

The last submitted patch, , failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: drupal-replace-calls-to-db_insert-2853118-8.patch, failed testing.

daffie’s picture

@Pavan_B_S: You are using an old version of Drupal core. The following file has been moved (https://api.drupal.org/api/drupal/core%21modules%21node%21tests%21src%21...).

@gaurav.kapoor: You are using double round brackets. That is why you have a test failure:

+++ b/core/tests/Drupal/KernelTests/Core/Database/InsertTest.php
@@ -136,8 +136,8 @@ function testInsertFieldOnlyDefinition() {
+    $id = \Drupal::database()->insert(('test'));
dhruveshdtripathi’s picture

Status: Needs work » Needs review
FileSize
37.07 KB

Removed double round brackets.

Pavan B S’s picture

FileSize
563 bytes

@daffie thanks for the comment , i cloned the latest drupal core. I am uploading the interdiff for the #13

The last submitted patch, , failed testing.

daffie’s picture

Status: Needs review » Needs work

There was an other bug:

+++ b/core/tests/Drupal/KernelTests/Core/Database/InsertTest.php
@@ -136,8 +136,8 @@ function testInsertFieldOnlyDefinition() {
+    $id = \Drupal::database()->insert('test');
+    $id ->fields(array(

There is a space between "$id" and "->fields(array(". That is wrong. Can we change it to:

  $id = \Drupal::database()->insert('test')
    ->fields(array(
yogeshmpawar’s picture

Status: Needs work » Needs review
FileSize
37.04 KB
669 bytes

Patch updated as per comment #16 & also added interdiff.

daffie’s picture

Status: Needs review » Needs work

The patch looks good, but I have some remarks:

  1. diff --git a/core/includes/database.inc b/core/includes/database.inc
    index ff03885..044a09b 100644
    --- a/core/includes/database.inc
    +++ b/core/includes/database.inc
    @@ -26,7 +26,7 @@
      * instead.
      *
      * Do not use this function for INSERT, UPDATE, or DELETE queries. Those should
    - * be handled via db_insert(), db_update() and db_delete() respectively.
    + * be handled via \Drupal::database()->insert(), db_update() and db_delete() respectively.
      *
      * @param string|\Drupal\Core\Database\StatementInterface $query
      *   The prepared statement query to run. Although it will accept both named and
    diff --git a/core/lib/Drupal/Core/Database/database.api.php b/core/lib/Drupal/Core/Database/database.api.php
    index 07e4343..1a767b6 100644
    --- a/core/lib/Drupal/Core/Database/database.api.php
    +++ b/core/lib/Drupal/Core/Database/database.api.php
    @@ -124,11 +124,11 @@
      * @section_insert INSERT, UPDATE, and DELETE queries
      * INSERT, UPDATE, and DELETE queries need special care in order to behave
      * consistently across databases; you should never use db_query() to run
    - * an INSERT, UPDATE, or DELETE query. Instead, use functions db_insert(),
    + * an INSERT, UPDATE, or DELETE query. Instead, use functions \Drupal::database()->insert(),
      * db_update(), and db_delete() to obtain a base query on your table, and then
      * add dynamic conditions (as illustrated in @ref sec_dynamic above).
      *
    - * As a note, db_insert() and similar functions are wrappers on connection
    + * As a note, \Drupal::database()->insert() and similar functions are wrappers on connection
      * object methods. In most classes, you should use dependency injection and the
      * database connection object instead of these wrappers; See @ref sec_connection
      * below for details.
    @@ -140,7 +140,7 @@
      * You can execute it via:
      * @code
      * $fields = array('id' => 1, 'uid' => 2, 'path' => 'path', 'name' => 'Name');
    - * db_insert('example')
    + * \Drupal::database()->insert('example')
      *   ->fields($fields)
      *   ->execute();
      * @endcode
    @@ -162,7 +162,7 @@
      *   $txn = db_transaction();
      *
      *   try {
    - *     $id = db_insert('example')
    + *     $id = \Drupal::database()->insert('example')
      *       ->fields(array(
      *         'field1' => 'mystring',
      *         'field2' => 5,
    ...
    diff --git a/core/modules/node/node.api.php b/core/modules/node/node.api.php
    index a4937f7..b96e010 100644
    --- a/core/modules/node/node.api.php
    +++ b/core/modules/node/node.api.php
    @@ -51,7 +51,7 @@
      *   'grant_update' => 0,
      *   'grant_delete' => 0,
      * );
    - * db_insert('node_access')->fields($record)->execute();
    + * \Drupal::database()->insert('node_access')->fields($record)->execute();
      * @endcode
      * And then in its hook_node_grants() implementation, it would need to return:
      * @code
    

    The changes in the documentation will be done in #2849745: Replace documentation recommending db_*() wrappers.

  2. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
    @@ -938,7 +938,7 @@ function hook_ENTITY_TYPE_presave(Drupal\Core\Entity\EntityInterface $entity) {
    @@ -947,7 +947,6 @@ function hook_entity_insert(Drupal\Core\Entity\EntityInterface $entity) {
    
    @@ -947,7 +947,6 @@ function hook_entity_insert(Drupal\Core\Entity\EntityInterface $entity) {
         ))
         ->execute();
     }
    -
     /**
      * Respond to creation of a new entity of a particular type.
      *
    
    @@ -962,7 +961,7 @@ function hook_entity_insert(Drupal\Core\Entity\EntityInterface $entity) {
    @@ -970,7 +969,6 @@ function hook_ENTITY_TYPE_insert(Drupal\Core\Entity\EntityInterface $entity) {
    
    @@ -970,7 +969,6 @@ function hook_ENTITY_TYPE_insert(Drupal\Core\Entity\EntityInterface $entity) {
         ))
         ->execute();
     }
    -
     /**
      * Respond to updates to an entity.
      *
    

    Please do not remove these lines.

  3. There is still one instance that needs to be changed in Drupal\Tests\node\Functional\NodeAccessGrantsCacheContextTest.
yogeshmpawar’s picture

Status: Needs work » Needs review
FileSize
34.7 KB
3.99 KB

Thanks @daffie, made changes as per remarks, also added a interdiff.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

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

xjm’s picture

Status: Reviewed & tested by the community » Needs work

The summary says:

Replace all usages of db_insert in core, except in code that is testing db_insert.

A significant portion of this patch is in database tests. InsertTest is the self-tests for this function. I also wonder about whether it is appropriate to replace the other usages in DB tests with this or a different replacement. All the non-DB test and non-test replacements look good though.

Thanks!

yogeshmpawar’s picture

Thanks @xjm for the comment, does that mean i need to remove "core/tests/Drupal/KernelTests/Core/Database/InsertTest.php" this file or all the files in "core/tests/Drupal/KernelTests/Core/Database"

hgunicamp’s picture

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

renatog’s picture

hgunicamp’s picture

I'm replacing the 'replace_all_calls_to-2853118-23.patch' patch by 'replace_all_calls_to-2853118-25.patch ' one because it was changing the files 'core/scripts/dump-database-d6.sh', 'core/scripts/dump-database-d7.sh' and 'core/scripts/generate-d7-content.sh'.

This files should stay as they are because they should work with Drupal 6 and 7.
See #2232391.

jeetendrakumar’s picture

pk188’s picture

Patch in #26 didn't apply and there are changes in some other files too. So made all the changes in patch in #27.

Status: Needs review » Needs work

The last submitted patch, 27: replace_all_calls_to-2853118-27.patch, failed testing.

pk188’s picture

Status: Needs work » Needs review
FileSize
38.17 KB
65.81 KB

Changed the code in picture.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

voleger’s picture

voleger’s picture

Title: Replace all calls to db_insert, which is deprecated » [PP-1] Replace all calls to db_insert, which is deprecated
Status: Needs review » Postponed
Related issues: +#2953385: Add a $connection property to DatabaseTestBase kernel test class to be used by extending classes

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

voleger’s picture

Title: [PP-1] Replace all calls to db_insert, which is deprecated » Replace all calls to db_insert, which is deprecated
Status: Postponed » Needs work
Issue tags: +Needs reroll
jofitz’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
36.25 KB

Re-rolled against 8.7.x.

Status: Needs review » Needs work

The last submitted patch, 36: 2853118-36.patch, failed testing. View results

jofitz’s picture

Status: Needs work » Needs review
FileSize
34 KB

Re-rolled against latest 8.7.x.

voleger’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Database/database.api.php
@@ -126,11 +126,11 @@
+ * an INSERT, UPDATE, or DELETE query. Instead, use functions \Drupal::database()->insert(),
...
+ * As a note, \Drupal::database()->insert() and similar functions are wrappers on connection

Need to fix the comments length.

jofitz’s picture

Status: Needs work » Needs review
FileSize
1.55 KB
34.03 KB

Corrected line lengths.

voleger’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/simpletest/simpletest.module
    @@ -138,7 +138,7 @@ function simpletest_run_tests($test_list) {
    +  $test_id = \Drupal::database()->insert('simpletest_test_id')
    

    We can use here Database::getConnection()

  2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/FieldSqlStorageTest.php
    @@ -123,7 +123,7 @@ public function testFieldLoad() {
    +    $query = \Drupal::database()->insert($this->revisionTable)->fields($columns);
    
    @@ -133,7 +133,7 @@ public function testFieldLoad() {
    +    $query = \Drupal::database()->insert($this->table)->fields($columns);
    
    @@ -167,8 +167,8 @@ public function testFieldLoad() {
    +    \Drupal::database()->insert($this->table)->fields($columns)->values($values)->execute();
    +    \Drupal::database()->insert($this->revisionTable)->fields($columns)->values($values)->execute();
    

    Also here we can use Database::getConnection()

  3. in Drupal\KernelTests\Core\Database namespace we should use $this->connection. See Drupal\KernelTests\Core\Database\DatabaseTestBase
voleger’s picture

+++ b/core/tests/Drupal/KernelTests/Core/Database/RegressionTest.php
@@ -24,7 +24,7 @@ class RegressionTest extends DatabaseTestBase {
-    db_insert('test')
+    \Drupal::database()->insert('test')

I guess we should not touch Regression tests.

jofitz’s picture

Status: Needs work » Needs review
FileSize
16.55 KB
33.37 KB

Addressed the requested changes in #41 and #42.

voleger’s picture

Status: Needs review » Reviewed & tested by the community

Looks good
+1 for RTBC

The last submitted patch, 32: replace_all_db_insert_calls-2853118-32-8.6.x.patch, failed testing. View results

mondrake’s picture

Status: Reviewed & tested by the community » Needs work

There are no more calls to db_insert in the codebase, good.

At this stage I think we should also add a @trigger_error for the deprecation in the db_insert function in database.inc, and relative CR.

Also, in the docblock of the db_query function there is one last stray reference to db_insert.

voleger’s picture

Status: Needs work » Needs review
FileSize
36.56 KB
2.75 KB

Status: Needs review » Needs work

The last submitted patch, 48: 2853118-48.patch, failed testing. View results

voleger’s picture

Status: Needs work » Needs review
FileSize
741 bytes
36.57 KB

Status: Needs review » Needs work

The last submitted patch, 50: 2853118-50.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

voleger’s picture

Status: Needs work » Needs review
FileSize
36.57 KB
667 bytes
mondrake’s picture

  1. +++ b/core/lib/Drupal/Core/Database/database.api.php
    @@ -126,14 +126,15 @@
    - * As a note, db_insert() and similar functions are wrappers on connection
    - * object methods. In most classes, you should use dependency injection and the
    - * database connection object instead of these wrappers; See @ref sec_connection
    - * below for details.
    + * As a note, \Drupal::database()->insert() and similar functions are wrappers
    + * on connection object methods. In most classes, you should use dependency
    + * injection and the database connection object instead of these wrappers; See
    + * @ref sec_connection below for details.
    

    This one I would leave unchanged - it's about what the db* wrappers are - the entire statement will have to be removed at later stage when db* functions are actually removed.

  2. +++ b/core/modules/simpletest/simpletest.module
    @@ -138,7 +138,7 @@ function simpletest_run_tests($test_list) {
    -  $test_id = db_insert('simpletest_test_id')
    +  $test_id = Database::getConnection()->insert('simpletest_test_id')
    

    Can't we use \Drupal::database()->insert like in all other cases with non-test code?

  3. +++ b/core/tests/Drupal/KernelTests/Core/Database/RegressionTest.php
    @@ -82,4 +81,27 @@ public function testDbDropTable() {
    +  /**
    +   * Tests the db_insert() function.
    +   *
    +   * @group legacy
    +   *
    +   * @expectedDeprecation db_insert() is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Use \Drupal\Core\Database\Database::getConnection()->insert() instead. See https://www.drupal.org/node/2989742
    +   */
    +  public function testDbInsert() {
    +    $job = str_repeat("é", 255);
    +    db_insert('test')
    +      ->fields([
    +        'name' => $this->randomMachineName(),
    +        'age' => 20,
    +        'job' => $job,
    +      ])->execute();
    +
    +    $from_database = $this->connection->select('test', 't');
    +    $from_database->fields('t', ['job']);
    +    $from_database->condition('job', $job);
    +    $from_database = $from_database->execute()->fetchField();
    +    $this->assertSame($job, $from_database, 'The database handles UTF-8 characters cleanly.');
    +  }
    +
    

    This is rather complicated for a simple deprecation test, why not sth simpler like

        $this->assertNotNull(db_insert('test')->fields(['name' => 'Yoko', 'age' => '29']->execute());
    
voleger’s picture

Status: Needs work » Needs review
FileSize
35.78 KB
2.52 KB

Addressed #53

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

looks good

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Database/database.api.php
    @@ -126,9 +126,10 @@
    + * an INSERT, UPDATE, or DELETE query. Instead, use functions
    + * \Drupal::database()->insert(), db_update(), and db_delete() to obtain a base
    + * query on your table, and then add dynamic conditions (as illustrated in @ref
    + * sec_dynamic above).
    

    I would fix db_update() and db_delete() here at the same time. To keep it consistent. In fact I might just have a blocking issue to update this one file to ensure it is not using any deprecated methods.

  2. +++ b/core/lib/Drupal/Core/Database/database.api.php
    @@ -126,9 +126,10 @@
      * As a note, db_insert() and similar functions are wrappers on connection
    

    Still documenting db_insert().

  3. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
    @@ -1081,7 +1081,7 @@ function hook_ENTITY_TYPE_presave(Drupal\Core\Entity\EntityInterface $entity) {
    +  \Drupal::database()->insert('example_entity')
    
    @@ -1105,7 +1105,7 @@ function hook_entity_insert(Drupal\Core\Entity\EntityInterface $entity) {
    +  \Drupal::database()->insert('example_entity')
    
    +++ b/core/modules/contact/tests/drupal-7.contact.database.php
    @@ -16,7 +16,7 @@
    +\Drupal::database()->insert('contact')->fields([
    
    +++ b/core/modules/file/tests/src/Kernel/UsageTest.php
    @@ -21,7 +21,7 @@ class UsageTest extends FileManagedUnitTestBase {
    +    \Drupal::database()->insert('file_usage')
    
    @@ -104,7 +104,7 @@ public function doTestRemoveUsage() {
    +    \Drupal::database()->insert('file_usage')
    
    +++ b/core/modules/history/tests/src/Kernel/Views/HistoryTimestampTest.php
    @@ -67,14 +67,14 @@ public function testHandlers() {
    +    \Drupal::database()->insert('history')
    ...
    +    \Drupal::database()->insert('history')
    
    +++ b/core/modules/locale/tests/src/Functional/LocaleUpdateBase.php
    @@ -280,7 +280,7 @@ protected function setCurrentTranslations() {
    +      \Drupal::database()->insert('locale_file')->fields($file)->execute();
    
    +++ b/core/modules/node/node.api.php
    @@ -51,7 +51,7 @@
    + * \Drupal::database()->insert('node_access')->fields($record)->execute();
    
    +++ b/core/modules/node/node.install
    @@ -134,7 +134,7 @@ function node_install() {
    +  \Drupal::database()->insert('node_access')
    
    +++ b/core/modules/node/tests/src/Functional/NodeAccessGrantsCacheContextTest.php
    @@ -92,7 +92,7 @@ public function testCacheContext() {
    +    \Drupal::database()->insert('node_access')->fields($record)->execute();
    
    +++ b/core/modules/node/tests/src/Functional/NodeQueryAlterTest.php
    @@ -151,7 +151,7 @@ public function testNodeQueryAlterOverride() {
    +    \Drupal::database()->insert('node_access')->fields($record)->execute();
    
    +++ b/core/modules/path/path.api.php
    @@ -20,7 +20,7 @@
    +  \Drupal::database()->insert('mytable')
    
    +++ b/core/modules/search/search.module
    @@ -537,7 +537,7 @@ function search_index($type, $sid, $langcode, $text) {
    +  \Drupal::database()->insert('search_dataset')
    
    +++ b/core/modules/search/tests/src/Functional/SearchRankingTest.php
    @@ -104,7 +104,7 @@ public function testRankings() {
    +    \Drupal::database()->insert('node_counter')
    
    +++ b/core/modules/simpletest/simpletest.module
    @@ -138,7 +138,7 @@ function simpletest_run_tests($test_list) {
    +  $test_id = \Drupal::database()->insert('simpletest_test_id')
    
    +++ b/core/modules/system/tests/modules/module_test/module_test.install
    @@ -29,7 +29,7 @@ function module_test_schema() {
    +  \Drupal::database()->insert('module_test')
    
    +++ b/core/modules/tracker/tracker.module
    @@ -69,7 +69,7 @@ function tracker_cron() {
    +      \Drupal::database()->insert('tracker_node')
    
    @@ -78,7 +78,7 @@ function tracker_cron() {
    +      \Drupal::database()->insert('tracker_user')
    
    @@ -101,7 +101,7 @@ function tracker_cron() {
    +        $query = \Drupal::database()->insert('tracker_user');
    
    +++ b/core/modules/user/user.api.php
    @@ -155,7 +155,7 @@ function hook_user_login($account) {
    +  \Drupal::database()->insert('logouts')
    
    +++ b/core/modules/views/src/Tests/ViewKernelTestBase.php
    @@ -71,7 +71,7 @@ protected function setUpFixtures() {
    +    $query = \Drupal::database()->insert('views_test_data')
    
    +++ b/core/modules/views/src/Tests/ViewTestBase.php
    @@ -57,7 +57,7 @@ protected function enableViewsTestModule() {
    +    $query = \Drupal::database()->insert('views_test_data')
    
    +++ b/core/modules/views/tests/src/Functional/Plugin/StyleTableTest.php
    @@ -115,7 +115,7 @@ public function testNumericFieldVisible() {
    +    $query = \Drupal::database()->insert('views_test_data')
    
    +++ b/core/modules/views/tests/src/Functional/ViewTestBase.php
    @@ -55,7 +55,7 @@ protected function enableViewsTestModule() {
    +    $query = \Drupal::database()->insert('views_test_data')
    
    +++ b/core/modules/views/tests/src/Kernel/Plugin/CacheTest.php
    @@ -88,7 +88,7 @@ public function testTimeResultCaching() {
    +    \Drupal::database()->insert('views_test_data')->fields($record)->execute();
    
    @@ -242,7 +242,7 @@ public function testNoneResultCaching() {
    +    \Drupal::database()->insert('views_test_data')->fields($record)->execute();
    
    +++ b/core/tests/Drupal/KernelTests/Core/Entity/FieldSqlStorageTest.php
    @@ -133,7 +133,7 @@ public function testFieldLoad() {
    +    $query = Database::getConnection()->insert($this->table)->fields($columns);
    

    There's a general preference for using Database::getConnection() atm. Let's use that here instead of \Drupal::database().

  4. +++ b/core/scripts/dump-database-d6.sh
    @@ -90,7 +90,7 @@
    -    $output .= "db_insert('". $table . "')->fields(". drupal_var_export(array_keys($data['fields'])) .")\n";
    +    $output .= "\Drupal::database()->insert('". $table . "')->fields(". drupal_var_export(array_keys($data['fields'])) .")\n";
    
    +++ b/core/scripts/dump-database-d7.sh
    @@ -80,7 +80,7 @@
    -    $output .= "db_insert('". $table . "')->fields(". drupal_var_export(array_keys($data['fields'])) .")\n";
    +    $output .= "\Drupal::database()->insert('". $table . "')->fields(". drupal_var_export(array_keys($data['fields'])) .")\n";
    
    +++ b/core/scripts/generate-d7-content.sh
    @@ -45,7 +45,7 @@
    -$query = db_insert('users')->fields(array('uid', 'name', 'pass', 'mail', 'status', 'created', 'access'));
    +$query = \Drupal::database()->insert('users')->fields(array('uid', 'name', 'pass', 'mail', 'status', 'created', 'access'));
    

    These files are Drupal 6 and 7 code respectively and should not be changing.

alexpott’s picture

I realise that #53.2 and #56.3 are contradicting. Hmmm. I don't really know what's best to be honest. Both are used all over core. Database::getConnection() definitely involves less layers as it doesn't go through the container. Perhaps we should have an issue to discuss this and the way forward - unless it has been decided somewhere else already.

mondrake’s picture

voleger’s picture

jofitz’s picture

Re-roll of patch in #54 now that #2987399: Update database.api.php to remove deprecated db_*() functions. is in.

Still NW to address @alexpott's comments in #56.

jofitz’s picture

#53.1 and #53.2 are no longer relevant (after the completion of #2987399: Update database.api.php to remove deprecated db_*() functions.).

Reverted the changes mentioned in #53.4.

Now only need to address #53.3: Database::getConnection() v \Drupal::database(), therefore postponing until a decision is made in #2991337: Document the recommended ways to obtain the database connection object.

jofitz’s picture

Status: Needs work » Postponed
andypost’s picture

Status: Postponed » Needs review
FileSize
15.16 KB
34.22 KB

Reroll & fixes for according #2991337: Document the recommended ways to obtain the database connection object

- I'm sure RegressionTest.php does not need to add new case so moved to \Drupal\KernelTests\Core\Database\DatabaseLegacyTest::testDbInsert()
- Places in functional & kernel tests that have no connection protected property replaced with Database::getConnection
- Updated usage of \Drupal::database() used in loops or in the same function or method

voleger’s picture

andypost’s picture

+++ b/core/modules/system/tests/modules/module_test/module_test.install
@@ -31,7 +29,7 @@
-  Database::getConnection()->insert('module_test')
+  \Drupal::database()->insert('module_test')

I changed install hooks to use getConnection() because a lot of other placec using it and it is not good idea to rely on container in install hook

Status: Needs review » Needs work

The last submitted patch, 64: 2853118-64.patch, failed testing. View results

longwave’s picture

+++ b/core/includes/database.inc
@@ -155,6 +155,7 @@ function db_query_temporary($query, array $args = [], array $options = []) {
+  @trigger_error('db_insert() 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 call insert() on it. For example, $injected_database->insert($table, $options); See https://www.drupal.org/node/2993033', E_USER_DEPRECATED);

Duplicate word in "call call"

jofitz’s picture

Status: Needs work » Needs review
FileSize
34.83 KB
1.13 KB

Removed duplicate word.

voleger’s picture

The last submitted patch, 68: 2853118-68.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 69: 2853118-69.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jofitz’s picture

Removed duplicate word from expected result too and corrected coding standards error.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks.

andypost’s picture

queued tests for sqlite & pgsql

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/file/tests/src/Kernel/UsageTest.php
@@ -22,7 +22,8 @@ class UsageTest extends FileManagedUnitTestBase {
    */
   public function testGetUsage() {
     $file = $this->createFile();
-    db_insert('file_usage')
+    $connection = \Drupal::database();
+    $connection->insert('file_usage')
       ->fields([
         'fid' => $file->id(),
         'module' => 'testing',
@@ -31,7 +32,7 @@ public function testGetUsage() {

@@ -31,7 +32,7 @@ public function testGetUsage() {
         'count' => 1,
       ])
       ->execute();
-    db_insert('file_usage')
+    $connection->insert('file_usage')
       ->fields([
         'fid' => $file->id(),
         'module' => 'testing',
@@ -105,7 +106,8 @@ public function doTestRemoveUsage() {

@@ -105,7 +106,8 @@ public function doTestRemoveUsage() {
     $file = $this->createFile();
     $file->setPermanent();
     $file_usage = $this->container->get('file.usage');
-    db_insert('file_usage')
+    $connection = Database::getConnection();
+    $connection->insert('file_usage')
       ->fields([
         'fid' => $file->id(),

This is using two different methods to get the connection in the same class, is it intentional?

longwave’s picture

Answering that would seem to need a consensus on #2991337: Document the recommended ways to obtain the database connection object, as that currently suggests $this->container->get('database') in kernel tests, which is neither of the methods used there.

Status: Needs review » Needs work

The last submitted patch, 72: 2853118-72.patch, failed testing. View results

voleger’s picture

Rerolled
Addressed #75. Replaced \Drupal::database() by Database::getConnection()

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Mostly all over kernel tests we should use Database::getConnection(); except cases then there's $this->connection

The last submitted patch, 78: 2853118-78.patch, failed testing. View results

catch’s picture

Status: Reviewed & tested by the community » Needs work

Needs a re-roll.

voleger’s picture

No changes, just reroll.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

After #2999678: Properly deprecate db_field_exists patch -p1

patching file core/tests/Drupal/KernelTests/Core/Database/DatabaseLegacyTest.php
Hunk #2 succeeded at 427 (offset 9 lines).
voleger’s picture

Yeah, patches committed too fast) previous few rerolls shows that there should not be the breaking changes, just lines offset.

voleger’s picture

catch’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 85: 2853118-85.patch, failed testing. View results

longwave’s picture

andypost’s picture

Status: Needs review » Reviewed & tested by the community

yep, clean reroll

voleger’s picture

Tests are "green". A patch ready to go.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 383f701 and pushed to 8.7.x. Thanks!

  • alexpott committed 383f701 on 8.7.x
    Issue #2853118 by voleger, Jo Fitzgerald, Pavan B S, yogeshmpawar,...

Status: Fixed » Closed (fixed)

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

quietone’s picture

Publish the CR