-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpKernel] Inherit Throwable in HttpExceptionInterface #33809
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
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.
the PR should target 4.4, as a new "feature"
b2d201b
to
2ac3fbf
8000
Compare
Did it, and removed the upgrade line. However, I do not understand why this is considered a new feature instead of a BC break. 🤔 |
If it's considered as a BC break, then merging on master is equally wrong: we don't do hard BC breaks that don't trigger a prior notice in 4.4. Now we can discuss the BC break nature of this. This is a BC break only if some class implements |
I see your point, I don't think anyone did this too. |
Thank you @nesk. |
… (nesk) This PR was merged into the 4.4 branch. Discussion ---------- [HttpKernel] Inherit Throwable in HttpExceptionInterface | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | Deprecations? | no | **BC BREAKS?**| **YES** | Tickets | n/a | License | MIT | Doc PR | n/a When using a static analysis tools, it is not possible to do this: ```php if ($exception instanceof HttpExceptionInterface) { $exception->getStatusCode(); $exception->getHeaders(); $exception->getMessage(); // ❌ Will fail here } ``` This is due to `getMessage()` not being a method declared in `HttpExceptionInterface`. Since Symfony now requires PHP 7.1+ to run, it is safe to inherit from the `Throwable` interface (added in PHP 7.0). ### About backward compatibility Adding new methods to `HttpExceptionInterface` [breaks BC](https://symfony.com/doc/current/contributing/code/bc.html#changing-interfaces), however this interface shouldn't be used on a class other than an exception, so this shouldn't affect much code. ### About tests I'm not sure this really needs tests, but maybe I'm wrong? Tell me what to test if you think this is required. Commits ------- 2ac3fbf Inherit Throwable in HttpExceptionInterface
When using a static analysis tools, it is not possible to do this:
This is due to
getMessage()
not being a method declared inHttpExceptionInterface
. Since Symfony now requires PHP 7.1+ to run, it is safe to inherit from theThrowable
interface (added in PHP 7.0).About backward compatibility
Adding new methods to
HttpExceptionInterface
breaks BC, however this interface shouldn't be used on a class other than an exception, so this shouldn't affect much code.About tests
I'm not sure this really needs tests, but maybe I'm wrong? Tell me what to test if you think this is required.