Skip to content

Update HttpKernel.php #36885

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

Closed
wants to merge 1 commit into from
Closed

Update HttpKernel.php #36885

wants to merge 1 commit into from

Conversation

thiagocordeiro
Copy link
Contributor

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
License MIT

HTTP Kernel catches \Exception, but PHP 7.0 introduced \Throwable and a few errors are not handled by this try/catch.

Symfony registers a global handler \Symfony\Component\ErrorHandler\ErrorHandler and at some point \Symfony\Component\HttpKernel\Event\ExceptionEvent is triggered, so errors are handled twice

@@ -78,7 +78,7 @@ public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQ

try {
return $this->handleRaw($request, $type);
} catch (\Exception $e) {
} catch (\Throwable $e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe:

catch(\Throwable | \Exception) {

?

Copy link
Contributor Author

@thiagocordeiro thiagocordeiro May 21, 2020

Choose a reason for hiding this comment

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

Not actually @Guikingone, back in time PHP allowed us to catch \Exception only, from PHP7 on, errors are thrown and we can catch by \Error. Both implement \Throwable, which means any \Exception will be catch by \Throwable.

In other words, \Throwable is the same of \Exception | \Error.

@nicolas-grekas nicolas-grekas added this to the 4.4 milestone May 22, 2020
@nicolas-grekas
Copy link
Member

This is done on purpose: \Error are later caught by the PHP exception handler, and reinjected into the kernel using terminateWithException().

@thiagocordeiro
Copy link
Contributor Author

thiagocordeiro commented May 22, 2020

This is not needed when you have an exception event listener right? any exception will be re-thrown in src/Symfony/Component/HttpKernel/HttpKernel.php:214 then handled by the PHP exception handler if there is no response from the event listener

@derrabus
Copy link
Member

duplicate of #26514 😉

@fabpot
Copy link
Member

fabpot commented Jun 3, 2020

Closing for the same reasons as #26514

@fabpot fabpot closed this Jun 3, 2020
@thiagocordeiro thiagocordeiro deleted the patch-1 branch June 3, 2020 19:42
fabpot added a commit that referenced this pull request Jun 25, 2022
…to show `HttpKernel::handle()` will catch throwables (Nyholm)

This PR was merged into the 6.2 branch.

Discussion
----------

[FrameworkBundle][HttpKernel] Add deprecation warning to show `HttpKernel::handle()` will catch throwables

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | Fix #16205
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

I suggest that starting from Symfony 7.0 the `HttpKernel` will start to catch `\Throwable` and convert them to a `Response`.

This was first asked in #16205, I face a similar issue with Runtime component and Bref..

----------

The reason I push for this change is to embrace the request/response workflow of the Kernel without trusting the custom error handler. In an environment where you serve multiple requests with the same PHP process (read: RoadRunner, Swoole, Bref) you would write something like:

```php
$kernel = new Kernel('prod', false);
while (true) {
  $request = /* create sf request from custom environment */
  try {
    $response = $kernel->handle(request);
    return ResponseConverter::convert($response);
  } catch (\Throwable $e) {
    exit(1);
  }
}
```
(pseudo code of course. Here is a [real example](https://github.com/php-runtime/bref/blob/0.3.1/src/BrefRunner.php#L30-L43))

The `exit(1)` means a hard crash. For Bref Runtime it would result in a 500 error from API-gateway. Since the `\Throwable` is caught, the Symfony error handler is not used. If we would not to catch the `\Throwable`, then the Symfony error handler would be used, but it would print a Response instead of returning it. (Printing a response will just add HTML on the CLI...)

-----------

Other PRs and issues related to this:

- #45301
- #26514
- #22128
- #36885
- #25467

I'm happy to let the `HttpKernel` to catch the `\Throwable` exception right now, but I thought this very conservative PR would have a higher change to get merged.

Also note that we do not specify any behaviour on our [HttpKernelInterface](https://github.com/symfony/symfony/blob/v6.0.7/src/Symfony/Component/HttpKernel/HttpKernelInterface.php#L43)

-------

To remove the deprecation message you need to add this to your config:

```yaml
framework:
    catch_all_throwables: true
```

Commits
-------

7977a15 Add deprecation warning to show HttpKernel::handle() will catch throwables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants