Doctrine Project

  • Log In Access more options
    • Online Help
    • Keyboard Shortcuts
    • About JIRA
    • JIRA Credits
    • What’s New
  • Dashboards Access more options (Alt+d)
  • Projects Access more options (Alt+p)
  • Issues Access more options (Alt+i)
Doctrine DBAL
  • Doctrine DBAL
  • DBAL-407

Refactor exceptions

  • Log In
  • Views
    • XML
    • Word
    • Printable

Details

  • Type: Improvement Improvement
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: Drivers, Platforms, Schema Managers
  • Security Level: All
  • Labels:
    None

Description

It's currently rather hard to figure out what went wrong when for example a DBALException was thrown. You have to actually match the message in it, or read the status code of the ->getPrevious() exception, which can be different for all drivers (as https://github.com/jackalope/jackalope-doctrine-dbal/issues/80 shows).

I'd suggest creating new exception classes for all situations and throwing them instead. If they extend the DBAL Exception and pass the message to it as it is right now, there will be no BC break.

If this were to be done, on which branch should this be applied?

Activity

Descending order - Click to sort in ascending order
  • All
  • Comments
  • History
  • Activity
  • Source
Hide
Permalink
Matthieu Napoli added a comment - 06/May/13 6:09 AM - edited

I have implemented a thing of that kind in a personal project (on top of Doctrine). It is really useful to be able to catch a ForeignKeyViolationException, and get with entity/field caused the problem (for that my EntityManager wrapper parse the exception message).

However, note that exception codes differ from DB engines. In my case, I did it quick and used MySQL error codes, but managing different RDBMS implies more work.

Show
Matthieu Napoli added a comment - 06/May/13 6:09 AM - edited I have implemented a thing of that kind in a personal project (on top of Doctrine). It is really useful to be able to catch a ForeignKeyViolationException, and get with entity/field caused the problem (for that my EntityManager wrapper parse the exception message). However, note that exception codes differ from DB engines. In my case, I did it quick and used MySQL error codes, but managing different RDBMS implies more work.
Hide
Permalink
Christopher Davis added a comment - 06/May/13 3:29 AM - edited

I would also prefer named exceptions. You're going to have a lot of problems providing the "code" value in DBALException in any case: SQLSTATE codes are alphanumeric, and will cause warnings/errors when creating new exception.

Besides we can get the SQL state code now:

    try {
        // ...
    } catch (\Doctrine\DBAL\DBALException $e) {
        $code = $e->getPrevious()->getCode();
        // do stuff with $code
    }

The problem is that there are a lot of error codes defined in the ANSI SQL standard: http://www.postgresql.org/docs/9.2/static/errcodes-appendix.html

Maybe throwing an specific exception for each "class" of SQLSTATE codes? So if the error code from a PDO exception starts with 23, DBAL would throw `\Doctrine\DBAL\Exception\IntegrityConstraintViolationException`.

This also seems like the logic to handle throwing exceptions should be contained in the platforms as some implementations may differ. You could have a method in `AbstractPlatform` that takes care of the ANSI SQLSTATE error code classes and leave it up subclasses to deal with platform specific cases. Whenever `Connection` catches a `PDOException`, dispatch it to the platform to deal with.

Example: https://gist.github.com/chrisguitarguy/e021918900e93dca304d

Thoughts?

Show
Christopher Davis added a comment - 06/May/13 3:29 AM - edited I would also prefer named exceptions. You're going to have a lot of problems providing the "code" value in DBALException in any case: SQLSTATE codes are alphanumeric, and will cause warnings/errors when creating new exception. Besides we can get the SQL state code now: try { // ... } catch (\Doctrine\DBAL\DBALException $e) { $code = $e->getPrevious()->getCode(); // do stuff with $code } The problem is that there are a lot of error codes defined in the ANSI SQL standard: http://www.postgresql.org/docs/9.2/static/errcodes-appendix.html Maybe throwing an specific exception for each "class" of SQLSTATE codes? So if the error code from a PDO exception starts with 23, DBAL would throw `\Doctrine\DBAL\Exception\IntegrityConstraintViolationException`. This also seems like the logic to handle throwing exceptions should be contained in the platforms as some implementations may differ. You could have a method in `AbstractPlatform` that takes care of the ANSI SQLSTATE error code classes and leave it up subclasses to deal with platform specific cases. Whenever `Connection` catches a `PDOException`, dispatch it to the platform to deal with. Example: https://gist.github.com/chrisguitarguy/e021918900e93dca304d Thoughts?
Hide
Permalink
Bart van den Burg added a comment - 07/Jan/13 8:09 PM

I'd prefer actual named exceptions. It makes catching them simpler. However, adding some code defined in DBAL would be an acceptable alternative.

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

v.s.

try {
    /* ... /*
} catch (DBALException $e) {
    if ($e->getCode() == DBALException::NO_SUCH_TABLE) {
        // do something
    } elseif ($e->getCode() == DBALException::DUPLICATE_KEY) {
        // do something else
    } else {
        throw $e;
    }
}
Show
Bart van den Burg added a comment - 07/Jan/13 8:09 PM I'd prefer actual named exceptions. It makes catching them simpler. However, adding some code defined in DBAL would be an acceptable alternative. try { /* ... /* } catch (NoSuchTableException $e) { // do something } catch (DuplicateKeyException $e) { // do something else } v.s. try { /* ... /* } catch (DBALException $e) { if ($e->getCode() == DBALException::NO_SUCH_TABLE) { // do something } elseif ($e->getCode() == DBALException::DUPLICATE_KEY) { // do something else } else { throw $e; } }
Hide
Permalink
Christophe Coevoet added a comment - 07/Jan/13 7:32 PM

This should be done in the master branch.

Another solution, avoiding to create many classes, would be to use the exception code, which is always kept as 0 currently (the default value of the Exception class). You could have a code for each case (with constants in the DBALException class) and then checking $e->getCode() to identify what went wrong.

Show
Christophe Coevoet added a comment - 07/Jan/13 7:32 PM This should be done in the master branch. Another solution, avoiding to create many classes, would be to use the exception code, which is always kept as 0 currently (the default value of the Exception class). You could have a code for each case (with constants in the DBALException class) and then checking $e->getCode() to identify what went wrong.

People

  • Assignee:
    Benjamin Eberlei
    Reporter:
    Bart van den Burg
Vote (1)
Watch (4)

Dates

  • Created:
    07/Jan/13 6:09 PM
    Updated:
    06/May/13 6:10 AM
  • Atlassian JIRA (v5.2.7#850-sha1:b2af0c8)
  • Report a problem
  • Powered by a free Atlassian JIRA open source license for Doctrine Project. Try JIRA - bug tracking software for your team.