Skip to content

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

Merged
nicolas-grekas merged 1 commit intosymfony:5.4from
nicolas-grekas:hk-log-uncaught-exceptions
Sep 17, 2024
Merged

[HttpKernel] Skip logging uncaught exceptions in ErrorHandler, assume $kernel->terminateWithException() will do it#58289
nicolas-grekas merged 1 commit intosymfony:5.4from
nicolas-grekas:hk-log-uncaught-exceptions

Conversation

@nicolas-grekas
Copy link
Copy Markdown
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
@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