Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.5
    • 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

        Bart van den Burg created issue -
        Hide
        Christophe Coevoet added a comment -

        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 - 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.
        Hide
        Bart van den Burg added a comment -

        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 - 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
        Christopher Davis added a comment - - 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 - - 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
        Matthieu Napoli added a comment - - 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 - - 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
        Benjamin Eberlei added a comment -

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

        Show
        Benjamin Eberlei added a comment - 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, ...
        Hide
        Benjamin Eberlei added a comment -

        This PR adds the last step https://github.com/doctrine/dbal/pull/458 for the feature to be fully usable in DBAL 2.5

        Show
        Benjamin Eberlei added a comment - This PR adds the last step https://github.com/doctrine/dbal/pull/458 for the feature to be fully usable in DBAL 2.5
        Hide
        Doctrine Bot added a comment -

        A related Github Pull-Request [GH-458] was closed:
        https://github.com/doctrine/dbal/pull/458

        Show
        Doctrine Bot added a comment - A related Github Pull-Request [GH-458] was closed: https://github.com/doctrine/dbal/pull/458
        Hide
        Benjamin Eberlei added a comment -

        Added in DBAL-722 and DBAL-723

        Show
        Benjamin Eberlei added a comment - Added in DBAL-722 and DBAL-723
        Benjamin Eberlei made changes -
        Field Original Value New Value
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 2.5 [ 10523 ]
        Resolution Fixed [ 1 ]

        This list may be incomplete, as errors occurred whilst retrieving source from linked applications:

        • Request to http://www.doctrine-project.org/fisheye/ failed: Error in remote call to 'FishEye 0 (http://www.doctrine-project.org/fisheye/)' (http://www.doctrine-project.org/fisheye) [AbstractRestCommand{path='/rest-service-fe/search-v1/crossRepositoryQuery', params={query=DBAL-407, expand=changesets[0:20].revisions[0:29],reviews}, methodType=GET}] : Received status code 503 (Service Temporarily Unavailable)

          People

          • Assignee:
            Benjamin Eberlei
            Reporter:
            Bart van den Burg
          • Votes:
            3 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: