DatabaseExceptionWrapper completely hides *unexpected* errors [#1831184]

Apply this patch, try to install Drupal, and check the actual exception messages.

PDOException: SQLSTATE[HY000]: General error: user-supplied statement does not accept constructor arguments in \core\lib\Drupal\Core\Database\Connection.php on line 322

Call Stack:
    0.0004     324496   1. {main}() \core\install.php:0
    0.0013     334256   2. install_drupal() \core\install.php:36
    0.0013     336952   3. install_begin_request() \core\includes\install.core.inc:82
    0.1443    1512608   4. install_verify_completed_task() \core\includes\install.core.inc:377
    0.1444    1512672   5. Drupal\Core\KeyValueStore\StorageBase->get() \core\includes\install.core.inc:920
    0.1444    1512840   6. Drupal\Core\KeyValueStore\DatabaseStorage->getMultiple() \core\lib\Drupal\Core\KeyValueStore\StorageBase.php:40
    0.1444    1513320   7. Drupal\Core\Database\Connection->query() \core\lib\Drupal\Core\KeyValueStore\DatabaseStorage.php:55
    0.1445    1514184   8. Drupal\Core\Database\Connection->prepareQuery() \core\lib\Drupal\Core\Database\Connection.php:521
    0.1445    1514344   9. PDO->prepare() \core\lib\Drupal\Core\Database\Connection.php:322

Comment	File	Size	Author
#5	1831184_5.patch	7.66 KB	chx
#5
#5	interdiff.txt	2.12 KB	chx
#4	db-exception-wrapper.4.patch	5.42 KB	sun
#4
#2	db-exception-wrapper.2.patch	5.35 KB	sun
#2
	db-exception-wrapper.0.patch	595 bytes	sun

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

3 November 2012 at 20:51

Status:

Needs review

» Needs work

The last submitted patch, db-exception-wrapper.0.patch, failed testing.

Comment #2

sun

German

Karlsruhe

CreditAttribution: sun commented 3 November 2012 at 21:56

Status:

Needs work

» Needs review

File	Size
db-exception-wrapper.2.patch	5.35 KB

Attached patch fixes the hidden PDO error under the hood.

It does not change the fact that the DatabaseExceptionWrapper hides logical code errors from developers, instead of only catching database, connection, and statement exceptions that relate to an actual database/query operation.

Comment #3

sun

German

Karlsruhe

CreditAttribution: sun commented 3 November 2012 at 21:58

Oh, and yeah, color me extremely confused regarding how this is able to work at all right now:

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -512,7 +512,7 @@ public function query($query, array $args = array(), $options = array()) {
-      if ($query instanceof DatabaseStatementInterface) {
+      if ($query instanceof StatementInterface) {

@@ -543,7 +543,7 @@ public function query($query, array $args = array(), $options = array()) {
-        $query_string = ($query instanceof DatabaseStatementInterface) ? $stmt->getQueryString() : $query;
+        $query_string = ($query instanceof StatementInterface) ? $stmt->getQueryString() : $query;

DatabaseStatementInterface only existed in D7. StatementInterface is also not imported or aliased in any way.

Comment #4

sun

German

Karlsruhe

CreditAttribution: sun commented 3 November 2012 at 22:27

File	Size
db-exception-wrapper.4.patch	5.42 KB

Fixed typos/phpDoc.

Comment #5

chx CreditAttribution: chx commented 31 December 2012 at 04:03

Assigned:

Unassigned

» Crell

File	Size
interdiff.txt	2.12 KB
1831184_5.patch	7.66 KB

Added tests. Assigning to Crell for review. Re #3 StatementInterface is also not imported because it lives in the same namespace as Connection. Re #2 that's a much bigger bag of hurt.

Edit: this test is dedicated to webchick ;)

Comment #6

Crell CreditAttribution: Crell commented 31 December 2012 at 08:04

Status:

Needs review

» Reviewed & tested by the community

PDO is really screwy at times... *sigh*

Comment #7

catch

he/him

English

CreditAttribution: catch commented 2 January 2013 at 11:54

Status:

Reviewed & tested by the community

» Fixed

Looks good to me, not sure how this wasn't blowing up in the first place. Committed/pushed to 8.x.

Comment #8

chx CreditAttribution: chx commented 3 January 2013 at 02:49

It wasnt blowing up because #0 is just theoretical code, it's not something we had and that the wrong exception was thrown (or fatal?), that's a rare case which wasnt tested.