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

Add deprecation warning to show HttpKernel::handle() will catch throwables #45997

Open
wants to merge 1 commit into
base: 6.1
Choose a base branch
from

Conversation

Copy link
Member

@Nyholm Nyholm commented Apr 11, 2022

Q A
Branch? 6.1
Bug fix? no
New feature? no
Deprecations? yes
Tickets Fix #16205
License MIT
Doc PR symfony/symfony-docs#...

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:

$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)

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:

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

@stof
Copy link
Member

@stof stof commented Apr 11, 2022

A deprecation for which there is no way to opt-in for the new behavior is bad: it does not help migrating and will annoy people seeing it without being able to get rid of it.

@Nyholm
Copy link
Member Author

@Nyholm Nyholm commented Apr 11, 2022

Hm. You are very correct. Thank you. Do you have a suggestion on how to make an opt-in?

@stof
Copy link
Member

@stof stof commented Apr 11, 2022

Well, you would need some kind of configuration parameter that can turn on the catching of errors.

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.

3 participants