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.
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.
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);
CreditAttribution: apaderno as a volunteer commented
Status:
Needs review
» Needs work
+ @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);",
@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
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.
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.
CreditAttribution: mondrake as a volunteer commented
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 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.
Comments
Comment #2
vidhatanand CreditAttribution: vidhatanand at OpenSense Labs commentedComment #3
pritamprasun CreditAttribution: pritamprasun at OpenSense Labs commentedComment #4
vidhatanand CreditAttribution: vidhatanand at OpenSense Labs commentedComment #5
JayKandari#3 Looks good.
1 "db_table_exists()" call found in
core/includes/schema.inc:137
. Rest calls are in test files.Thanks !
Comment #6
cilefen CreditAttribution: cilefen commentedRead the meta issue carefully about test replacements.
Comment #7
hgunicamp CreditAttribution: hgunicamp at CI&T commentedI'm sending a patch to replace the tests cases that use db_table_exists.
Comment #8
hgunicamp CreditAttribution: hgunicamp at CI&T commentedComment #10
hgunicamp CreditAttribution: hgunicamp at CI&T commentedFixing my previous patch. Removing a wrong regression test.
Comment #11
Sharique CreditAttribution: Sharique as a volunteer commentedInstead use "Database::getConnection()->schema()->tableExists()" method,
check db_table_exists() implementation for details.
Comment #12
jeetendrakumar CreditAttribution: jeetendrakumar as a volunteer and at HyTechPro.com commented@Sharique
I do not agree with you. "\Drupal::database()->schema()->tableExists($table)" will also work.
Comment #13
Sharique CreditAttribution: Sharique as a volunteer commentedTo 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...
Comment #15
JacobSanford@hgunicamp's patch in #10 no longer applied to 8.5.x. A reroll with no further modifications is attached.
Comment #16
JayKandariChanging status to review after #15.
Comment #19
volegerPatch #15 still applies. Tested on 8.6.x
Looks good
Comment #20
alexpottThis 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/deprecationThis will also prove we've replaced all the usages in core.
Comment #21
alexpottWill also need to add
@group legacy
and@expectedDeprecation ...
to \Drupal\KernelTests\Core\Database\RegressionTest::testDBTableExists()Comment #22
voleger#20 #21 - added
Comment #23
apadernoComment #24
alexpottInstead of doing this we could do:
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.
So tldr; let's just remove the check.
Comment #25
voleger#24: Removed additional check.
Comment #26
volegerComment #27
volegerTests are green. So, back to RTBC?
Comment #28
andypostReroll + 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...cos patch does change that line makes sense to clean-up it as #24 said
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
Comment #29
andypostAnd better to have this comment instead of check
Comment #30
andypostOh this functions were deprecated on 8.0.x so revert version
Comment #31
andypostannotation better to stick per method to prevent collision with other patches
Comment #32
apadernoThe trigger message should be similar to the following one (https://www.drupal.org/core/deprecation).
Comment #33
andypostThis functions were deprecated before 8.0, so we just add triggering error here
Comment #34
voleger#33 +1
See #1894396: Mark db_*() wrappers in database.inc as @deprecated for 9.x
Comment #35
alexpottIt 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.Comment #36
volegerAdded CR
https://www.drupal.org/node/2947929
Comment #37
volegerChanged deprecation message.
Comment #38
andypostSure 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_* functionsComment #40
volegerReverted changes in SchemaTest. See #2987856: Followup: Update Schema test.
Rerolled.
Comment #41
volegerComment #43
volegerComment #44
andypostLooks great to go, but needs one more pair of eye on it
Comment #45
borisson_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.Comment #48
alexpottGiving 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!
Comment #51
mondrakeI 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.
Comment #52
alexpott@mondrake nice spot - can you create a issue to do that?
Comment #53
alexpott@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.
Comment #54
volegerLooks like IS of #2848161: [meta] Replace calls to deprecated db_*() wrappers should be updated
Comment #55
mondrake#51/#52, opened issue #2991542: Introduce a DatabaseLegacyTest class for deprecation testing.
Comment #57
quietone CreditAttribution: quietone at PreviousNext commentedpublish change record