\Drupal\Core\Database\Query\Update::execute has wrong documentation for return type

IMHO we should not rely on the int value returned here. Only return TRUE or fail with an exception. The row count returned by different drivers is not consistent. See similar point for the UPSERT operation in #2547493-33: Add support for unique / primary key constraints composed of multiple fields for Upsert queries.

I’ve updated the return type and opened an MR for this documentation fix.

Believe your local may have been on 11.3 not 11.x, you may need to rebase your local branch and push that. MR is showing 1000s of changes

Retracted, see #11

#7 IMHO in a concurrent db that information returned here should be avoided as much as possible, given that rows affected (= the rows that the UPDATE conditions would return), and rows changed (= the affected rows actually changed) numbers may be impacted by concurrent operations.

If knowing the precise number is absolutely necessary, then locking should be used, in pseudo:

START TRANSACTION
SELECT ... WHERE {update conditions} ... FOR UPDATE
count the result, that gives a reliable number
UPDATE ... WHERE {update conditions}
COMMIT

Sorry I made a mistake in #8, will retract and rewrite it below.

Anyway: affected rows returns in Drupal the rows matching the condition, and not necessarily the changed ones. So I am not sure what #9 is actually doing.

#7 IMHO in a concurrent db that information returned here should be avoided as much as possible, given that rows matched (= the rows that the UPDATE conditions would return), and rows changed (= the matched rows actually changed) numbers may be impacted by concurrent operations.

Also as per #3248575: Documentation conflict re: affected rows vs. matched rows, Drupal MySQL driver sets

      // Make sure MySQL returns all matched rows on update queries including
      // rows that actually didn't have to be updated because the values didn't
      // change. This matches common behavior among other database systems.
      // @phpstan-ignore class.notFound
      (\PHP_VERSION_ID < 80400 ? \PDO::MYSQL_ATTR_FOUND_ROWS : Mysql::ATTR_FOUND_ROWS) => TRUE,

which makes me suspect PgSQL and SQLite would have the same default behaviour, then.

If knowing the precise number is absolutely necessary, then locking should be used, in pseudo:

START TRANSACTION
SELECT ... WHERE {update conditions} ... FOR UPDATE
count the result, that gives a reliable number
UPDATE ... WHERE {update conditions}
COMMIT

where {update conditions} should be set in a way to ensure matched rows=changed rows.

Postgres seems to return matched rows since 9.0 => https://www.php.net/manual/en/function.pg-affected-rows.php

#13 OK, but then a boolean is enough in this case, right? As long as it's not 0 we don't really need to know the exact number.

SQLite viceversa seems to return the changed rows and not the matched ones => https://www.php.net/manual/en/sqlite3.changes.php

MR!14392 is a proposal to return a bool.

I spot checked usage of update across core's code base, and AFAICS the value returned by ::execute is

• not assigned to a variable (most common case), or
• explicitly cast to boolean, or
• when checked in an if, checked in a boolean way i.e. that its value is non-zero

#21 yes casting to boolean is not safer per se, but in practice it does not look like the actual rowcount is used for its int value, apart in the update tests themselves.

I stand by #4. In fact IMHO for all of UPDATE, UPSERT and DELETE operations, what we should assume is that if the execution is not failing, then all the records identified by the conditions are eventually affected by the operation itself; trying to interpret the various row-affected or info internals seems to me not reliable (e.g. see the UPSERT related issue, or think someone in MySql set Mysql::ATTR_FOUND_ROWS to false: then if the row is found but not changed, update will return 0 or FALSE which is not what Drupal would expect, and in fact the row would be set to the values indicated by UPDATE).

If casting is not safe why not just return nothing? Make the method void and advise using SELECT FOR UPDATE statement as workaround.

Fully agree. That should be the end game. BC is in the way, of course.

Filed #3568486: [meta] Change return of ::execute() DML/DDL methods (INSERT, UPDATE, UPSERT, DELETE, MERGE, TRUNCATE) to void not to derail this issue further.

NW for #6

.

Yes, this should be on main and so should the MR.

Appears to have a phpstan error. Most likely the baseline needs to be updated. Believe the artifact can be retrieved directly from the MR

Updated the phpstan baseline using the artifacts from the MR.

@mondrake or @chi thoughts on what's in the MR?

I am removing the Novice tag from this issue because I am not sure what the next task is for a new contributor.

I’m using this documentation as a source: https://www.drupal.org/community/contributor-guide/task/triage-novice-is...

Updated the return type for ExceptionHandler::handleExecutionException() as per #39.

Removed outdated ignore rules from phpstan-baseline.php that are no longer triggered, fixing ignore.unmatched errors in the pipeline.

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.