Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

DBAL-371: Forward the SQLSTATE error code to DBALException #221

Closed
wants to merge 10 commits into from

5 participants

Matthieu Napoli Benjamin Eberlei Miha Vrhovnik Bart van den Burg Markus Bachmann
Matthieu Napoli

Forward the SQLSTATE error code to DBALException

http://www.doctrine-project.org/jira/browse/DBAL-371

Matthieu Napoli Issue DBAL-371
Forward the SQLSTATE error code to DBALException
8d707a5
Matthieu Napoli

SQL error code seems to differ according to the DBMS :(

Matthieu Napoli

Now the test is passing, but there are errors with pgsql, I don't understand because they are SQL errors:

SQLSTATE[25P02]: In failed sql transaction: 7 ERROR: current transaction is aborted, commands ignored until end of transaction block.

Are these tests supposed to pass?

Benjamin Eberlei
Owner

That would be wrong imho, the DBALException is used for lots of different stuff. The exception hierachy is a bit borked here. I would accept the PR, if you introduce a new Exception \Doctrine\DBAL\Driver\DriverException extends DBALException that does this.

Matthieu Napoli

@beberlei I added \Doctrine\DBAL\Driver\DriverException as you requested, I see what you mean by "DBALException is used for lots of different stuff".

I didn't remove DBALException::driverExceptionDuringQuery even though it's not used anymore (replaced by DriverException::driverExceptionDuringQuery). I'm not sure wether I should remove it, what do you think?

Thanks

Matthieu Napoli

I'm giving up on postgresql :( I don't get how a simple create table would not work

Markus Bachmann Baachi commented on the diff December 14, 2012
lib/Doctrine/DBAL/Driver/DriverException.php
... ...
@@ -0,0 +1,19 @@
  1
+<?php
  2
+
  3
+namespace Doctrine\DBAL\Driver;
  4
+
  5
+use Doctrine\DBAL\DBALException;
  6
+
  7
+class DriverException extends DBALException
  8
+{
  9
+    public static function driverExceptionDuringQuery(\Exception $driverEx, $sql, array $params = array())
  10
+    {
  11
+        $msg = "An exception occurred while executing '".$sql."'";
  12
+        if ($params) {
  13
+            $msg .= " with params ".json_encode($params);
  14
+        }
  15
+        $msg .= ":\n\n".$driverEx->getMessage();
2
Markus Bachmann
Baachi added a note December 14, 2012

Newlines in an exception message? Really?

Matthieu Napoli
mnapoli added a note December 14, 2012

erm, this is an exact copy of the same method in DBALException: https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/DBALException.php#L39

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Miha Vrhovnik

@mnapoli, @beberlei I've spent past 4 hours dissecting this, but it seems that something is seriously wrong with Postgresql tests. Half of the tables including dbal371 get created in the postgresql database, and the other half in designated databases the one specified in phpunit.xml, doctrine_tests in my case.
I'm very time constraint till the end of the year and I don't know if I'll be able to help more.

psql -U postgres
psql (9.2.1)
Type "help" for help.

postgres=# \c postgres
You are now connected to database "postgres" as user "postgres".
postgres=# \d
                    List of relations
 Schema |           Name            |   Type   |  Owner
--------+---------------------------+----------+----------
 public | alter_table               | table    | postgres
 public | alter_table_foreign       | table    | postgres
 public | column_comment_test       | table    | postgres
 public | column_comment_test2      | table    | postgres
 public | dbal371                   | table    | postgres
 public | doctrine_test_view        | view     | postgres
 public | domains                   | table    | postgres
 public | list_table_columns        | table    | postgres
 public | list_table_indexes_test   | table    | postgres
 public | list_tables_test          | table    | postgres
 public | test_autoincrement        | table    | postgres
 public | test_autoincrement_id_seq | sequence | postgres
 public | test_blob_table           | table    | postgres
 public | test_create_fk            | table    | postgres
 public | test_create_fk3           | table    | postgres
 public | test_create_fk4           | table    | postgres
 public | test_create_index         | table    | postgres
 public | test_fk_base              | table    | postgres
 public | test_fk_rename            | table    | postgres
 public | test_foreign              | table    | postgres
 public | test_table                | table    | postgres
 public | view_test_table           | table    | postgres
 public | write_table_id_seq        | sequence | postgres
(23 rows)
postgres=# \c doctrine_tests
You are now connected to database "doctrine_tests" as user "postgres".
doctrine_tests=# \d
                      List of relations
 Schema |             Name              |   Type   |  Owner
--------+-------------------------------+----------+----------
 public | autoinc_table                 | table    | postgres
 public | autoinc_table_add             | table    | postgres
 public | autoinc_table_add_id_seq      | sequence | postgres
 public | autoinc_table_drop            | table    | postgres
 public | autoinc_table_drop_id_seq     | sequence | postgres
 public | autoinc_table_id_seq          | sequence | postgres
 public | caching                       | table    | postgres
 public | dbal204_test_prefix           | table    | postgres
 public | dbal204_without_prefix        | table    | postgres
 public | dbal91_something              | table    | postgres
 public | ddc1372_foobar                | table    | postgres
 public | domain_type_test              | table    | postgres
 public | dropcreate_sequences_test_seq | sequence | postgres
 public | fetch_table                   | table    | postgres
 public | list_sequences_test_seq       | sequence | postgres
 public | master_slave_table            | table    | postgres
 public | modify_limit_table            | table    | postgres
 public | modify_limit_table2           | table    | postgres
 public | modify_limit_table2_id_seq    | sequence | postgres
 public | nontemporary                  | table    | postgres
 public | portability_table             | table    | postgres
 public | sequences                     | table    | postgres
 public | test_create_fk1               | table    | postgres
 public | test_create_fk2               | table    | postgres
 public | type_conversion               | table    | postgres
 public | write_table                   | table    | postgres
 public | write_table_id_seq            | sequence | postgres
(27 rows)

P.S.

phpunit --version
PHPUnit 3.7.7 by Sebastian Bergmann.
php -v
PHP 5.4.9-1.pi (cli) (built: Dec 14 2012 13:58:25)
Copyright (c) 1997-2012 The PHP Group
Zend Engine v2.4.0, Copyright (c) 1998-2012 Zend Technologies
    with Xdebug v2.2.1, Copyright (c) 2002-2012, by Derick Rethans
Miha Vrhovnik

The problem is probably in getSchemaManager()->tablesExist() I'm going to assume that it temporary switches the database to get the tables list, but then doesn't switch back. If I comment all the tests with a call to that method. Some tests fail because the table they attempt to create already exists, but none of the created tables gets created in the postgresql database.

P.S.
calling $this->_conn->getDatabase(); before and after the call to getSchemaManager()->tablesExist() outputs the doctrine_tests which seems to be wrong...

Matthieu Napoli

@mvrhov thank you I'm not crazy!

Bart van den Burg

You can always do a ->getPrevious() call on the DBALException and read the code from that. However, codes are driver dependent, so unreliable IMO...

I'd like to see the exceptions completely refactored (http://www.doctrine-project.org/jira/browse/DBAL-407). That way, the driver can throw the appropriate exception which can be caught by classname. Checking the error code or the error message is then no longer necessary

Matthieu Napoli

@Burgov +1 that is exactly what I am trying to do, but I'm taking it the other way (i.e. changing DBAL so that it passes the error code, and then catching the exception and rethrowing a more specific one).

The way you suggest is way better, but it needs a lot more changes to DBAL. However it is way more logical, your example speaks for itself:

try {
    /* ... /*
} catch (NoSuchTableException $e) {
    // do something
} catch (DuplicateKeyException $e) {
    // do something else
}

Regarding the error code that changes according to the RDBMS, a layer of abstraction resolves the problem.

I've voted for your ticket and I am watching it, I hope it will lead to something interesting. Waiting for that, I think the current proposition can be a first step.

Benjamin Eberlei
Owner

I want to provide a much better Exception handling in the future, but until now why not use $ex->getPreviousException()->getcode()? This is not really needed.

Benjamin Eberlei beberlei closed this May 09, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
11  lib/Doctrine/DBAL/Connection.php
@@ -24,6 +24,7 @@
24 24
     Doctrine\DBAL\Driver\Connection as DriverConnection,
25 25
     Doctrine\Common\EventManager,
26 26
     Doctrine\DBAL\DBALException,
  27
+    Doctrine\DBAL\Driver\DriverException,
27 28
     Doctrine\DBAL\Cache\ResultCacheStatement,
28 29
     Doctrine\DBAL\Cache\QueryCacheProfile,
29 30
     Doctrine\DBAL\Cache\ArrayStatement,
@@ -597,7 +598,7 @@ public function prepare($statement)
597 598
         try {
598 599
             $stmt = new Statement($statement, $this);
599 600
         } catch (\Exception $ex) {
600  
-            throw DBALException::driverExceptionDuringQuery($ex, $statement);
  601
+            throw DriverException::driverExceptionDuringQuery($ex, $statement);
601 602
         }
602 603
 
603 604
         $stmt->setFetchMode($this->_defaultFetchMode);
@@ -646,7 +647,7 @@ public function executeQuery($query, array $params = array(), $types = array(),
646 647
                 $stmt = $this->_conn->query($query);
647 648
             }
648 649
         } catch (\Exception $ex) {
649  
-            throw DBALException::driverExceptionDuringQuery($ex, $query, $this->resolveParams($params, $types));
  650
+            throw DriverException::driverExceptionDuringQuery($ex, $query, $this->resolveParams($params, $types));
650 651
         }
651 652
 
652 653
         $stmt->setFetchMode($this->_defaultFetchMode);
@@ -741,7 +742,7 @@ public function query()
741 742
         try {
742 743
             $statement = call_user_func_array(array($this->_conn, 'query'), $args);
743 744
         } catch (\Exception $ex) {
744  
-            throw DBALException::driverExceptionDuringQuery($ex, func_get_arg(0));
  745
+            throw DriverException::driverExceptionDuringQuery($ex, func_get_arg(0));
745 746
         }
746 747
 
747 748
         $statement->setFetchMode($this->_defaultFetchMode);
@@ -790,7 +791,7 @@ public function executeUpdate($query, array $params = array(), array $types = ar
790 791
                 $result = $this->_conn->exec($query);
791 792
             }
792 793
         } catch (\Exception $ex) {
793  
-            throw DBALException::driverExceptionDuringQuery($ex, $query, $this->resolveParams($params, $types));
  794
+            throw DriverException::driverExceptionDuringQuery($ex, $query, $this->resolveParams($params, $types));
794 795
         }
795 796
 
796 797
         if ($logger) {
@@ -818,7 +819,7 @@ public function exec($statement)
818 819
         try {
819 820
             $result = $this->_conn->exec($statement);
820 821
         } catch (\Exception $ex) {
821  
-            throw DBALException::driverExceptionDuringQuery($ex, $statement);
  822
+            throw DriverException::driverExceptionDuringQuery($ex, $statement);
822 823
         }
823 824
 
824 825
         if ($logger) {
19  lib/Doctrine/DBAL/Driver/DriverException.php
... ...
@@ -0,0 +1,19 @@
  1
+<?php
  2
+
  3
+namespace Doctrine\DBAL\Driver;
  4
+
  5
+use Doctrine\DBAL\DBALException;
  6
+
  7
+class DriverException extends DBALException
  8
+{
  9
+    public static function driverExceptionDuringQuery(\Exception $driverEx, $sql, array $params = array())
  10
+    {
  11
+        $msg = "An exception occurred while executing '".$sql."'";
  12
+        if ($params) {
  13
+            $msg .= " with params ".json_encode($params);
  14
+        }
  15
+        $msg .= ":\n\n".$driverEx->getMessage();
  16
+
  17
+        return new self($msg, (int) $driverEx->getCode(), $driverEx);
  18
+    }
  19
+}
5  lib/Doctrine/DBAL/Statement.php
@@ -23,7 +23,8 @@
23 23
 
24 24
 use PDO,
25 25
     Doctrine\DBAL\Types\Type,
26  
-    Doctrine\DBAL\Driver\Statement as DriverStatement;
  26
+    Doctrine\DBAL\Driver\Statement as DriverStatement,
  27
+    Doctrine\DBAL\Driver\DriverException;
27 28
 
28 29
 /**
29 30
  * A thin wrapper around a Doctrine\DBAL\Driver\Statement that adds support
@@ -141,7 +142,7 @@ public function execute($params = null)
141 142
         try {
142 143
             $stmt = $this->stmt->execute($params);
143 144
         } catch (\Exception $ex) {
144  
-            throw DBALException::driverExceptionDuringQuery($ex, $this->sql, $this->conn->resolveParams($this->params, $this->types));
  145
+            throw DriverException::driverExceptionDuringQuery($ex, $this->sql, $this->conn->resolveParams($this->params, $this->types));
145 146
         }
146 147
 
147 148
         if ($logger) {
41  tests/Doctrine/Tests/DBAL/Functional/Ticket/DBAL371Test.php
... ...
@@ -0,0 +1,41 @@
  1
+<?php
  2
+
  3
+namespace Doctrine\Tests\DBAL\Functional\Ticket;
  4
+
  5
+use Doctrine\DBAL\DBALException;
  6
+use Doctrine\DBAL\Driver\DriverException;
  7
+
  8
+/**
  9
+ * @group DBAL-371
  10
+ */
  11
+class DBAL371Test extends \Doctrine\Tests\DbalFunctionalTestCase
  12
+{
  13
+    protected function setUp()
  14
+    {
  15
+        parent::setUp();
  16
+
  17
+        if ($this->_conn->getSchemaManager()->tablesExist('dbal371')) {
  18
+            $this->_conn->getSchemaManager()->dropTable('dbal371');
  19
+        }
  20
+        $table = new \Doctrine\DBAL\Schema\Table('dbal371');
  21
+        $table->addColumn('id', 'integer');
  22
+        $table->setPrimaryKey(array('id'));
  23
+
  24
+        $this->_conn->getSchemaManager()->createTable($table);
  25
+    }
  26
+
  27
+    public function testExceptionCode()
  28
+    {
  29
+        $stmt = $this->_conn->prepare('INSERT INTO dbal371 VALUES (1)');
  30
+        $stmt->execute();
  31
+        $exception = false;
  32
+        try {
  33
+            $stmt->execute();
  34
+        } catch(DBALException $e) {
  35
+            $exception = true;
  36
+            $this->assertInstanceOf('Doctrine\DBAL\Driver\DriverException', $e);
  37
+            $this->assertGreaterThan(0, $e->getCode());
  38
+        }
  39
+        $this->assertTrue($exception);
  40
+    }
  41
+}
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.