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

Command icon 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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

mondrake’s picture

Status: Active » Postponed

mondrake’s picture

A first patch, pretty raw.

daffie’s picture

Status: Postponed » Needs review
mondrake’s picture

Issue tags: +Needs change record
apaderno’s picture

Issue summary: View changes
mondrake’s picture

Status: Needs review » Needs work
mondrake’s picture

Status: Needs work » Needs review
mondrake’s picture

Actually, Database::RETURN_NULL becomes unused in core with this patch.

mondrake’s picture

Issue tags: -Needs change record

Added CR https://www.drupal.org/node/3185520, and reflected in the MR.

alexpott’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

I'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.

alexpott’s picture

Also the issue title is misleading because we are not only deprecating something but we are adding something new.

alexpott’s picture

Plus 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.

mondrake’s picture

Yeah. 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 call Connection::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.

alexpott’s picture

@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.

mondrake’s picture

Title: Deprecate Database::RETURN_INSERT_ID » [PP-2] Deprecate Database::RETURN_* constants
Status: Needs work » Postponed
Related issues: +#3177660: Remove public properties from StatementInterface implementations
andypost’s picture

Title: [PP-2] Deprecate Database::RETURN_* constants » [PP-1] Deprecate Database::RETURN_* constants
mondrake’s picture

Rerolled.

mondrake’s picture

Deprecating 'return' query option itself.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

daffie’s picture

daffie’s picture

Title: [PP-1] Deprecate Database::RETURN_* constants » Deprecate Database::RETURN_* constants
mondrake’s picture

Rerolled.

mondrake’s picture

Title: Deprecate Database::RETURN_* constants » Deprecate the 'return' query option and Database::RETURN_* constants
Issue summary: View changes
mondrake’s picture

Status: Needs work » Needs review

Updated CR draft.

andypost’s picture

Status: Needs review » Needs work

Deprecation message needs work

- add "()" after __METHOD__
- fix the @todo inside of it

PS: looking on 40 commits in MR ... patch workflow looks preferable (from reviewer POV)

mondrake’s picture

Status: Needs work » Needs review

Thanks, addressed #27.

andypost’s picture

Looks only #12 needs to be resolved, it's really not clear why all this new classes been added

mondrake’s picture

Title: Deprecate the 'return' query option and Database::RETURN_* constants » Introduce Connection::lastInsertId and deprecate the 'return' query option and Database::RETURN_* constants
Issue summary: View changes
Issue tags: -Needs issue summary update
mondrake’s picture

#29:

it's really not clear why all this new classes been added

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.

andypost’s picture

It could affect very old bugs but sounds could solve some, at least remove duplication of APIs

daffie’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

The MR needs to be rebased.

mondrake’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

Rerolled

daffie’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

All 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.

mondrake’s picture

Reroll

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

I'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.

mondrake’s picture

#38 thanks, good catch. Let's see what the revert says.

mondrake’s picture

Status: Needs review » Postponed

Anyway 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.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mondrake’s picture

Status: Postponed » Needs review

Rebased to 9.4 and rerolled.

daffie’s picture

Status: Needs review » Needs work

The testbot is not happy. Probably related to moving the database driver to their own module.

apaderno’s picture

The test before comment #42 failed because this error.

Your XML configuration validates against a deprecated schema.

For what I saw, it sometimes happens with tests, but it doesn't seem to be caused by the introduced changes.

mondrake’s picture

#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.

mondrake’s picture

Is 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?

daffie’s picture

@mondrake: What happens when you add the following code to the test:

  /**
   * Modules to enable.
   *
   * @var array
   */
  protected static $modules = ['mysql'];

The testbot error is:

Drupal\FunctionalTests\Installer\InstallerNonDefaultDatabaseDriverTest::testInstalled
Exception: Error: Class "Drupal\mysql\Driver\Database\mysql\Select" not found
include()() (Line: 10)
mondrake’s picture

Status: Needs work » Needs review

#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

daffie’s picture

Status: Needs review » Needs work

The MR looks good.

mondrake’s picture

Status: Needs work » Needs review

Thanks

daffie’s picture

Status: Needs review » Needs work

The 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.

mondrake’s picture

Status: Needs work » Reviewed & tested by the community

Let’s mark this RTBC then, so we can get attention. I’ll work out a D10 patch… next year.

mondrake’s picture

Actually, the MR applies to 10.0.x too.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Left a couple of review comments on the MR.

mondrake’s picture

Status: Needs work » Needs review

Thanks for review @alexpott

daffie’s picture

Status: Needs review » Reviewed & tested by the community

The points of @alexpott have been addressed.
Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 87dc8f4192b to 10.0.x and 9a28ddcea55 to 9.4.x. Thanks!

  • alexpott committed 87dc8f4 on 10.0.x
    Issue #3185269 by mondrake, daffie, alexpott, andypost: Introduce...

  • alexpott committed 9a28ddc on 9.4.x
    Issue #3185269 by mondrake, daffie, alexpott, andypost: Introduce...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.