So I've been working on making exceptions sent to the client match the format of the request (or, the route, if it really should only speak one format) over at #3304981: JSON and other serialization formats now handle non-4xx exceptions.
While testing this locally, I had the happy accident of writing some code which threw not an \Exception but a \TypeError, which is not an ancestor of \Exception but rather \Throwable. This resulted in a counterintuitive result where I got the ugly, last-ditch error formatter instead of my preferred formatted one.
A bit of tracing led me to discover that since Drupal 9 is still "stuck" on Symfony 4.4, the HttpKernel component only catches \Exception.
This was changed in April for Symfony 6, where the Kernel can now catch Throwables.
Interestingly, this functionality is not "on" by default, for BC reasons. (That is, the default is to only still catch \Exception.) It will, however, be the only behavior from Symfony 7.
I think Drupal should catch all the things and give them proper treatment in error handling. I've overridden the HttpKernel (I know...) on my project, but to opt-in to this behavior all we need to do is set a container parameter... and that means of course that end users can opt back out if a project needs time to prepare for Symfony 7.
This also requires changes to `InstallerRedirectTrait` and `ExceptionJsonSubscriber` which should should typehint on \Throwable. This is backwards-compatible of course because \Throwable includes \Exception.
I'm marking this major because I think it could get in "in time" for this to be a Change Record for 10.0.0? But since it's new behavior in Symfony 6, which Drupal 9 does not support, it does not need a deprecation in the same way as other code? I think?
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !2652
Comments
Comment #3
bradjones1Comment #4
andypostI think it needs @todo link and follow-up to remove new container param in 11.0.x
Comment #5
bradjones1Todo added and follow-up ticketed at #3305021: [Symfony 7] Drop unneeded argument to `HttpKernel`
Comment #6
bradjones1Added draft CR https://www.drupal.org/node/3305024
Comment #7
andypostBot should be green
Comment #8
andypostProper parent
Comment #9
catchTest failure is quickedit so unrelated.
I can't really think of a situation where someone would need to change this back, so wondering if we can just change the service definition without the new parameter (and hence no follow-up, or at least the follow-up would just remove passing the parameter). Someone could still alter the service definition itself to change the parameter if they really need to.
Comment #10
bradjones1I'm ambivalent, fine either way with it. I'm often told that X or Y change needs a deprecation, and this is a change in behavior... so I erred on the side of completeness. But it's a 1-line change without it. Just hard-code in
true. And as you mention, people can always alter the definition.Comment #11
catchI think we should just hard-code it, it makes it slightly harder to alter but not impossible - and as you say it's a behaviour change rather than an API change where we can't do a formal deprecation anyway.
Comment #12
bradjones1Done.
Comment #13
catchThat looks great thank you.
Comment #14
bradjones1Updated CR.
Comment #15
alexpottCommitted and pushed c86d7ad0c9 to 10.1.x and bc0acf7354 to 10.0.x. Thanks!
I really can't see why this is a major issue. Changed to a normal one.