Problem/Motivation

In #2868273: Missing a test for table TRUNCATE while in transaction it was discussed to introduce a $connection property to the DatabaseTestBase base class so that extending classes can use it in the test code without having to implement it in each class.

Proposed resolution

Move the logic from the DeleteTruncateTest to the DatabaseTestBase class.

Remaining tasks

Do it.

User interface changes

none

API changes

none

Data model changes

none

CommentFileSizeAuthor
#2 2953385-2.patch3.8 KBmondrake

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: mondrake » Unassigned
Status: Active » Needs review
StatusFileSize
new 2953385-2.patch3.8 KB

Patch.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/core/tests/Drupal/KernelTests/Core/Database/DatabaseTestBase.php
@@ -56,8 +65,10 @@ public function ensureSampleDataNull() {
    */
   public static function addSampleData() {
+    $connection = Database::getConnection();
+
     // We need the IDs, so we can't use a multi-insert here.
-    $john = db_insert('test')
+    $john = $connection->insert('test')
       ->fields([

confused why this isn't using $this->connection now ;)

mondrake’s picture

mondrake
Location 🇮🇹
commented

#3 addSampleData() is a static ... so unless we change it to non-static, $this will not be in scope :)

Did not want to change it, but apparently this is not used anywhere else. It could also be a protected, actually.

mondrake’s picture

mondrake
Location 🇮🇹
commented

#3, #4,

actually no:

$ git grep addSample
core/modules/system/src/Tests/Database/DatabaseWebTestBase.php:    DatabaseTestBase::addSampleData();
core/modules/system/tests/src/Functional/Database/DatabaseTestBase.php:    DatabaseKernelTestBase::addSampleData();
core/tests/Drupal/KernelTests/Core/Database/DatabaseTestBase.php:    self::addSampleData();
core/tests/Drupal/KernelTests/Core/Database/DatabaseTestBase.php:  public static function addSampleData() {

it's more complicated, we cannot change easily to non-static.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented 
+++ b/core/tests/Drupal/KernelTests/Core/Database/DatabaseTestBase.php
@@ -14,8 +15,16 @@
+    $this->connection = Database::getConnection();

it is possible to use
$this->container->get('database');
here?

mondrake’s picture

mondrake
Location 🇮🇹
commented

#6: the database connection code in the KernelTestBase class that DatabaseTestBase extends from uses direct calls to the Database methods rather than going through the service container. That's the reason I was going for it too.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented

#7 ok
So maybe it would be better to store connection instance in a single static property?

self::$connection = Database::getConnection();
mondrake’s picture

mondrake
Location 🇮🇹
commented

@voleger re #8 why do you think it would be better? I cannot say the full consequences of that, but

a) the ::setUp method will anyway reset it for every test, which I am afraid may cause issues in running multiple tests in parallel

b) it will not solve #3 / #4 because Drupal\system\Tests\Database\DatabaseWebTestBase and Drupal\Tests\system\Functional\Database\DatabaseTestBase are not extending from Drupal\KernelTests\Core\Database\DatabaseTestBase, so the property will not be initialised

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented

for now, it's ok for me.
+1 for RTBC
I'm waiting this issue to unblock #2939387: Replace all calls to db_query [core/test/../Database]

joshmiller’s picture

joshmiller
he/him
Primary language English
Location Indianapolis, Indiana, USA
commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 872ddb8101 to 8.6.x and bdfce71e7c to 8.5.x. Thanks!

Backported to 8.5.x as this is a test only change.

  • alexpott committed 872ddb8 on 8.6.x 
    Issue #2953385 by mondrake, voleger: Add a $connection property to...

  • alexpott committed bdfce71 on 8.5.x 
    Issue #2953385 by mondrake, voleger: Add a $connection property to...

Status: Fixed » Closed (fixed)

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