Problem/Motivation
Connection::query
can overload the $query
argument to be either a string or an already prepared statement. This is a bit confusing, since the logic of potentially calling query
to get a statement and then query
again with the statement as input is weird. Normally in PHP world you would prepare
a statement, execute
it and iterate to fetch
the results - then execute
it again with different $args
if you want to reuse.
Also the current status will prevent in the future to typehint the $query argument, which is now possible in PHP 7+.
Proposed resolution
- Deprecate passing a StatementInterface
as $query
argument to Connection::query
.
- When a database operation is started with a Connection::prepareStatement
, follow suit with StatementInterface::execute
and StatementInterface::fetch
(in case of a SELECT), or other statement inspection methods for INSERT / DELETE / UPDATE.
- Keep Connection::query
as a convenience helper that prepares the statement and executes it in one go.
Remaining tasks
User interface changes
None
API changes
None
Data model changes
None
Release notes snippet
Deprecate passing a Drupal\Core\Database\StatementInterface object to Drupal\Core\Database\Connection::query().
Comment | File | Size | Author |
---|
Issue fork drupal-3137883
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3137883-deprecate-passing-a changes, plain diff MR !74
Comments
Comment #2
tdnshah CreditAttribution: tdnshah as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #3
tdnshah CreditAttribution: tdnshah as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #4
mondrakeBetter wait on #2345451: Introduce a Connection::prepareStatement method allowing to pass options to the Statement, deprecate Connection::prepareQuery() and Connection::prepare(), bit in the meantime open for discussion if this is a good idea.
Comment #5
daffie CreditAttribution: daffie commented#2345451: Introduce a Connection::prepareStatement method allowing to pass options to the Statement, deprecate Connection::prepareQuery() and Connection::prepare() has landed.
Comment #6
mondrakeKick-off, discovery, patch.
Comment #7
mondrakeNot bad, #6 tells us there are already no calls to
query
with a statement argument in core code - apart from the ones within the pgsql driver itself.Let's see if this fixes pgsql, by using the appropriate statement methods instead of
query
.Comment #8
mondrakeComment #9
mondrakeNeeds a deprecation test.
Comment #10
mondrakeWhoops.
Comment #12
mondrakeOn this.
Comment #13
mondrakeComment #14
daffie CreditAttribution: daffie commentedCan we add some testing for the exceptions being thrown here.
Comment #15
mondrakeThanks @daffie.
Fixed #14.1 and .4.
#14.2 what's strange? Since we are not calling query anymore, we need to explicitly manage any exception thrown by the call to
StatementInterface::execute
. The existing insert tests cover that already.#14.3 outstanding.
Comment #16
mondrakeComment #17
mondrakeComment #18
mondrake#17 adds testing for the new Connection::lastInsertId method, per #14.2.
Re #14.3,
InvalidDataTest::testInsertDuplicateData
, covers the case already.Switching to NR.
Comment #19
daffie CreditAttribution: daffie commentedUpdated the IS and created an CR.
Fix the TODO parts.
Should we not add a @throws annotation to the docblock?
Is this part PostgreSQL specific or does this belong in the method Connection::lastInsertId()? Just asking.
Comment #20
mondrake#19:
2. Well this is just a proxy to the method on the PDO connection. Whatever that one will throw, this method will just pass it through. In other words, whateevr is thrown, it is not thrown here. We have a
@see \PDO::lastInsertId
already, IMO that's enough.3. It is pgsql specific, in the sense that for this driver we prepare a statement (already, in HEAD), and then we execute it and have to manage any exceptions that previoulsy would have been captured by
query
and managed byhandleQueryException
. Other drivers just runquery
which is self contained.See this in PGSql (currently in HEAD, without the patch):
and in MySql
IMHO,
Connection::query
is overdoing. Since we have db drivers, each operation should prepare its own statement, execute it, and handle its own exceptions.Comment #21
daffie CreditAttribution: daffie commented@mondrake: Thank you for your explanation. If you could do #19.1, then it is RTBC for me.
Comment #22
mondrakeon this
Comment #23
mondrakeDone #19.1.
Comment #24
daffie CreditAttribution: daffie commentedAll looks good to me.
All my remarks are addressed.
For me it is RTBC.
Comment #25
mondrakeComment #26
catchIs this actually necessary in order to deprecate passing a statement, or is it a bug found while working on the patch?
Comment #27
mondrakeIt's not a bug, and yes it's needed to deprecate.
That's because the post execute processing is no longer done in
Connection::query
here:Normally
query
will deal with processing the results, but now in these classes we now no longer callquery
, rather prepare the statement and execute it. So the same code that is currently inConnection
:needs to be replicated into the dedicated classes.
Comment #28
mondrakeAlso, see #20.
Comment #29
alexpottIs this change actually worth it? In PHP 8.0 we'll get the ability to do
string|StatementInterface
as a typehint.Adding this has it's own risks and adds new API in order to deprecate other API - which seems odd.
Comment #30
andypostneeds re-roll for https://www.drupal.org/node/3176667
Comment #31
andypostNW for #30
New API is a way to get rid of
getClientConnection()
(btw why notgetDriverConnection
)Sequences API mostly unused by both core and contrib so there's at least #2665216: Deprecate Drupal\Core\Database\Connection::nextId() and the {sequences} table and schema
Comment #32
mondrakeLooking outside of Drupal, doctrine/dbal has a lastInsertId method on its Connection class, https://github.com/doctrine/dbal/blob/2224d36702f0d7313bdecd2b01169767b7...
Comment #33
andypostAnd even https://www.php.net/manual/ru/sqlite3.lastinsertrowid.php non-PDO drivers implement it
Comment #34
andypostBetter to mark it @internal as return value may flux depending on driver
PHP 8 defines it as
@return string|false
https://github.com/php/php-src/blob/php-8.0.0RC2/ext/pdo/pdo_dbh.stub.ph...
Comment #35
andypostAlso looking at implementation it may throw https://github.com/php/php-src/blob/php-8.0.0RC2/ext/pdo/pdo_dbh.c#L955-... but returns
FALSE
Comment #36
andypostFix for #30 and re-roll
Comment #38
daffie CreditAttribution: daffie commentedShould we not move this text to a
@throws
in the docblock of this method?Why do we not add the
$e->getCode()
DatabaseExceptionWrapper call?Can we change the construction to (for better readability):
We are now working on 9.2.0 and not with 9.1.0
For #29: If we want to use the construction parameter type hint with
string|StatementInterface
we shall have to wait for A Drupal version that only supports PHP 8.0 or higher. Personally I do not want to wait for that. Just my opinion. :)Comment #39
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedHi,
Creating a patch as per the #38 comment suggestion.
Please review the patch.
Thanks.
Comment #40
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented@Pooja Ganjage can you please provide interdiff as well.
Comment #41
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedAttached interdiff for #39 patch.
Comment #42
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedComment #43
daffie CreditAttribution: daffie commented@Pooja: All your changes look good. Only point #38.3 is still open. After that is done, will I give it a RTBC.
Comment #44
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedHi,
@daffie, I have not understand the point of #38.3, what need to change?
Please give me suggestion for this. So, I can create a patch again.
Thanks.
Comment #45
paulocsComment #46
paulocsFixing code standard.
Point #38.3 is still open.
Comment #47
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedHi,
Updated patch with mentioned changes into the #38.3 point.
Please review the patch.
Thanks.
Comment #48
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedAttached interdiff for #47 patch.
Comment #49
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedComment #50
daffie CreditAttribution: daffie commentedI reviewed the patch from Pooja Ganjage. Just one point to fix:
According to the Drupal coding standards the tag @returns comes before @throws. See: https://www.drupal.org/docs/develop/standards/api-documentation-and-comm.... Almost RTBC.
@paulocs: I did not review your patch, because the other patch has comment #38.3 fixed.
Comment #51
mondrakeActually I think we can drop this altogether because if the PDO driver were not implementing this, it would not be possible to use it in Drupal.
Comment #52
daffie CreditAttribution: daffie commentedI agree with @mondrake. Lets remove the @throws from the docblock.
Comment #53
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedRemoved the @throws from the docblock.
Comment #54
daffie CreditAttribution: daffie commentedThe patch does what is in the IS.
There is deprecation testing.
The CR has the deprecation and the API addition in it.
All changes in the patch look good to me.
For me it is RTBC.
Comment #55
mondrakeIMHO this is not really @internal, since database drivers may override it. How about using the statement recently used in the
StatementWrapper
class:should have a
void
return typehintthis is a database test, and IMHO we should be strict, therefore suggest we do not typecast:
Comment #56
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedUpdated patch as per the #55 comment suggestion.
Comment #57
mondrakeplease revert this, it's not in the scoipe of the issue here
and do this for lastInsertId
so this is a bit less PDO-centric, we have discussions open to abstract more from PDO so let's not go in the opposite direction.
Comment #58
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedUpdated patch.
Comment #59
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedComment #60
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedAttached interdiff for #58 patch.
Comment #61
daffie CreditAttribution: daffie commentedThe patch does not what @mondrake has asked for in comment #57. Therefor back to needs work.
Comment #62
mondrakeworking on this
Comment #63
mondrakedone #57
Comment #64
daffie CreditAttribution: daffie commentedThe docblock for lastInsertId looks good to me.
Back to RTBC.
Comment #65
alexpottI think this needs to match the previous logic. And not overwrite the $last_insert_id when it is already set.
Like the comment is
// Only use the returned last_insert_id if it is not already set.
is still there but with the current logic it is not working.Something like
I'm not sure about adding API to remove API. But also I'm wondering if it is really needed?
This is the only place where the new method is used and I'm not sure it is needed. Is it really necessary to add Connection::lastInsertId() in this issue?
Comment #67
mondrakeAddressed #65 first suggestion, the rest I need to think and figure out. Personally I do not see the problem to have a mehod for getting last insert id, this would just put us on par with what, for instance, doctrine/dbal have.
We might think to have it on pgsql Connection only, since its Insert method needs now to access the lower level PDO connection, one way or another, to get the info that is no longer available since we are inserting by executing the statement and no longer Connection::query.
Comment #68
mondrakeComment #69
alexpottWhat we can do in postgres is do a query :) that's what
return $this->connection->lastInsertId($sequence_name);
does anyway - see https://github.com/php/php-src/blob/master/ext/pdo_pgsql/pgsql_driver.c#...Looking at https://stackoverflow.com/questions/2944297/postgresql-function-for-last... - there's we can do something like
SELECT currval('sequence_name');
Or maybe even better we can use the
RETURNING id
thing because we're constructing the query here to. And there we can avoid the extra query completely :)Comment #70
mondrake#65 I stand by the new method; I'd rather go the other way round and convert also base, mysql and sqlite Insert classes to use the statement directly instead of ::query. That would potentially open the door to deprecate
Database::RETURN_INSERT_ID
and simplify::handleQueryException
since these casescould be dealt on the Insert::execute methods only.
Comment #71
mondrake#69
that sounds promising and also a performance improvement.
IMHO #70 could be pursued anyway, though.
Comment #72
alexpott@mondrake I think we should decouple the new API from deprecating.
The simplest thing to do here seems to be to do the
SELECT currval('sequence_name');
in the postgres insertWe can then open a followup about deprecating
Database::RETURN_INSERT_ID
and replacing that with a new method on the connection object.We can also open a follow-up to explore the possible performance improvements of using
RETURNING id
Comment #73
mondrakeOK, on that.
Comment #74
mondrakeComment #75
daffie CreditAttribution: daffie commented@mondrake: Maybe the problem is that there are to many quotes in the following line:
Comment #76
mondrakeThanks @daffie it looks like I was just missing the connection.
Comment #77
daffie CreditAttribution: daffie commentedI have updated the CR.
Could we change the following code:
to:
For better readability.
The rest of the patch is for me RTBC.
Comment #78
mondrake@daffie I see #77 more as a personal preference, but OK.
Comment #79
daffie CreditAttribution: daffie commentedUpdated the IS.
The patch looks good now.
For me it is RTBC.
@mondrake: Thank you.
Comment #80
alexpottI tried leaving a code review on the merge request. Looking at the comment above we need to integrate this a bit better. Here's the a link to the comment - https://git.drupalcode.org/project/drupal/-/merge_requests/74#note_5067 and also this sets this back to needs work.
Comment #81
mondrakeComment #82
alexpottA further tidy up can be done.
Comment #83
mondrakeComment #84
mondrakeFiled #3185269: Introduce Connection::lastInsertId and deprecate the 'return' query option and Database::RETURN_* constants.
Comment #85
mondrakeNeed to add a deprecation trigger to the case a PDOStatement is passed in.
Comment #86
mondrakeComment #87
daffie CreditAttribution: daffie commentedPatch looks good. Just 2 minor remarks.
We are not only deprecating StatementInterface input, but also PDOStatement input. Can we mentioning that in the docblock.
Nitpick: This line goes over the 80 character limit.
Comment #88
mondrakeThanks @daffie
Comment #89
mondrakeIn a push earlier today I added deprecation comments not meant for this issue/MR, reverted.
Comment #90
daffie CreditAttribution: daffie commentedThe patch now looks great.
For me it is RTBC.
Comment #91
alexpottI agree this patch looks good now - waiting for the test runs before committing. Noticed a couple of extra things that we need to file issues for:
I couldn't find an issue to the upsert one and I didn't look for the other one.
I assigned issue credit for patch review based on issue comments.
Comment #92
alexpottCommitted 6aab223 and pushed to 9.2.x. Thanks!
I added an ignore for currval for cspell as this failed the check.
Comment #95
mondrakeFiled follow ups.
Comment #97
mradcliffeFiled a follow-up issue #3210071: DatabaseExceptionWrapper has incorrect arguments in Insert as the arguments passed in this line,
throw new DatabaseExceptionWrapper($message, 0, $e->getCode());
, aren't correct.