Select queries should not use rowCount() to calculate number of rows

Suppose better to get rid of this fragile test at all!
Better to ask @Crell

All functions in the file use foreach() to calculate record count, so let's implement a helper method for that and replace this wrong usage

  /**
   * Tests range queries.
   *
   * The SQL clause varies with the database.
   */
  function testRange() {
    $query = db_select('test');
    $query->addField('test', 'name');
    $query->addField('test', 'age', 'age');
    $query->range(0, 2);
    $result = $query->execute();

    $this->assertEqual($result->rowCount(), 2, 'Returned the correct number of rows.');
  }

  /**
   * Tests distinct queries.
   */
  function testDistinct() {
    $query = db_select('test_task');
    $query->addField('test_task', 'task');
    $query->distinct();
    $result = $query->execute();

    $this->assertEqual($result->rowCount(), 6, 'Returned the correct number of rows.');
  }

I looked how this done in another tests and implement to this one.

That is correct. It's not what we dreamed up in Drupal, this is PDO: http://php.net/manual/en/pdostatement.rowcount.php

PDOStatement::rowCount() returns the number of rows affected by a DELETE, INSERT, or UPDATE statement.

Sigh. I hate SQL. Yes, this looks correct.

3: drupal-fix_SelectComplexTest-2146733-3.patch queued for re-testing.

I think we should be throwing an exception if rowCount() is called on a select query so that we can ensure that we never depend on MySQL's ability to do this. If we do, then exception should be tested.

Alex: That's going to turn an 4 line patch into a 50 line patch, excluding tests. I'm not sure off hand how the result object can tell what type of query it came from. (It may be easy, but it could also be quite hard; I've not checked.) Are you sure about that? :-/

Added method override, named exception and test

Right patch (previous does not have exception)

Not sure it's possible proceed without detection of query type

The statement should not be returned other by select. It makes no sense to fetch the results of an UPDATE. if it gets returned by other, that's a severe bug.

Also, never, ever try parse a SQL query.

how the result object can tell what type of query it came from

without parsing a query it is not possible.

Suppose there's other ways:
1) add a property to database driver rowCountSupported and set this in Connection::query() for result type Database::RETURN_STATEMENT
2) as we wanna throw exception add property allowRowCount to StatementPrefetch and Statement classes and set as in 1

Both approaches does not save us in case of db_query() without parsing

Approach 2

db_query('UPDATE')->execute()->rowCount() throws an exception? Excellent! What else can we do to stop people running such broken code? While db_select is optional, db_update/db_insert/db_delete is not. If you run db_query('UPDATE') you have a bug and this reveals that. Those queries might work on MySQL but good luck on anything else -- check whether your favorite non-MySQL database has an Insert or Update override. If it has, then you can see the problem.

To make this even more concrete: inserting/updating a blob works from a SQL query on MySQL but if the pgsql classes are any indicator they won't on PostgreSQL.

Should fix most failures, needs some sleep...

#21 seems like a reasonable approach to me. chx?

I resolved a couple TODOs and turned the whole ship around: we allow calling rowCount when that will happen. Otherwise, no rowCount for you.

I agree with this, just going to add a test for "db_query(insert" case

And now it's looks finished

One more fix, there was no tests for result of db_insert()

Actually inserted 3 records but reported 5 affected... no idea

the proper fix, db_insert() returns last inserted ID or undefined in case of multi-insert

+++ b/core/modules/system/lib/Drupal/system/Tests/Database/InsertTest.php
@@ -42,10 +44,10 @@ function testSimpleInsert() {
-    $query = db_insert('test');
+    $query = db_insert('test', array('return' => Database::RETURN_AFFECTED));

Calling code should never have to specify the return mode. That's an internal property callers should never have to think about.

So let's just skip the insert test changes hunk

OK, let's do. Thanks!

31: 2146733-select-rowcount-31.patch queued for re-testing.

31: 2146733-select-rowcount-31.patch queued for re-testing.

Moving back to rtbc since random re-test was the cause of this not being rtbc.

Committed 588e678 and pushed to 8.x. Thanks!

Yay for Drupal becoming a little bit more db agnostic.

This patch causes Drupal8 to NOT install on SQLite. See #2167507: Fix rowCount query usage in pgsql and sqlite drivers. Apparently SQLite also has issues with rowCount and Select.

What is going to tell a database driver writer that they have to be aware of the setting for this flag for those database operations for which rowCount is legitimate, and for the one for which it is not?

#2167507-13: Fix rowCount query usage in pgsql and sqlite drivers fixes pgsql that was wrongly patched here