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.
For UPSERT, returning a value from Upsert::execute() seems irrelevant, also. In core, the returned value seems only used in tests, where we can check results of the operation in alternative ways.
QueryExecutionVoidInterface in D13| Comment | File | Size | Author |
|---|
Show commandsStart 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
mondrakeComment #4
ghost of drupal pastThis is not necessary. We have solid affected rows support everywhere. Yes in upsert too see my comments there.
Yes. This is a repeated comment from #3568486: [meta] Change return of ::execute() DML/DDL methods (INSERT, UPDATE, UPSERT, DELETE, MERGE, TRUNCATE) to void but since it got ignored I need to repeat it.
#3567868-17: \Drupal\Core\Database\Query\Update::execute has wrong documentation for return type was also ignored. Let me copy-paste that too
I am trying not to be just a nuisance here. I know I have not been the most cooperativee in the upsert issue but when you stated you are not interested in my idea I turned to the MR and I am trying to improve it as much as I can. If there really is an affected rows problem then yes please stop relying on it but the IS only says "see discussions" but I do not see anything. Care to point where the problems are actually shown?
Comment #5
andypostYes, this methods must return value as all databases I know return number of affected rows (sometimes even for DDL)
Moreover I'm trying to add attribute to this methods to make sure the result is used via #3575577: Add [#NoDiscard] to database methods
Comment #6
mondrakeComment #7
mondrakeComment #8
daffie commentedLooks good to me.
Just missing a test that Upsert->execute() returns NULL.
The CR needs to be updated.
Comment #9
mondrakerebased, made some fixes based on reviews (thanks!), fleshed out the CR.
Comment #10
daffie commentedLooks good to me.
The IS and the CR are in order.
There is enough testing.
For me it is RTBC.
Comment #11
needs-review-queue-bot commentedThe 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.
Comment #12
mondrakerebased after removal of the shortcut module
Comment #13
longwaveCan we name the flag after what it does, something like
return_affected_rows, the current name is confusing when you read it out of context.I wish there was a better way to do this, but I don't see one. We could use attributes on the calling method instead, but that's just something else to be removed later. Technically you could parse the code and look to see if the result is passed to something or assigned to a variable, but that's way too expensive at runtime.
Comment #14
mondrakeThanks for input, done.
Comment #15
daffie commentedThe remark of @longwave has been addressed.
Back to RTBC.
Comment #16
mondrakeUpdated CR.
Comment #17
longwaveThis will conflict with #3575577: Add [#NoDiscard] to database methods as noted in #5 - @mondrake @daffie please can you answer the questions from @ghost of drupal past and @andypost here as to why this is necessary?
Comment #18
mondrakeIn general, I do not think it's necessary for calling code to inspect the returned value. When you upsert, and it succeeds, the result is that at the end all the records exists, either because they are updated or because they are added. If the upsert fails, conversely, it's going to throw an exception for the error encountered, not return a partial rowcount.
In a specific case, #3586760-21: Use composite key Upsert queries in core, we already had to work around mysqli and pdo mysql returning different values for the affected rowcount.
Comment #19
mondrakeI think #3575577: Add [#NoDiscard] to database methods is a different thing than this, and the two things are independent. Please see #3575577-15: Add [#NoDiscard] to database methods.
Comment #20
daffie commentedThe question of @longwave has been answered.
Back to RTBC.
Comment #21
needs-review-queue-bot commentedThe 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.
Comment #22
mondrakerebased and bumped deprecation version to 12.1