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

Extend RetryableException from Throwable interface #3991

Merged
merged 1 commit into from
May 9, 2020
Merged

Extend RetryableException from Throwable interface #3991

merged 1 commit into from
May 9, 2020

Conversation

mitelg
Copy link
Contributor

@mitelg mitelg commented May 5, 2020

Let RetryableException extend from the Throwable interface to prevent errors in Psalm if catching RetryableException

Q A
Type improvement
BC Break no, I don't think so
Fixed issues

Summary

We have some code which catches the RetryableException. Now Psalm complains about that catch because the RetryableException does not implement the Throwable interface

Class/interface Doctrine\DBAL\Exception\RetryableException cannot be caught (see https://psalm.dev/132)

This is my first PR here. I hope I have done everything right. Looking forward for feedback 😊 👋

@greg0ire
Copy link
Member

greg0ire commented May 8, 2020

Please do the same for lib/Doctrine/DBAL/Driver/ExceptionConverterDriver.php

@greg0ire greg0ire added the Bug label May 8, 2020
@greg0ire
Copy link
Member

greg0ire commented May 8, 2020

I think this can be considered as a bug, which makes this the correct branch to target. Can you also please annotate both interfaces with @psalm-immutable?

@mitelg
Copy link
Contributor Author

mitelg commented May 8, 2020

hey @greg0ire
thanks for your feedback. did as you requested and also rebased the branch

@mitelg
Copy link
Contributor Author

mitelg commented May 8, 2020

it seems that ExceptionConverterDriver is not meant to be a throwable interface for exceptions.
I will revert that. Error in PHPStan:

Child process error (exit code 255): PHP Fatal error: Class Doctrine\DBAL\Driver\AbstractOracleDriver cannot implement interface Throwable, extend Exception or Error instead in
/home/mt/www/dbal/lib/Doctrine/DBAL/Driver/AbstractOracleDriver.php on line 15
Fatal error: Class Doctrine\DBAL\Driver\AbstractOracleDriver cannot implement interface Throwable, extend Exception or Error instead in
/home/mt/www/dbal/lib/Doctrine/DBAL/Driver/AbstractOracleDriver.php on line 15

Let RetryableException extend from the Throwable interface to prevent errors in Psalm if catching it
@greg0ire
Copy link
Member

greg0ire commented May 8, 2020

Oh right, it has indeed nothing to do with exceptions, my bad. I'm closing and reopening to try to get the time sensitive test in Appveyor to pass.

@greg0ire greg0ire closed this May 8, 2020
@greg0ire greg0ire reopened this May 8, 2020
@SenseException
Copy link
Member

Exception interfaces have the suffix Exception and since DriverException already implements Throwable, RetryableException seems to be the last one.

@greg0ire greg0ire merged commit e3c9a8f into doctrine:2.10.x May 9, 2020
@greg0ire
Copy link
Member

greg0ire commented May 9, 2020

Thanks @mitelg !

@mitelg mitelg deleted the extend-retryable-exception branch May 11, 2020 07:44
@mitelg
Copy link
Contributor Author

mitelg commented May 11, 2020

nice! thank you guys for accepting 😊 👍

@greg0ire greg0ire added this to the 2.10.3 milestone Jun 7, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants