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.
Problem
Connection::query()
is designed to containSELECT
statements only.rowCount()
onSELECT
is illegal.- We have enforced this in a recent patch which makes
$database->query->rowCount()
blow up. - The SQLite and Postgesql drivers runs
query
withUPDATE
. This is not supported but because it is inside the database driver it gets a pass.
As a corollary, both drivers blows up and this is by design.
Proposed resolution
Change the illegal query that drivers runs to better mimic db_update
by setting the right return
option. This actually shortens the code.
Comment | File | Size | Author |
---|---|---|---|
#23 | interdiff.rowcount-exception-message.txt | 1.03 KB | sun |
#16 | 2167507_16.patch | 2.68 KB | andypost |
#16 | interdiff.txt | 615 bytes | andypost |
#15 | interdiff.txt | 596 bytes | andypost |
#13 | interdiff.txt | 769 bytes | andypost |
Comments
Comment #1
webchickConfirmed.
git bisect should be able to help nail down the culprit.
Comment #2
aspilicious CreditAttribution: aspilicious commenteddid you remove the files directory when reinstalling??
Comment #3
dcrocks CreditAttribution: dcrocks commentedI could not use 'git bisect' because the failure is in the middle of install. So I did a manual 'bisect' based on the calendar and think I have found the suspect. I did a clean clone of current 8.x and reverted the patch for issue #2146733: Select queries should not use rowCount() to calculate number of rows and install was successful. However I now see a lot of errors
I will take a look at that patch for now.
Comment #4
andypostI don't think that rowCount() caused this because it useless for sqlite, suppose other issue caused this failure
Comment #5
dcrocks CreditAttribution: dcrocks commentedHowever, D8 doesn't install on SQLite with the patch in, but does with the patch reverted. I'm not arguing to revert this patch, only to try to determine the error causing the install failure.
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe StatementPrefetch has not been updated for this change. Not sure why it causes the issue.
Comment #7
dcrocks CreditAttribution: dcrocks commentedComment #8
dcrocks CreditAttribution: dcrocks commentedI have a small patch that seems to fix the problem. Please, someone more knowledgeable about databases review this. And someone test it. My tests were of the trivial kind.
Comment #9
chx CreditAttribution: chx commentedThanks for the bug report and the patch! It was revealing. But I believe this to be a more correct patch. I also added some comments.
Comment #10
chx CreditAttribution: chx commentedKindly forget that :) I meant this.
Comment #11
chx CreditAttribution: chx commentedComment #13
andypostpgsql needs the same fix after #2146733-22: Select queries should not use rowCount() to calculate number of rows
Comment #14
dcrocks CreditAttribution: dcrocks commentedPatch installed cleanly and install of clone of D8 on SQLite was successful. Don't have PGsql to test.
Thanx @chx and @andypost for doing this right.
Comment #15
andypostThe proper fix for pgsql, tested with
psql (9.3.2)
Comment #16
andypostSuppose we could close duplicate issue #2001350: [meta] Drupal cannot be installed on PostgreSQL
Comment #17
andypostComment #18
mradcliffeI tested with the patch and without the patch on PostgreSQL 9.2.4.
No, #2001350: [meta] Drupal cannot be installed on PostgreSQL should not be closed because PostgreSQL wasn't successfully installed. The install stopped at the same point from October, 2013, comment #74 re: comment_entity_statistics.
I think this issue should be renamed to be more specific to the actual issue it is trying to fix instead of attempting to fix "does not install", which is a meta issue now. So I think whatever this issue is trying to solve it may have solved it, but it did not fix the entire problem.
Comment #19
mradcliffeChanged title to be more specific.
Comment #20
dcrocks CreditAttribution: dcrocks commentedThen can we focus on this, get it reviewed, and committed?
Comment #21
chx CreditAttribution: chx commentedI am not sure why this wasn't RTBC'd by the testers above but apparently both databases are fixed so I will do the honors despite I wrote half of it.
Comment #22
chx CreditAttribution: chx commentedPs. I checked and the suggested change to allowRowCount in the postgresql driver query() now makes it match the generic query() .
Comment #23
sunCan we also throw a proper exception message, please? Often times, we're catching database exceptions and just
$e->getMessage()
or to re-throw them. Without a message, you have no clue what happened.Potential fix attached as interdiff.
Comment #24
mradcliffeWould including the query potentially expose sensitive information? I don't think so because it's probably going to be for php developers only who are trying to debug.
Comment #25
sunI don't want to hold this fix off any longer, because the broken SQLite is quite annoying for everyone working on the installer.
Hence moved into a separate issue: #2171359: Output proper exception message for database RowCountExceptions
Comment #26
Crell CreditAttribution: Crell commentedPer request from chx, /me nods approvingly.
Comment #27
webchickAwesome! Great work on this, folks. Looking forward to installing in SQLite again. :)
Committed and pushed to 8.x.