Skip to content

[HttpKernel] Skip logging uncaught exceptions in ErrorHandler, assume $kernel->terminateWithException() will do it #58289

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

Merged

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Sep 17, 2024

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #57902
License MIT

When uncaught exceptions are handled by ErrorHandler::handleException(), that method logs them, then calls a callback to let the app know about the exception itself.

This causes two issues:

  1. that log happens without context, which leads to excluded_http_codes not working as intended for http errors thrown inside the callback of a StreamedResponse #57902
  2. logs are duplicated: one by ErrorHandler, twice by the callback

In this PR, I propose to consider that if terminateWithException is configured as a callback, we assume that the app will handle the logging on its own.

Tested in practice, this works nicely.

Adding a test case is quite complex as all this involves many pieces and global state, so I skipped adding one.

… $kernel->terminateWithException() will do it
@carsonbot carsonbot added this to the 5.4 milestone Sep 17, 2024
@OskarStark OskarStark changed the title [HttpKernel] Skip logging uncaught exceptions in ErrorHandler, assume $kernel->terminateWithException() will do it [HttpKernel] Skip logging uncaught exceptions in ErrorHandler, assume $kernel->terminateWithException() will do it Sep 17, 2024
@nicolas-grekas nicolas-grekas merged commit ad3f44e into symfony:5.4 Sep 17, 2024
10 of 12 checks passed
@nicolas-grekas nicolas-grekas deleted the hk-log-uncaught-exceptions branch September 17, 2024 11:56
This was referenced Sep 21, 2024
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.

2 participants