DatabaseConnection->query() swallows PDOException->errorInfo [#458176]

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
#7
#4	errorInfo-2.patch	8.22 KB	c960657
#4
	errorInfo-1.patch	1.49 KB	c960657

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

c960657 CreditAttribution: c960657 commented 9 May 2009 at 13:40

Status:

Active

» Needs review

Comment #2

Dries CreditAttribution: Dries commented 9 May 2009 at 14:50

- Is there a reason we don't re-throw the existing exception?

- Should we write a test for this?

Comment #3

Crell CreditAttribution: Crell commented 11 May 2009 at 04:57

Status:

Needs review

» Needs work

We 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 commented 11 May 2009 at 18:27

File	Size
errorInfo-2.patch	8.22 KB

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