-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Mark ExceptionInterfaces throwable #2 #28307
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
Conversation
Can we verify the same usecase is OK with PHPUnit? |
Seems support has been added in sebastianbergmann/phpunit-mock-objects#402 |
This has been reverted because it breaks BC as demonstrated by Prophecy and PHPUnit. There could be other packages/projects out there with the same issues. |
I also think this provides little to no benefit and the BC risk is proven already. |
It provides benefits if you're using a static code analysis tool which notes you're trying to catch non-throwable exceptions. I'd argue that the BC issue highlighted deeper issues in Prophecy and PHPUnit, not that it was Symfony's fault. |
What is a non-throwable exception? Any is throwable, the interface does not add any more "power" to the exception class... or I'm missing something. |
If you're using the exception marker interface as intended (to catch any package's exception), you're doing something like: try {
} catch (ExceptionInterface $exception) {
} This code is marked invalid by static code analysis as the interface is not throwable, as in the fact it can be thrown at all is circumstantial. Relying on the concrete implementation to make the interface throwable doesn't rely on the interface but the concrete implementation, making the marker interface pretty much useless. Real example: https://github.com/dkarlovi/xezilaires/blob/60ca197333b0974d543c2e4a88f94e7d1878d3d7/src/Xezilaires/Metadata/Mapping.php#L61 |
I don't think it's exactly a BC break, it just showed there's a bug in Prophecy that didn't allow mocking Throwable interfaces... |
Sorry, but unfortunately I'm too dumb to understand the issue. I will let the other @symfony/deciders team members decide. |
@ostrolucky I'm also having problems trying to understand the necessity of this change. Could you please explain very briefly (or with a small code example) what does this change allow to do that wasn't possible before? Thanks! |
Basically this: https://phpstan.org/r/d90d2b30f1bd1eeb90255e92927e4fdf You're putting something that might not be an exception into places like |
So if you intend some interfaces only to be used by exceptions, you can enforce that by extending the interface from |
As opposed to https://phpstan.org/r/e4e2c496c15be8d8ec2fcc55e7b29051 |
Same issue with Psalm: |
The benefit of this PR is indeed for any tool doing static analysis. So this concerns phpstan, psalm, but also IDEs (PHPStorm does not currently has such inspection in its stable version to highlight a mistake, but autocompletion in It won't make a huge difference for developers themselves (if we forget the impact on their tooling), as the only change this makes for PHP is forbidding silly mistakes (implementing something called |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, let's do this. I don't agree with the "error" level: a "warning" would be more appropriate to me unless of course one explicitly enabled a rule to make this an error. But that's not a reason against this PR :)
About the BC break, it's true the issues were bugs actually, highlighting the fact that Throwable
is very special since the only way to implement it is by extending Exception
or Error
.
Should we add a line in UPGRADING-4.2/5.0.md?
Ok, understood now. Thanks for the additional info. |
I'd argue the "impact on their tooling" is the crucial point here: since the tooling is becoming as prevalent in a modern PHP toolbox. Looking at the pace and value the tools generate, this trend will only increase and changes like these will be just as important as any other bugfix / feature in the future. |
Thank you @ostrolucky. |
This PR was merged into the 4.2-dev branch. Discussion ---------- Mark ExceptionInterfaces throwable #2 | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This has been reverted in beta of 4.1 because of lack of support in prophecy, which has been fixed since then (incl. release). Can be merged again. References: #26702 #27420 #27419 phpspec/prophecy#412 ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes Commits ------- 17c3675 Mark ExceptionInterfaces throwable
I totally agree on that. but I wanted to point out the other impact, as I already detailed the tooling impact previously in my comment |
…native Throwable interface (xabbuh) This PR was merged into the 7.2 branch. Discussion ---------- [Messenger] Let WrappedExceptionsInterface extend the native Throwable interface | Q | A | ------------- | --- | Branch? | 7.2 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | Fix #57667 | License | MIT We already did that in the past for `ExceptionInterface` in #28307. Commits ------- 0193dcd let WrappedExceptionsInterface extend the native Throwable interface
This has been reverted in beta of 4.1 because of lack of support in prophecy, which has been fixed since then (incl. release). Can be merged again.
References:
#26702
#27420
#27419
phpspec/prophecy#412
ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes