Problem/Motivation
From #3137883-72: Deprecate passing a StatementInterface object to Connection::query:
We can then open a followup about deprecating Database::RETURN_INSERT_ID and replacing that with a new [lastInsertId] method on the connection object.
We have already made steps to reduce the polymorphism of Connection::query - now it can only accept SQL strings in input, and de-facto row count is enabled via an argument to the Statement object, no longer through the Database::RETURN_AFFECTED return option.
With this issue we complete the cleanup by making so that Connection::query only returns StatementInterface objects, and affected rows (for UPDATE, DELETE, UPSERT, etc) and last insert id (for INSERT) are determined via specific methods.
Proposed resolution
The 'return' query option and the Database::RETURN_* constants are deprecated. In Drupal 10, they will be removed and Connection::query()
will always return a StatementInterface
object.
A new Connection::lastInsertId()
method is avaiable in the edge cases where the use Connection::query()
is needed for INSERT; in that case, Connection::lastInsertId()
will allow extraction the last insert id value from the connection.
Remaining tasks
User interface changes
API changes
A new method called Drupal\Core\Database\Connection::lastInsertId(). Which returns the last inserted row or sequence value.
Data model changes
Release notes snippet
Issue fork drupal-3185269
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:
- 3185269-deprecate-databasereturninsertid changes, plain diff MR !78
Comments
Comment #2
mondrakeComment #4
mondrakeA first patch, pretty raw.
Comment #5
daffie CreditAttribution: daffie commented#3137883: Deprecate passing a StatementInterface object to Connection::query has landed. :)
Comment #6
mondrakeComment #7
apadernoComment #8
mondrakeComment #9
mondrakeComment #10
mondrakeActually, Database::RETURN_NULL becomes unused in core with this patch.
Comment #11
mondrakeAdded CR https://www.drupal.org/node/3185520, and reflected in the MR.
Comment #12
alexpottI'm not convinced that this change is worth it. What is the benefit? The issue summary needs to clearly outline why we should make this change. Consistency with other database abstraction libraries is the only reason I've heard so far and I don't find that particularly convincing.
Comment #13
alexpottAlso the issue title is misleading because we are not only deprecating something but we are adding something new.
Comment #14
alexpottPlus we should probably land #3185399: Use RETURNING to determine last insert id in Drupal\Core\Database\Driver\pgsql\Insert so we don't use postgres as a reason why because it's not. It has a more efficient way of returning the last ID that we can leverage.
Comment #15
mondrakeYeah. My vision is that we should not need any of these RETURN_* constants, at all.
Connection::query
should always return a Statement object. The specialised Connection methods for insert, update, delete etc. should know by themselves what to return via their::execute
methods, because they prepare a Statement and the execute it, and do not callConnection::query
.But yes, this is my vision and it should be agreed if it's worth doing beforehand. This issue would be a step in that direction already.
Comment #16
alexpott@mondrake I think doing this change piecemeal is problematic. Removing one of the RETURN_* constants without doing the rest leaves us in an inconsistent state. I like the idea more of deprecating all of them than just one.
Comment #17
mondrakeOK we definitely have to do #3185399: Use RETURNING to determine last insert id in Drupal\Core\Database\Driver\pgsql\Insert, and #3177660: Remove public properties from StatementInterface implementations, beforehand.
Comment #18
andypostComment #19
mondrakeRerolled.
Comment #20
mondrakeDeprecating 'return' query option itself.
Comment #22
daffie CreditAttribution: daffie commented#3177660: Remove public properties from StatementInterface implementations has landed.
Comment #23
daffie CreditAttribution: daffie commentedComment #24
mondrakeRerolled.
Comment #25
mondrakeComment #26
mondrakeUpdated CR draft.
Comment #27
andypostDeprecation message needs work
- add "()" after
__METHOD__
- fix the
@todo
inside of itPS: looking on 40 commits in MR ... patch workflow looks preferable (from reviewer POV)
Comment #28
mondrakeThanks, addressed #27.
Comment #29
andypostLooks only #12 needs to be resolved, it's really not clear why all this new classes been added
Comment #30
mondrakeComment #31
mondrake#29:
To provide BC and avoid deprecation errors in core tests. We need to revert setting the 'return' queryOption. That is set in the base classes, that can be used by contrib drivers. So only for core drivers (including the test drivers), we need to extend from the base class, then unset the option. The test drivers that extend from core mysql/pgsql/sqlite need to define the classes in their file namespace otherwise they fall back to the base ones via ::getDriverClass. #3217531: Deprecate usage of Connection::getDriverClass for some classes, and use standard autoloading instead will help, because it will cut the magic fallback to base classes.
Comment #32
andypostComment #33
andypostIt could affect very old bugs but sounds could solve some, at least remove duplication of APIs
Comment #34
daffie CreditAttribution: daffie commentedThe MR needs to be rebased.
Comment #35
mondrakeRerolled
Comment #36
daffie CreditAttribution: daffie commentedAll code changes look good to me.
The constants are correctly deprecated.
There are no BC break as far as I can see.
Updated the IS with new method.
For me it is RTBC.
Comment #37
mondrakeReroll
Comment #38
alexpottI'm not sure about the changes to core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php, core/lib/Drupal/Core/Menu/MenuTreeStorage.php and core/lib/Drupal/Core/Config/DatabaseStorage.php - these are outside the core db drivers so I think they shouldn't change - just in case a contrib driver is relying on the return constants being set. I think we can only remove these in D10. I think if we revert them everything will be fine because we're unsetting the return stuff in the core drivers. I might be wrong though cause BC is hard.
Comment #39
mondrake#38 thanks, good catch. Let's see what the revert says.
Comment #40
mondrakeAnyway at this point I think it'd preferable to do #3129043: Move core database drivers to modules of their own first so we do not have more classes to move over from lib to the modules.
Comment #42
mondrakeRebased to 9.4 and rerolled.
Comment #43
daffie CreditAttribution: daffie commentedThe testbot is not happy. Probably related to moving the database driver to their own module.
Comment #44
apadernoThe test before comment #42 failed because this error.
For what I saw, it sometimes happens with tests, but it doesn't seem to be caused by the introduced changes.
Comment #45
mondrake#44 actually that is not relevant. I’s just PHPUnit complaining about its own configuration file, it’s not implying anything for the tests themselves.
Comment #46
mondrakeIs it possible this last failure is due to the driver provided by DrivertestMysql extending the classes from Mysql that post installation, not being enabled because it’s not the default, are not autoloadable?
Comment #47
daffie CreditAttribution: daffie commented@mondrake: What happens when you add the following code to the test:
The testbot error is:
Comment #48
mondrake#47 here the problem is that
DrivertestMysql
has classes extending from MySql, and those need to be accessible BEFORE the site starts running. It's in fact a problem of dependencies and installing the extended module as well as the extending one very early in the installation process, doing it in the test would be too late. See latest patch.BTW this is a bigger problem that this issue - a BC shim module extending from core drivers will not work with HEAD ATM
Comment #49
daffie CreditAttribution: daffie commentedThe MR looks good.
Comment #50
mondrakeThanks
Comment #51
daffie CreditAttribution: daffie commentedThe MR for D9.4 is for me RTBC.
I think we should also create the patch/MR for D10.0.
All other stuff is done.
Comment #52
mondrakeLet’s mark this RTBC then, so we can get attention. I’ll work out a D10 patch… next year.
Comment #53
mondrakeActually, the MR applies to 10.0.x too.
Comment #54
alexpottLeft a couple of review comments on the MR.
Comment #55
mondrakeThanks for review @alexpott
Comment #56
daffie CreditAttribution: daffie commentedThe points of @alexpott have been addressed.
Back to RTBC.
Comment #57
alexpottCommitted and pushed 87dc8f4192b to 10.0.x and 9a28ddcea55 to 9.4.x. Thanks!