Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
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.
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
Reverted changes in self-test.
Used $schema instance in separate variable because we have another functions to replace that will use $schema instance.
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.
CreditAttribution: mondrake as a volunteer commented
+++ 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.
+++ 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.
+++ 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'));
+ }
CreditAttribution: mondrake as a volunteer commented
+++ 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.
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?
Comments
Comment #2
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedAll the calls to db_drop_table replaced with new syntax.
Comment #3
cilefen CreditAttribution: cilefen commentedThank you! Link the parent issue when you make these, please. They are tasks, not bugs.
Comment #4
JayKandari2 instances of "db_drop_table()" found in following:
patch #2 covers both of them. Looks good!
Thanks !
Comment #5
cilefen CreditAttribution: cilefen commentednitpick - the line indentation is wrong.
Comment #6
marvin_B8 CreditAttribution: marvin_B8 as a volunteer and at comm-press commentedComment #7
daffie CreditAttribution: daffie commented@marvin_B8: You forgot to replace some instances in Drupal\KernelTests\Core\Database\SchemaTest
Comment #8
yogeshmpawarUpdated patch as per comment #7 & also added interdiff.
Comment #9
yogeshmpawarSame patch with added tests against 8.4.x branch.
Comment #10
daffie CreditAttribution: daffie commentedAll code changes are good.
All instances of the usage of
db_drop_table()
are replaced.The patch looks good to me.
Comment #11
xjmAt 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!
Comment #12
yogeshmpawarThanks @xjm, as per your comment #11 i have updated the patch & added a interdiff also.
Comment #13
daffie CreditAttribution: daffie commentedThis is what @xjm wants. Back to RTBC.
Comment #14
xjmlol, 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()
inSchemaTest
(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 (especiallySchemaTest
) and discuss the best approach.Again, though, I could be wrong. :)
Comment #15
hgunicamp CreditAttribution: hgunicamp at CI&T commentedI 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.
Comment #16
hgunicamp CreditAttribution: hgunicamp at CI&T commentedComment #19
volegerReroll for 8.6.x
Patch is pretty small, so it could be ready to go.
+1 RTBC
Comment #20
volegerPatch cleanup
Comment #22
volegerReroll
Reverted changes in self-test.
Used
$schema
instance in separate variable because we have another functions to replace that will use$schema
instance.Comment #23
volegerUpload the correct interdiff
Comment #24
mondrakeNice!
Looks good to me, RTBC.
Comment #26
alexpottCan we add a deprecation trigger_error() - see db_set_active() for an example.
Comment #27
alexpottWrt to SchemaTest we can replace the calls to drop_table() with
Database::getConnection()->schema()->dropTable()
-we're already usingDatabase::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.Comment #28
volegerAdded change notice https://www.drupal.org/node/2987737
Added Regression test.
Added replacements in the Schema tests.
Comment #29
volegerComment #31
mondrakeI 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, anddropTable
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 thetestFindTables
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.Comment #32
volegerThank's for the review @mondrake
Comment #33
alexpottdb_create_table() is no longer tested.
+1
I like it +1
But needs a
{@inheritdoc}
Comment #34
mondrakeBoth docblock and test method name need to be updated as table is no longer created.
@voleger interdiff in #32 is incorrect
Comment #35
mondrakeAlso, nitpick -
testDBCreateDropTable()
: according to CS should be 'testDb*' with lower 'b'. So in this case justtestDbDropTable
.Comment #36
mondrakeThere's an extra white line before next method. Also, I suggest to use
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)
Comment #37
voleger@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
Comment #38
volegerComment #39
mondrakeThere is still a double newline after the setUp method, but I think it's RTBC now, it could be fixed on commit.
Comment #40
alexpottCan 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!
Comment #41
volegerCreated 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?
Comment #44
alexpott@voleger oops pushed now. Sorry.