DBAL-407: Refactor exceptions #1600

Closed
doctrinebot opened this Issue Jan 7, 2013 · 9 comments

2 participants

@doctrinebot

Jira issue originally created by user burgov:

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 jackalope/jackalope-doctrine-dbal#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?

@doctrinebot

Comment created by stof:

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.

@doctrinebot

Comment created by burgov:

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;
    }
}
@doctrinebot

Comment created by chrisguitarguy:

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?

@doctrinebot

Comment created by mnapoli:

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.

@doctrinebot

Comment created by @beberlei:

Steps:

  1. Introduce a list of constant status codes on \Doctrine\DBAL\DBALException for common errors: Duplicate Key, Integrity Constraint Violation (NOT NULL), table not found, ... (the three most common ones?)

  2. Add a test to Doctrine DBAL Functional Tests that provoke the exception for Sqlite/MySQL/,.. to find out what their error code and error message is.

  3. Write a simple helper function on every \Doctrine\DBAL\Driver and extend the interface, called "convertExceptionCode". Start with empty implementations return code 0 for every driver.

  4. \Doctrine\DBAL\DBALException::driverExceptionDuringQuery() should be extended to accept the driver and convert the exception to an error code that is not the currently used one 0.

  5. Introduce new exception types \Doctrine\DBAL\Exception\DuplicateKeyException, IntegrityConstraintViolationException, ...

@doctrinebot

Comment created by @beberlei:

This PR adds the last step #458 for the feature to be fully usable in DBAL 2.5

@doctrinebot

Comment created by @doctrinebot:

A related Github Pull-Request [GH-458] was closed:
#458

@doctrinebot

Comment created by @beberlei:

Added in DBAL-722 and DBAL-723

@doctrinebot

Issue was closed with resolution "Fixed"

@beberlei beberlei was assigned by doctrinebot Dec 6, 2015
@doctrinebot doctrinebot added this to the 2.5 milestone Dec 6, 2015
@doctrinebot doctrinebot closed this Dec 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment