When writing code to catch DatabaseException, intellisense doesn't see the existence of any exception-related methods.
Write something like this:
namespace Drupal\Core\Database;
try {
throw new DatabaseExceptionWrapper('Error message.');
}
catch (DatabaseException $e) {
\Drupal::logger()->error($e->getMessage());
}
Then, since DatabaseException is just an empty interface and isn't guaranteed to have a getMessage() function, an IDE with intellisense will complain.
A workaround can be to either put /** @var \Throwable $e */ inside the catch, or to catch every specific class that implements the interface.
interface DatabaseException extends \Throwable { }
This still works with all the other existing code, and it makes it obvious to development tools that what you're catching is something that is a \Throwable.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 3340077-4.patch | 501 bytes | ankitsingh0188 |
Comments
Comment #2
solideogloria commentedComment #3
solideogloria commentedComment #4
ankitsingh0188I've created the patch with the new CR.
Comment #5
mondrakeMaybe \Throwable is too high in the inheritance tree? In the end aren't we facing a kind of \RuntimeException when the database fails?
Comment #6
ankitsingh0188What if we get the \DatabaseException?
In that case
$e->getMessage()will not work as mentioned in Steps to reproduce section.I think the exception interface should extend the \Throwable with respect to what you're catching is something that is a \Throwable.
Please clarify why there's need of \DatabaseException? If we will handle everything in \RuntimeException?
And even \RuntimeException implements the \Throwable interface.
class RuntimeException extends Exception implements Throwable, StringableComment #7
solideogloria commentedNot every implementer of
DatabaseExceptionextends\RuntimeException, though. See\InvalidQueryException.It could be
\Exceptioninstead of\Throwable, but no lower in the tree. Except that\Exceptionis a class, not an interface. So if you do that, then you have to change every single exception class to extend rather than implementDatabaseException, which might break third-party exception classes that implementDatabaseException.Comment #8
solideogloria commentedThe proposed patch is guaranteed to work with existing code, and I think it's the best solution.
Edit: The change record draft also looks good to me.
Comment #10
catchOverall I feel like this shouldn't be necessary, however to make it unnecessary I think we'd have to change the database API's InvalidArgumentExceptions to assertions. Also discussed briefly with @longwave who pointed out that Guzzle has the same pattern.
So.. Committed 87d90f9 and pushed to 10.1.x. Thanks!
Comment #12
catchComment #14
quietone commentedPublished the CR.