Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

Closed
wants to merge 10 commits into from

5 participants

@mnapoli

Forward the SQLSTATE error code to DBALException

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

@mnapoli mnapoli Issue DBAL-371
Forward the SQLSTATE error code to DBALException
8d707a5
@mnapoli

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

@mnapoli

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?

@beberlei
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.

@mnapoli

@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

@mnapoli

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

@Baachi Baachi commented on the diff
lib/Doctrine/DBAL/Driver/DriverException.php
@@ -0,0 +1,19 @@
+<?php
+
+namespace Doctrine\DBAL\Driver;
+
+use Doctrine\DBAL\DBALException;
+
+class DriverException extends DBALException
+{
+ public static function driverExceptionDuringQuery(\Exception $driverEx, $sql, array $params = array())
+ {
+ $msg = "An exception occurred while executing '".$sql."'";
+ if ($params) {
+ $msg .= " with params ".json_encode($params);
+ }
+ $msg .= ":\n\n".$driverEx->getMessage();
@Baachi
Baachi added a note

Newlines in an exception message? Really?

@mnapoli
mnapoli added a note

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
@mvrhov

@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
@mvrhov

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...

@mnapoli

@mvrhov thank you I'm not crazy!

@Burgov

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

@mvrhov mvrhov referenced this pull request in doctrine/doctrine2
Closed

Revived #265: [WIP] Mapping support for Embeddables (VOs). #547

@mnapoli

@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.

@beberlei
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.

@beberlei beberlei closed this
@mnapoli mnapoli deleted the branch
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.
View
11 lib/Doctrine/DBAL/Connection.php
@@ -24,6 +24,7 @@
Doctrine\DBAL\Driver\Connection as DriverConnection,
Doctrine\Common\EventManager,
Doctrine\DBAL\DBALException,
+ Doctrine\DBAL\Driver\DriverException,
Doctrine\DBAL\Cache\ResultCacheStatement,
Doctrine\DBAL\Cache\QueryCacheProfile,
Doctrine\DBAL\Cache\ArrayStatement,
@@ -597,7 +598,7 @@ public function prepare($statement)
try {
$stmt = new Statement($statement, $this);
} catch (\Exception $ex) {
- throw DBALException::driverExceptionDuringQuery($ex, $statement);
+ throw DriverException::driverExceptionDuringQuery($ex, $statement);
}
$stmt->setFetchMode($this->_defaultFetchMode);
@@ -646,7 +647,7 @@ public function executeQuery($query, array $params = array(), $types = array(),
$stmt = $this->_conn->query($query);
}
} catch (\Exception $ex) {
- throw DBALException::driverExceptionDuringQuery($ex, $query, $this->resolveParams($params, $types));
+ throw DriverException::driverExceptionDuringQuery($ex, $query, $this->resolveParams($params, $types));
}
$stmt->setFetchMode($this->_defaultFetchMode);
@@ -741,7 +742,7 @@ public function query()
try {
$statement = call_user_func_array(array($this->_conn, 'query'), $args);
} catch (\Exception $ex) {
- throw DBALException::driverExceptionDuringQuery($ex, func_get_arg(0));
+ throw DriverException::driverExceptionDuringQuery($ex, func_get_arg(0));
}
$statement->setFetchMode($this->_defaultFetchMode);
@@ -790,7 +791,7 @@ public function executeUpdate($query, array $params = array(), array $types = ar
$result = $this->_conn->exec($query);
}
} catch (\Exception $ex) {
- throw DBALException::driverExceptionDuringQuery($ex, $query, $this->resolveParams($params, $types));
+ throw DriverException::driverExceptionDuringQuery($ex, $query, $this->resolveParams($params, $types));
}
if ($logger) {
@@ -818,7 +819,7 @@ public function exec($statement)
try {
$result = $this->_conn->exec($statement);
} catch (\Exception $ex) {
- throw DBALException::driverExceptionDuringQuery($ex, $statement);
+ throw DriverException::driverExceptionDuringQuery($ex, $statement);
}
if ($logger) {
View
19 lib/Doctrine/DBAL/Driver/DriverException.php
@@ -0,0 +1,19 @@
+<?php
+
+namespace Doctrine\DBAL\Driver;
+
+use Doctrine\DBAL\DBALException;
+
+class DriverException extends DBALException
+{
+ public static function driverExceptionDuringQuery(\Exception $driverEx, $sql, array $params = array())
+ {
+ $msg = "An exception occurred while executing '".$sql."'";
+ if ($params) {
+ $msg .= " with params ".json_encode($params);
+ }
+ $msg .= ":\n\n".$driverEx->getMessage();
@Baachi
Baachi added a note

Newlines in an exception message? Really?

@mnapoli
mnapoli added a note

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
+
+ return new self($msg, (int) $driverEx->getCode(), $driverEx);
+ }
+}
View
5 lib/Doctrine/DBAL/Statement.php
@@ -23,7 +23,8 @@
use PDO,
Doctrine\DBAL\Types\Type,
- Doctrine\DBAL\Driver\Statement as DriverStatement;
+ Doctrine\DBAL\Driver\Statement as DriverStatement,
+ Doctrine\DBAL\Driver\DriverException;
/**
* A thin wrapper around a Doctrine\DBAL\Driver\Statement that adds support
@@ -141,7 +142,7 @@ public function execute($params = null)
try {
$stmt = $this->stmt->execute($params);
} catch (\Exception $ex) {
- throw DBALException::driverExceptionDuringQuery($ex, $this->sql, $this->conn->resolveParams($this->params, $this->types));
+ throw DriverException::driverExceptionDuringQuery($ex, $this->sql, $this->conn->resolveParams($this->params, $this->types));
}
if ($logger) {
View
41 tests/Doctrine/Tests/DBAL/Functional/Ticket/DBAL371Test.php
@@ -0,0 +1,41 @@
+<?php
+
+namespace Doctrine\Tests\DBAL\Functional\Ticket;
+
+use Doctrine\DBAL\DBALException;
+use Doctrine\DBAL\Driver\DriverException;
+
+/**
+ * @group DBAL-371
+ */
+class DBAL371Test extends \Doctrine\Tests\DbalFunctionalTestCase
+{
+ protected function setUp()
+ {
+ parent::setUp();
+
+ if ($this->_conn->getSchemaManager()->tablesExist('dbal371')) {
+ $this->_conn->getSchemaManager()->dropTable('dbal371');
+ }
+ $table = new \Doctrine\DBAL\Schema\Table('dbal371');
+ $table->addColumn('id', 'integer');
+ $table->setPrimaryKey(array('id'));
+
+ $this->_conn->getSchemaManager()->createTable($table);
+ }
+
+ public function testExceptionCode()
+ {
+ $stmt = $this->_conn->prepare('INSERT INTO dbal371 VALUES (1)');
+ $stmt->execute();
+ $exception = false;
+ try {
+ $stmt->execute();
+ } catch(DBALException $e) {
+ $exception = true;
+ $this->assertInstanceOf('Doctrine\DBAL\Driver\DriverException', $e);
+ $this->assertGreaterThan(0, $e->getCode());
+ }
+ $this->assertTrue($exception);
+ }
+}
Something went wrong with that request. Please try again.