Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Hi there.
StatementInterface::rowCount() method return the number of rows affected by the last DELETE, INSERT, or UPDATE statement executed.
(according to php.net documentation and api.drupal.org)
But in one core test (name: 'Select tests, complex') this method used to get row count for SELECT statement.
In MySQL it works well, but this behaviour is not guaranteed for all databases.
For example I work to port Oracle Driver for D8 now, and this test fails.
I have created patch that allow skip needed test methods for not MySQL drivers, but maybe a solution will be just to remove them.
Comment | File | Size | Author |
---|---|---|---|
#31 | 2146733-select-rowcount-31.patch | 11.59 KB | andypost |
#31 | interdiff.txt | 1.61 KB | andypost |
#23 | 2146733_22.patch | 10.46 KB | chx |
Comments
Comment #1
andypostSuppose better to get rid of this fragile test at all!
Better to ask @Crell
Comment #2
andypostAll functions in the file use
foreach()
to calculate record count, so let's implement a helper method for that and replace this wrong usageComment #3
InternetDevels CreditAttribution: InternetDevels commentedI looked how this done in another tests and implement to this one.
Comment #4
chx CreditAttribution: chx commentedThat is correct. It's not what we dreamed up in Drupal, this is PDO: http://php.net/manual/en/pdostatement.rowcount.php
Comment #5
Crell CreditAttribution: Crell commentedSigh. I hate SQL. Yes, this looks correct.
Comment #6
andypost3: drupal-fix_SelectComplexTest-2146733-3.patch queued for re-testing.
Comment #7
alexpottI 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.
Comment #8
Crell CreditAttribution: Crell commentedAlex: 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? :-/
Comment #9
andypostAdded method override, named exception and test
Comment #10
andypostRight patch (previous does not have exception)
Not sure it's possible proceed without detection of query type
Comment #12
chx CreditAttribution: chx commentedThe 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.
Comment #13
chx CreditAttribution: chx commentedComment #16
andypostwithout parsing a query it is not possible.
Suppose there's other ways:
1) add a property to database driver
rowCountSupported
and set this inConnection::query()
for result typeDatabase::RETURN_STATEMENT
2) as we wanna throw exception add property
allowRowCount
toStatementPrefetch
andStatement
classes and set as in 1Both approaches does not save us in case of
db_query()
without parsingComment #17
andypostApproach 2
Comment #18
chx CreditAttribution: chx commenteddb_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 rundb_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.Comment #19
chx CreditAttribution: chx commentedTo 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.
Comment #21
andypostShould fix most failures, needs some sleep...
Comment #22
Crell CreditAttribution: Crell commented#21 seems like a reasonable approach to me. chx?
Comment #23
chx CreditAttribution: chx commentedI resolved a couple TODOs and turned the whole ship around: we allow calling rowCount when that will happen. Otherwise, no rowCount for you.
Comment #24
andypostI agree with this, just going to add a test for "db_query(insert" case
Comment #25
andypostAnd now it's looks finished
Comment #26
andypostOne more fix, there was no tests for result of
db_insert()
Comment #28
andypostActually inserted 3 records but reported 5 affected... no idea
Comment #29
andypostthe proper fix,
db_insert()
returns last inserted ID or undefined in case of multi-insertComment #30
Crell CreditAttribution: Crell commentedCalling code should never have to specify the return mode. That's an internal property callers should never have to think about.
Comment #31
andypostSo let's just skip the insert test changes hunk
Comment #32
Crell CreditAttribution: Crell commentedOK, let's do. Thanks!
Comment #33
xjm31: 2146733-select-rowcount-31.patch queued for re-testing.
Comment #35
alexpott31: 2146733-select-rowcount-31.patch queued for re-testing.
Comment #36
alexpottMoving back to rtbc since random re-test was the cause of this not being rtbc.
Comment #37
alexpottCommitted 588e678 and pushed to 8.x. Thanks!
Yay for Drupal becoming a little bit more db agnostic.
Comment #38
dcrocks CreditAttribution: dcrocks commentedThis 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.
Comment #39
dcrocks CreditAttribution: dcrocks commentedWhat 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?
Comment #40
andypost#2167507-13: Fix rowCount query usage in pgsql and sqlite drivers fixes pgsql that was wrongly patched here