Problem/Motivation

The values returned by ::execute() DML methods (INSERT, UPDATE, UPSERT, DELETE, MERGE) and from the TRUNCATE DDL method are inconsistent and fragile.

See discussions in #3567868: \Drupal\Core\Database\Query\Update::execute has wrong documentation for return type and #2547493: Add support for unique / primary key constraints composed of multiple fields for Upsert queries.

Proposed resolution

  1. Introduce QueryExecutionVoidInterface, that declares a public function execute(): void method
  2. Change TRUNCATE class to implement such interface, and add void as execute() return type
  3. Adjust PgSql implementation

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#10 3569137-nr-bot_cxvxb929.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3569137

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:

About issue forks

Comments

mondrake created an issue. See original summary.

mondrake opened merge request !14464

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Deprecate TRUNCATE ::execute() method » Deprecate usage of TRUNCATE ::execute() method
Issue summary: View changes
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

First thought was this seems like something that should be removed in 13 if deprecated now and boom you already had that!

I did a search for truncate( and got 23 instances in 19 files and the MR addressed them all

Seems like a good deprecation to me.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

It's not clear to me why we need a new method and can't change the return value of the current method.

mondrake’s picture

mondrake
Location 🇮🇹
commented

#6 @catch because in the new method (yes the name is debatable, naming things is hard) I suggest to add a void native return type to make it clear that there is no returned value from the call. We cannot add void to execute because it is inherited from Drupal\Core\Database\Query\Query, and its other implementations do have a return value (in phpdoc at least for the moment). If we narrow to void, then we would force the parent to the impossible task of supporting both void and a concrete return value: https://php.watch/versions/8.0/union-types#:~:text=The%20void%20type%20i....

sivaji_ganesh_jojodae’s picture

sivaji_ganesh_jojodae commented
Status: Needs review » Needs work

Merge conflicts must be resolved.

sivaji_ganesh_jojodae’s picture

sivaji_ganesh_jojodae commented
Status: Needs work » Needs review

Merge conflicts has been resolved.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3569137-nr-bot_cxvxb929.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems like a good deprecation, CR reads fine.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review

Hang on. I think #6 needs more discussion. Why can't the return type of Drupal\Core\Database\Query\Query be documented correctly. Having to call a different method to execute a query depending on the what the query is doing feels like bad API. I think there are other options one of which is leaving the API docs as they are and returning NULL from truncate because the amount of rows are unknowable.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Or, we could introduce two interfaces

  • QueryExecutionReturningRecordsetInterface, with protected function execute(): StatementInterface;
  • QueryExecutionVoidInterface, with protected function execute(): void;

the first to be implemented by the Select base class, the other by the Truncate base class (and ideally in the future by DML statements where we should not expect a value). That would be type strict which would be a win IMHO.

mondrake opened merge request !16067

mondrake’s picture

mondrake
Location 🇮🇹
commented

Funnily, I found out that Query::execute() is not API at the moment... the abstract class is setting it protected!
Then each DB operation base class extends its visibility to public.

Only SelectInterface is having it as an API.

MR!16067 is doing #14 only for Truncate.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Deprecate usage of TRUNCATE ::execute() method » Ensure TRUNCATE ::execute() method returns void
mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes

mondrake changed the visibility of the branch 3569137-deprecate-truncate-execute to hidden.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

All code changes look good to me.
The IS and CR are in order.
The only problem could be that there is no testing added. I leave it up to the committer if this is necessary.
For me it is RTBC.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

The changes look good here, but the change record is very out of date with the MR now. Should we just mention the new interface?

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

Edited the CR.

  • catch committed 702f50bd on 11.x 
    task: #3569137 Ensure TRUNCATE ::execute() method returns void

By:...

  • catch committed 9eb69cba on main 
    task: #3569137 Ensure TRUNCATE ::execute() method returns void

By:...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main and 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !16067

Status: Fixed » Closed (fixed)

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