Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Making the DBALException implement Throwable #2658

Merged
merged 1 commit into from
Jul 22, 2017

Conversation

svycka
Copy link
Contributor

@svycka svycka commented Feb 21, 2017

Doctrine\DBAL\DBALException does not implement \Exception

@@ -45,13 +45,7 @@ class DriverException extends DBALException
*/
public function __construct($message, \Doctrine\DBAL\Driver\DriverException $driverException)
{
$exception = null;

if ($driverException instanceof \Exception) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes perfect sense because we are type hinting against an interface in the constructor and need to make sure that we really pass an exception to the parent constructor (IIRC).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deeky666 we should probably make DriverException inherit from Throwable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable, yes. I guess it was just a sanity check here for PHP < 7

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@svycka can you add the extends on DriverException then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I read it wrong :D

Copy link
Contributor Author

@svycka svycka Feb 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ocramius but a bit don't udnerstand if I add interface DriverException extends \Throwable It would still be not possible to pass parent::__construct($message, 0, $driverException); because cosntructor expects \Exception not \Throwable

@svycka
Copy link
Contributor Author

svycka commented Feb 21, 2017

ok it looks tat php does not allow implement \Throwable so closing

@svycka svycka closed this Feb 21, 2017
@Ocramius
Copy link
Member

@svycka patch looked correct: you can only implement Throwable by extending an Exception though. Our codebase does that, but mocking doesn't. We'd probably drop tests mocking this interface.

@Ocramius Ocramius self-assigned this Feb 21, 2017
@svycka svycka reopened this Feb 21, 2017
@photodude
Copy link
Contributor

FYI you should rebase so travis will correctly test your PR

@Ocramius
Copy link
Member

This change goes in as-is: patch is correct and we now enforce PHP 7.1 anyway. 👍

@Ocramius Ocramius merged commit 0a17b49 into doctrine:master Jul 22, 2017
@Ocramius Ocramius changed the title this check does not make sense Making the DBALException implement Throwable Jul 22, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants