Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
DatabaseConnection->query() catches a PDOException and throws a new one, but it does not copy $exception->errorInfo to the new exception.
This prevents modules from checking whether a given exception is a duplicate key exception or some other error. A well-behaved module should show an informative error message if a duplicate key exception occurs e.g. due to two users concurrently trying to insert into the same table, but it shouldn't try to handle e.g. connection errors.
Comment | File | Size | Author |
---|---|---|---|
#7 | errorInfo-3.patch | 8.21 KB | c960657 |
#4 | errorInfo-2.patch | 8.22 KB | c960657 |
errorInfo-1.patch | 1.49 KB | c960657 | |
Comments
Comment #1
c960657 CreditAttribution: c960657 commentedComment #2
Dries CreditAttribution: Dries commented- Is there a reason we don't re-throw the existing exception?
- Should we write a test for this?
Comment #3
Crell CreditAttribution: Crell commentedWe throw a new exception because for debugging purposes knowing the original query is critical. Otherwise tracking down the actual source of the error (especially very very early in the bootstrap process) is decidedly difficult. By making the exception text the query, arguments, and message, we get that printed trivially by default and can easily see what went wrong and where and how.
The current structure also made a great deal more sense before we had exception handling in core, and our testing framework was much less robust. :-) It's probably a good idea to revisit that design.
If we can safely take the original exception, add our query string to it, and rethrow that, that's probably the best of all options. Test-wise, I think a try-catch block around trying to insert a record with a primary key that is known to already exist would be the best approach, and fairly simple to do.
Comment #4
c960657 CreditAttribution: c960657 commentedIt is not possible to alter the message of an existing exception, but you can add other properties containing additional information and then rethrow the exception.
This patch lets the exception handler create an updated message using the query string and args (it already handles PDOExceptions specially). It isn't much cleaner - but perhaps a little bit :-) I also changed the exception handler test to verify that the actual SQL query (with placeholders) is displayed.
Comment #5
c960657 CreditAttribution: c960657 commentedComment #7
c960657 CreditAttribution: c960657 commentedReroll.
Comment #8
Dries CreditAttribution: Dries commentedI reviewed this patch and it looks good. Only one test seems to fail without this patch, although a couple of tests were modified.
Comment #9
c960657 CreditAttribution: c960657 commentedIn addition to adding a test for $e->errorInfo, I changed DrupalErrorHandlerUnitTest to test the changes to _drupal_decode_exception(), i.e. that the error handler displays the SQL string.
Comment #10
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!