-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Mark ExceptionInterfaces throwable #26702
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
Legit but breaks BC, not sure it's worth it ( |
Not sure how it breaks BC? I can update rest as well. |
It breaks for implementing classes that do not have the |
Ah ok. Let's see if others see a way to move this forward. I'm for doing this in next minor version. Chance for BC break isn't high, this interface is meant to be used for exception classes. |
If we decide to do it, we should do it on all components, not only on one. |
Well benefit is not just theoretical safety of catching. This thing is going to be picked up by static analysis tools each time someone catches such exception, so user will need to suppress this for each of such case - pretty annoying. Will change in all components if this change is approved. |
Why? It's perfectly fine to catch non-throwable. |
@@ -16,6 +16,6 @@ | |||
* | |||
* @author Bernhard Schussek <bschussek@gmail.com> | |||
*/ | |||
interface ExceptionInterface | |||
interface ExceptionInterface extends \Throwable |
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.
This interface, like the equivalent ones throughout the other components (about 20 of them), is meant to be a marker, nothing more.
So, I would not extend \Throwable
here. But if we decide to do so, would it make sense to do it on all our markers?
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.
This marker is for exceptions though, shouldn't it guarantee it's used on exception classes only?
How about deprecating this interface and replacing it another interface, extending Throwable?
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.
Deprecating implies to find a name for the replacement in addition to bother our users, I don't think it's worth the hassle.
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.
Regarding name, it could be {Component}ExceptionInterface. E.g. ConsoleExceptionInterface.
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.
I think it makes sense to extend Throwable everywhere to semantically indicate that it is for exceptions. No need for deprecations as this should not break anything.
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 at least somebody is with me, so I've applied this change in rest of the components as well.
Two questions:
|
I have thought about that same way as you do. Maybe it would make sense to mark these |
To answers the 2 questions:
|
It make sense to me too. |
I would agree on making this change in all components. |
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.
I don't see any huge reason to do this change, but let's do it so that we won't discuss this again in another PR in the future... :)
For the record, these interfaces did not extend anything as it was just a marker. All exception classes implementing this interface also extend a built-in PHP exception class ( |
Thank you @ostrolucky. |
This PR was merged into the 4.1-dev branch. Discussion ---------- Mark ExceptionInterfaces throwable | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This interface is meant to be catched, but currently it doesn't guarantee class extending it is an instance of Throwable or Exception. Commits ------- b2d8792 Mark ExceptionInterfaces throwable
…(ostrolucky)" (nicolas-grekas) This PR was merged into the 4.1 branch. Discussion ---------- Revert "feature #26702 Mark ExceptionInterfaces throwable (ostrolucky)" This reverts commit 9fda6d3, reversing changes made to ca53d3e. | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27419 | License | MIT | Doc PR | - #26702 introduced a BC break. Commits ------- 2ddb89b Revert "feature #26702 Mark ExceptionInterfaces throwable (ostrolucky)"
* 4.1: (22 commits) [HttpKernel] Fix restoring trusted proxies in tests Update UPGRADE-4.0.md [Messenger] Fix suggested enqueue adapter package bumped Symfony version to 4.1.1 updated VERSION for 4.1.0 updated CHANGELOG for 4.1.0 Insert correct parameter_bag service in AbstractController Revert "feature #26702 Mark ExceptionInterfaces throwable (ostrolucky)" CODEOWNERS: some more rules removed unneeded comments in tests removed unneeded comments in tests Change PHPDoc in ResponseHeaderBag::getCookies() to help IDEs [HttpKernel] fix registering IDE links update UPGRADE-4.1 for feature #26332 Form field help option [HttpKernel] Set first trusted proxy as REMOTE_ADDR in InlineFragmentRenderer. [Process] Consider \"executable\" suffixes first on Windows Triggering RememberMe's loginFail() when token cannot be created bumped Symfony version to 4.1.0 updated VERSION for 4.1.0-BETA3 updated CHANGELOG for 4.1.0-BETA3 ...
We'll be able to merge this patch again when phpspec/prophecy#412 will be merged and a new version of Prophecy will be tagged. |
This is fixed in Prophecy 1.8 I haven't assessed impact on other test double libraries |
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: symfony/symfony#26702 symfony/symfony#27420 symfony/symfony#27419 phpspec/prophecy#412 ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes Commits ------- 17c3675226 Mark ExceptionInterfaces throwable
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: symfony/symfony#26702 symfony/symfony#27420 symfony/symfony#27419 phpspec/prophecy#412 ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes Commits ------- 17c3675226 Mark ExceptionInterfaces throwable
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
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: symfony/symfony#26702 symfony/symfony#27420 symfony/symfony#27419 phpspec/prophecy#412 ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes Commits ------- 17c3675226 Mark ExceptionInterfaces throwable
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: symfony/symfony#26702 symfony/symfony#27420 symfony/symfony#27419 phpspec/prophecy#412 ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes Commits ------- 17c3675226 Mark ExceptionInterfaces throwable
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: symfony/symfony#26702 symfony/symfony#27420 symfony/symfony#27419 phpspec/prophecy#412 ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes Commits ------- 17c3675226 Mark ExceptionInterfaces throwable
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: symfony/symfony#26702 symfony/symfony#27420 symfony/symfony#27419 phpspec/prophecy#412 ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes Commits ------- 17c3675226 Mark ExceptionInterfaces throwable
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: symfony/symfony#26702 symfony/symfony#27420 symfony/symfony#27419 phpspec/prophecy#412 ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes Commits ------- 17c3675226 Mark ExceptionInterfaces throwable
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: symfony/symfony#26702 symfony/symfony#27420 symfony/symfony#27419 phpspec/prophecy#412 ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes Commits ------- 17c3675226 Mark ExceptionInterfaces throwable
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: symfony/symfony#26702 symfony/symfony#27420 symfony/symfony#27419 phpspec/prophecy#412 ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes Commits ------- 17c3675226 Mark ExceptionInterfaces throwable
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: symfony/symfony#26702 symfony/symfony#27420 symfony/symfony#27419 phpspec/prophecy#412 ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes Commits ------- 17c3675226 Mark ExceptionInterfaces throwable
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: symfony/symfony#26702 symfony/symfony#27420 symfony/symfony#27419 phpspec/prophecy#412 ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes Commits ------- 17c3675226 Mark ExceptionInterfaces throwable
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: symfony/symfony#26702 symfony/symfony#27420 symfony/symfony#27419 phpspec/prophecy#412 ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes Commits ------- 17c3675226 Mark ExceptionInterfaces throwable
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: symfony/symfony#26702 symfony/symfony#27420 symfony/symfony#27419 phpspec/prophecy#412 ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes Commits ------- 17c3675226 Mark ExceptionInterfaces throwable
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: symfony/symfony#26702 symfony/symfony#27420 symfony/symfony#27419 phpspec/prophecy#412 ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes Commits ------- 17c3675226 Mark ExceptionInterfaces throwable
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: symfony/symfony#26702 symfony/symfony#27420 symfony/symfony#27419 phpspec/prophecy#412 ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes Commits ------- 17c3675226 Mark ExceptionInterfaces throwable
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: symfony/symfony#26702 symfony/symfony#27420 symfony/symfony#27419 phpspec/prophecy#412 ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes Commits ------- 17c3675226 Mark ExceptionInterfaces throwable
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: symfony/symfony#26702 symfony/symfony#27420 symfony/symfony#27419 phpspec/prophecy#412 ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes Commits ------- 17c3675226 Mark ExceptionInterfaces throwable
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: symfony/symfony#26702 symfony/symfony#27420 symfony/symfony#27419 phpspec/prophecy#412 ping @dunglas @ciaranmcnulty @dkarlovi @Wirone @teohhanhui @stof @nicolas-grekas @ondrejmirtes Commits ------- 17c3675226 Mark ExceptionInterfaces throwable
This interface is meant to be catched, but currently it doesn't guarantee class extending it is an instance of Throwable or Exception.