Needs work
Project:
Drupal core
Version:
main
Component:
database system
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
25 Feb 2026 at 02:17 UTC
Updated:
1 Sep 2026 at 09:55 UTC
Jump to comment: Most recent
Problem/Motivation
Make sense to add to catch dead code and incomplete implementations
Connection::startTransaction() discarding result = transaction instantly commits via __destruct()
Proposed resolution
1. Connection::select/insert/update/delete/merge/upsert/truncate() — You created a query object and threw it away. The query never runs. This is dead code that looks like it does something.
2. Select::countQuery() — Returns a completely new Select object. Discarding means you built a count query for nothing.
3. Select/QueryInterface::execute() — You ran a SELECT query against the database and discarded the result set. The DB did work for nothing. (However, Update/Delete/Insert::execute() is less clear-cut — callers often legitimately don't care about the affected row count or insert ID.)
4. andConditionGroup()/orConditionGroup() — Creates a new condition group that's never attached to anything. Silent logic bug — the condition you thought you added doesn't exist.
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3575577
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:
changes
Comments
Comment #3
andypostComment #4
smustgrave commentedSorry no harm in being review but don't want to accidentally get reviewed. #3575572: Add [#NoDiscard] to Utility classes is determining if this is worth it.
Comment #5
andypostrebased as
insert()method now abstractComment #6
smustgrave commentedLeft a question on the MR.
Comment #7
smustgrave commentedGoing to mark but not sure if there needs to be a CR for any tests that may be doing
startTransaction()in their tests too. Based on that answer I can mark the other one.Comment #8
catchYes let's add a change record.
Comment #9
andypostCR created https://www.drupal.org/node/3619055
Comment #10
dcam commentedI made a minor formatting change to the change record that I felt enhanced its clarity. Otherwise it looked good to me.
I also want to say that I appreciate the explanations of why this is a good idea in the Proposed Resolution section. I look at some of these
#[NoDiscard]issues and wonder "Why?"Comment #11
longwaveThis conflicts with #3569213: Deprecate UPSERT ::execute() method returning a value, until the direction is decided I don't think we should commit either of these.
Comment #12
mondrakeI am -1 on this, for the reasons explained in #3568486: [meta] Change return of ::execute() DML/DDL methods (INSERT, UPDATE, UPSERT, DELETE, MERGE, TRUNCATE) to void. The TRUNCATE DDL operation was already made to returnvoidwhen executed. IMHO, when we run DML operation we should assume that they succeed, and if they don't manage that via exception handling.Comment #13
longwaveComment #14
mondrakeComment #15
mondrakeSorry, I retract #12. Here the #[NoDiscard] is not about the execution of the operation itself, but about not discarding the query object built in Connection that needs to be executed, which is a valid point.
In other words,
makes no sense as the query object is built and discarded without being executed, while
is ok and the attribute will ensure it's executed.
Different is using the result of the execute() method itself. That one is what #3569213: Deprecate UPSERT ::execute() method returning a value and #3569137: Ensure TRUNCATE ::execute() method returns void are dealing with, but that's a separate thing.
Comment #16
mondrakeAFAICS points #2,3,4 from the IS proposed resolution are actually not implemented in the MR, so this could use a IS update. That also explains we do not have massive failures of DML statement executions results not being assigned.
Also note this is eligible for D12 only as the attribute is only added to PHP 8.5.