Fix DeleteQuery::execute() return value documentation and DeleteQuery_sqlite documentation (fix Delete::execute() documentation in Drupal 8)

94 characters; need a linebreak before 80; otherwise great.

Good to go.

Um... Maybe it should say that the return value is *typically* the number of deleted rows? What else could it be and in what way does it depend on the database connection? I don't really think this makes the situation much clearer.

Also, I think we need to do this in D8 first:
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Database!Query!De...
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Database!Query!De...
and other classes/interfaces

Apparently I remembered wrong. Here's the code:

        case Database::RETURN_AFFECTED:
          return $stmt->rowCount();

Here's the official docs: http://www.php.net/manual/en/pdostatement.rowcount.php and a relevant #805858: Affected rows inconsistent across database engines well researched issue.

Reading the latter,

In line with the ANSI SQL standard, both PostgreSQL and SQLite define "affected rows" as all rows matched by the query.

So apparently the standard does define affected rows... and the problem really is only with UPDATEs not DELETEs. Well then, thanks for catching me being too cautious -- let's make the patch say definitely it is the number of deleted rows.

Excellent! Thanks for the detailed research notes. I think this is now a good Novice issue. The task:

Search out all Delete query classes/interfaces in Drupal 8, and make sure their execute() return value docs say that the return value is the number of deleted rows.

I just found another occurrence but the documentation seems right:

/**
 * SQLite specific implementation of DeleteQuery.
 *
 * When the WHERE is omitted from a DELETE statement and the table being deleted
 * has no triggers, SQLite uses an optimization to erase the entire table content
 * without having to visit each row of the table individually.
 *
 * Prior to SQLite 3.6.5, SQLite does not return the actual number of rows deleted
 * by that optimized "truncate" optimization.
 */
class Delete extends QueryDelete {

It sounds like the SQLite class's execute() documentation should say that it returns the number of rows deleted, except in the case where there is no WHERE, then? Or does it? The documentation you pasted here is just the class documentation, not the execute() method.

And aren't there more cases than just core/lib/Drupal/Core/Database/Query/Delete.php where the execute() documentation needs an update?

@jhodgdon I just found this extra case. The documentation is in the class and nothing in the execute method.

Maybe we can move some documentation from the class to the execute method:

/**
 * SQLite specific implementation of DeleteQuery.
 *
 * When the WHERE is omitted from a DELETE statement and the table being deleted
 * has no triggers, SQLite uses an optimization to erase the entire table content
 * without having to visit each row of the table individually.
 *
 * Prior to SQLite 3.6.5, SQLite does not return the actual number of rows deleted
 * by that optimized "truncate" optimization.
 */
class Delete extends QueryDelete {
  public function execute() {
  ...

So, you can found similar documentation in other places like TRUCATE executes:

* Executes the TRUNCATE query.
*
* @return
* Return value is dependent on the database type.

...but all those cases are out of the issue scope.

Right, let's stick with the execute() method on Delete classes for this issue, and yes, let's move the documentation on the return value of execute() to the execute() method on the SQLite class.

And maybe on the delete interface, we should mention that some engines do make exceptions to the rule that execute() always returns the number of rows.

Anyway, the supplied patch does not appear to fix all the cases of execute() on all Delete classes and interfaces, right?

In order to find all the files that implemented the execute() function of Delete, I made a search for all files that contained the string "function execute()", and then made another search for the string "delete", just on the files that were returned by the first search.

The only result besides the file previously patched (a/core/lib/Drupal/Core/Database/Query/Delete.php) was a/core/lib/Drupal/Core/Database/Driver/sqlite/Delete.php.

I added the {@inheritdoc} to its execute() function.

Thanks! I agree with your analysis and the SQLite class is the only one that overrides the base Delete::execute() method.

However, as noted above, in SQLite, the execute() method does not always return the number of rows deleted, so it should not be inheriting the documentation from the base class. It needs its own documentation header.

Also, when attaching a patch, please name it *.patch so it gets flagged as a patch for testing/review. Thanks!

I added its own documentation to the SQLite implementation and mentioned on the parent class that some implementations work differently.

Same as #17 but with unix line ending

#22 failed because of changes in #2465221: Raise the minimun version requirement for SQLite to 3.6.8. Beginner error, i forgot to git pull.

I think that now, because SQLite support starts at 3.6.8 which always return the number of rows deleted , #9 is relevant and should be used.

tagging in hopes the SQLite folks see this... I do not know the answer.

In response to #24 and #25, it seems the "number of rows deleted" bug was fixed in 3.6.5.

Added beta evaluation.

This was actually fixed with issue 2465221. There is no longer a special behaviour to Delete for sqlite and the documentation in question has already been removed.

In that case, it's a D7 issue.

How does this look? There didn't appear to be anything unclear about the documentation in 7.x but I modified it a little to limit lines to 80 characters and added @return, like the D8 patch.

I removed the extra whitespace after "entire" and moved "deleted" up a line.
I left the spelling as "optimization" because, as far as I can tell, Drupal prefers the Oxford spelling.[1]

There wasn't anything wrong with the changes #22; it's just not applicable to D8 any more and it doesn't apply to D7.

[1] https://www.drupal.org/style-guide/content#english

yes, please use American English spelling with a Z not S in optimization.

+1, thanks!

+ * @return
+ *  The return value is the number of deleted rows.
....
  */
+ * Prior to SQLite 3.6.5, SQLite does not return the actual number of rows
+ * deleted by that optimized "truncate" optimization.
  */
 class DeleteQuery_sqlite extends DeleteQuery {
   public function execute() {

This doesn't look right to me; classes don't have return values. Perhaps the @return is supposed to be on the execute() method instead?

And it's written pretty definitively but that conflicts with the documentation below which says that it depends on SQLite version...

Also, this patch no longer seems to deal with the original issue, which is DeleteQuery::execute() itself, which still says the vague/unhelpful "The return value is dependent on the database connection"....

Doh. Actually, we should go back to Drupal 8 first. The Delete::execute() method does not say what its return value is. All it says is:
The return value is dependent on the database connection.

That will teach me to accept it when someone else says the issue is fixed, when it isn't.

Looks good, thanks!

Also I noticed #2594845: Many database driver classes have no class doc blocks while checking this... Sigh. Who lets all these documentless-classes into Core???!?

That's much better. :) As a documentation-only fix, this change is considered "rc eligible" and can be committed any time during RC. Reference: https://www.drupal.org/core/d8-allowed-changes#rc

Committed and pushed to 8.0.x. Thanks!

Done for 7.x

+++ b/includes/database/query.inc
@@ -845,8 +845,8 @@ class DeleteQuery extends Query implements QueryConditionInterface {
+   *   The number of rows affected by the delete query. ¶

This line has an extra space at the end.

By the way, there was a long discussion above about SQLite and whether it was correct that all of the databases return the number of rows affected. So I looked at the execute() method on the D7 SQLite DeleteQuery_sqlite class, and the execute method definitely returns the number of rows (the implementation has some code that makes sure this happens).

So, in this patch, we should remove this text in the class header docs... or actually, we should move it into a code comment:

 * When the WHERE is omitted from a DELETE statement and the table being deleted
 * has no triggers, SQLite uses an optimization to erase the entire table content
 * without having to visit each row of the table individually.
 *
 * Prior to SQLite 3.6.5, SQLite does not return the actual number of rows deleted
 * by that optimized "truncate" optimization.

This information should be turned into // comments and put just before this if() in the execute() method:

 * When the WHERE is omitted from a DELETE statement and the table being deleted
 * has no triggers, SQLite uses an optimization to erase the entire table content
 * without having to visit each row of the table individually.
 *
 * Prior to SQLite 3.6.5, SQLite does not return the actual number of rows deleted
 * by that optimized "truncate" optimization.

Also maybe say something like "But we want to return the number of rows affected, so we calculate it directly.

Please review attached one.

+++ b/includes/database/query.inc
@@ -845,11 +845,18 @@ class DeleteQuery extends Query implements QueryConditionInterface {
+    // When the WHERE is omitted from a DELETE statement and the table being
+    // deleted has no triggers, SQLite uses an optimization to erase the entire
+    // table content without having to visit each row of the table individually.
+    // Prior to SQLite 3.6.5, SQLite does not return the actual number of rows
+    // deleted by that optimized "truncate" optimization. But we want to return
+    // the number of rows affected, so we calculate it directly.

This information needs to go into the SQLite Delete class, not the generic class. Sorry, my comment above wasn't specific and I pasted in the comment again instead of the if() statement I was referring to. But you need to find the place in the SQLite class where it calculates the number of rows affected, and put this comment there.

Hi,

I moved the comment to the SQLite Delete class as mentionned in #48 and removed the class header comment.

Tagging the Drupal 7 backport as Novice -- this still needs review to see if this is RTBC and all the feedback has been addressed, and to further iterate if necessary.

Changing the status of this ticket.

Committed to 7.x - thanks!

I removed this on commit since we don't have @inheritdoc anywhere else in Drupal 7 right now and I didn't want to add it to just this one place:

diff --git a/includes/database/sqlite/query.inc b/includes/database/sqlite/query.inc
index 1ac4464..c9c028b 100644
--- a/includes/database/sqlite/query.inc
+++ b/includes/database/sqlite/query.inc
@@ -101,9 +101,6 @@ class UpdateQuery_sqlite extends UpdateQuery {
  * SQLite specific implementation of DeleteQuery.
  */
 class DeleteQuery_sqlite extends DeleteQuery {
-  /**
-  * {@inheritdoc}
-  */
   public function execute() {
     // When the WHERE is omitted from a DELETE statement and the table being
     // deleted has no triggers, SQLite uses an optimization to erase the entire