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.
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.
Comment | File | Size | Author |
---|---|---|---|
#88 | 2853118-88.patch | 35.25 KB | longwave |
Comments
Comment #2
dhruveshdtripathi CreditAttribution: dhruveshdtripathi as a volunteer and at DevsAdda for OpenSense Labs commentedComment #3
Pavan B S CreditAttribution: Pavan B S at Valuebound commentedWIll be working on this issue.
Comment #4
Pavan B S CreditAttribution: Pavan B S at Valuebound commentedApplying the patch, please Review
Comment #5
daffie CreditAttribution: daffie commentedYou forgot to replace some instances. If I apply your patch and run the commend
grep -r "db_insert" *
I get the following result:Comment #6
Pavan B S CreditAttribution: Pavan B S at Valuebound commentedUpdating the patch as per the comment #5.Please review the patch
Comment #8
Pavan B S CreditAttribution: Pavan B S at Valuebound commentedApplying the patch, please review.
Comment #9
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #12
daffie CreditAttribution: daffie commented@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:
Comment #13
dhruveshdtripathi CreditAttribution: dhruveshdtripathi as a volunteer and at DevsAdda for OpenSense Labs commentedRemoved double round brackets.
Comment #14
Pavan B S CreditAttribution: Pavan B S at Valuebound commented@daffie thanks for the comment , i cloned the latest drupal core. I am uploading the interdiff for the #13
Comment #16
daffie CreditAttribution: daffie commentedThere was an other bug:
There is a space between "$id" and "->fields(array(". That is wrong. Can we change it to:
Comment #17
yogeshmpawarPatch updated as per comment #16 & also added interdiff.
Comment #18
daffie CreditAttribution: daffie commentedThe patch looks good, but I have some remarks:
The changes in the documentation will be done in #2849745: Replace documentation recommending db_*() wrappers.
Please do not remove these lines.
Comment #19
yogeshmpawarThanks @daffie, made changes as per remarks, also added a interdiff.
Comment #20
daffie CreditAttribution: daffie commentedAll code changes are good.
All instances of the usage of
db_insert()
are replaced.The patch looks good to me.
Comment #21
xjmThe summary says:
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!
Comment #22
yogeshmpawarThanks @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"
Comment #23
hgunicamp CreditAttribution: hgunicamp at CI&T commentedI 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.
Comment #24
renatoghttps://api.drupal.org/api/drupal/core%21includes%21database.inc/function/db_insert/8.3.x
Comment #25
hgunicamp CreditAttribution: hgunicamp at CI&T commentedI'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.
Comment #26
jeetendrakumar CreditAttribution: jeetendrakumar as a volunteer and at HyTechPro.com commentedComment #27
pk188 CreditAttribution: pk188 at OpenSense Labs commentedPatch in #26 didn't apply and there are changes in some other files too. So made all the changes in patch in #27.
Comment #29
pk188 CreditAttribution: pk188 at OpenSense Labs commentedChanged the code in picture.
Comment #32
volegerReroll 8.6.x
Comment #33
volegerComment #35
volegerComment #36
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled against 8.7.x.
Comment #38
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled against latest 8.7.x.
Comment #39
volegerNeed to fix the comments length.
Comment #40
jofitz CreditAttribution: jofitz at ComputerMinds commentedCorrected line lengths.
Comment #41
volegerWe can use here
Database::getConnection()
Also here we can use
Database::getConnection()
Drupal\KernelTests\Core\Database
namespace we should use$this->connection
. SeeDrupal\KernelTests\Core\Database\DatabaseTestBase
Comment #42
volegerI guess we should not touch Regression tests.
Comment #43
jofitz CreditAttribution: jofitz at ComputerMinds commentedAddressed the requested changes in #41 and #42.
Comment #44
volegerLooks good
+1 for RTBC
Comment #46
mondrakeThere 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 thedb_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.Comment #47
mondrakeI also suggest to change the
db_insert
call intestRegression_310447
in RegressionTest to$this->connection->insert
, and add atestDbInsert
method with @group legacy and an @expectedDeprecation.Comment #48
volegerAdded CR https://www.drupal.org/node/2989742
Addresses #46 and #47
Comment #50
volegerComment #52
volegerComment #53
mondrakeThis 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.
Can't we use \Drupal::database()->insert like in all other cases with non-test code?
This is rather complicated for a simple deprecation test, why not sth simpler like
Comment #54
volegerAddressed #53
Comment #55
mondrakelooks good
Comment #56
alexpottI 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.
Still documenting db_insert().
There's a general preference for using Database::getConnection() atm. Let's use that here instead of \Drupal::database().
These files are Drupal 6 and 7 code respectively and should not be changing.
Comment #57
alexpottI 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.
Comment #58
mondrakeRe. #56.1 and 2 maybe we shall do #2987399: Update database.api.php to remove deprecated db_*() functions. first?
Comment #59
volegerAdded follow-up issue #2991337: Document the recommended ways to obtain the database connection object
Comment #60
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-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.
Comment #61
jofitz CreditAttribution: jofitz at ComputerMinds commented#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.Comment #62
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #63
andypostReroll & 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 methodComment #64
volegerThanks @andypost
Fixed few lines according to #2991337: Document the recommended ways to obtain the database connection object
Comment #65
andypostI 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 hookComment #67
longwaveDuplicate word in "call call"
Comment #68
jofitz CreditAttribution: jofitz at ComputerMinds commentedRemoved duplicate word.
Comment #69
volegerAddressed #65
I guess this detail should be mentioned in #2991337: Document the recommended ways to obtain the database connection object
Comment #72
jofitz CreditAttribution: jofitz at ComputerMinds commentedRemoved duplicate word from expected result too and corrected coding standards error.
Comment #73
longwaveLooks good, thanks.
Comment #74
andypostqueued tests for sqlite & pgsql
Comment #75
catchThis is using two different methods to get the connection in the same class, is it intentional?
Comment #76
longwaveAnswering 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.Comment #78
volegerRerolled
Addressed #75. Replaced
\Drupal::database()
byDatabase::getConnection()
Comment #79
andypostMostly all over kernel tests we should use
Database::getConnection();
except cases then there's$this->connection
Comment #81
catchNeeds a re-roll.
Comment #82
volegerNo changes, just reroll.
Comment #83
andypostAfter #2999678: Properly deprecate db_field_exists
patch -p1
Comment #84
volegerYeah, patches committed too fast) previous few rerolls shows that there should not be the breaking changes, just lines offset.
Comment #85
volegerReroll. Addressed #83
Comment #86
catchComment #88
longwaveRerolled.
Comment #89
andypostyep, clean reroll
Comment #90
volegerTests are "green". A patch ready to go.
Comment #91
alexpottCommitted 383f701 and pushed to 8.7.x. Thanks!
Comment #94
quietone CreditAttribution: quietone at PreviousNext commentedPublish the CR