I'm mostly 👎 on this change: the debug component is already a dependency of http-kernel, so you just need to use it. And using the debug component handles real fatal errors, which do still exist. This change could give the illusion that nothing else is needed. That's not true, and that's why the debug component exists.

I'm fine with the debug component being a dependency of the kernel. In fact, this PR makes use of the component as well. Also, I don't say that a proper error handler is not needed. I just don't see why I'm required to use the error handler from the debug component.

Take the script from the PR description: Why am I required to register a specific global error handler to make this script work?

Why am I required to register a specific global error handler to make this script work?

because you want to catch fatal errors, and this is the only way to do it?

because you want to catch fatal errors, and this is the only way to do it

I could use any other error handler for fatal errors. That's not the point. The script does not raise a fatal error. It raises a simpleDivisionByZeroError that is catchable in php 7.

Also, if I changed the controller above to raise an exception instead, that exception would be nicely handled. But an error falls through. That feels inconsistent.

Exception andError are very different kind of throwables, that's why PHP core made them separate type hierarchies. The current behavior is inconsistent with PHP IMHO.DivisionByZeroError is the only case where this might be debatable. If you consider all the other*Error, you really don't want to recover from them, but fix you code instead (same forDivisionByZeroError btw...). At least that's the current reasoning.

Exception andError are very different kind of throwables

The main difference is that exceptions are usually thrown by userland code and errors are raised by php core and extensions.

you really don't want to recover from them, but fix you code instead

You're absolutely right. Same goes for many exceptions btw, likeInvalidArgumentException orBadMethodCallException.

What I expect an application framework to do in this case is to fail nicely, provide me with helpful information and (in case it happens on production) give me the chance to log that error somehow. That is to my understanding the purpose of the global try/catch block in HttpKernel. And that block is – imho – incomplete if it ignores errors.

Yep, same expectations, including for fatal errors on my side. This is already provided by Debug, and thinking catching solves the problem is an illusion :)

same expectations, including for fatal errors on my side.

As you said, that's what we need an error handler for. It was not my intention to solve fatals.

This is already provided by Debug, and thinking catching solves the problem is an illusion :)

So with that argument, we could remove the entire try/catch block from HttpKernel. In fact, the debug component really handles it nicely, if I actually do that. 😮

Yet, that block is there and it serves a purpose. I'm only updating it for the php 7 era.

From Slack:

derrabus [3:55 PM]
Hey. I feel like we’re spinning circles with the discussion on that PR. I’m really surprised you’re opposing the PR that heavily. If you have some time, I’d like to discuss it with you.

nicolasgrekas [3:57 PM]
Hi 🙂
I feel like blurring the lines between Exception and Error type hierarchies is not a good idea
I prefer clear separate handling paths
an unhandled Error in not different from an unhandled fatal error IMHO
but your pushing to make it the other way around: split fatal errors from Error, and make Error like Exception

derrabus [3:59 PM]
Okay, so we mainly have a different understanding of theError type.

nicolasgrekas [3:59 PM]
if I reason asymptotically, all fatal errors will be converted to Error by php core
yes 🙂

derrabus [4:01 PM]
So, php documents the class as follows: “Error is the base class for all internal PHP errors.”
It does not say anything about the severity of the error, it simply says: this is an error that has been raised by the php core and not by userland code.

nicolasgrekas [4:02 PM]
the severity is always high, because it's always a programming mistake

derrabus [4:03 PM]
I agree that fatals have to be handled differently. That’s why you cannot catch them.

nicolasgrekas [4:03 PM]
not something anybody shuld silently catch and discard

derrabus [4:03 PM]
I agree, but that’s not what we’re doing, right?

nicolasgrekas [4:03 PM]
actually that's not my understanding of fatal errors: they are just situations that are too hard for php code to turn into Error, due to internal details
(until now)
I really wish nobody catches Error (thus Throwable), and deal with them indifferentialy (unless rethrown) (edited)
I do also understand your POV of course.

derrabus [4:06 PM]
Okay, but what exactly are we doing differently then? Right now, the error is converted into aFatalThrowableError exception and pushed through the application’skernel.exception event, right?

nicolasgrekas [4:06 PM]
via terminateWithException yes
another global exception handler could see them (edited)

derrabus [4:08 PM]
So, the only difference with my PR would be that the kernel itself would trigger that event and not the error handler?

nicolasgrekas [4:08 PM]
if you look at handleException, it does some things
like setting exitCode to 255, converting the Error to Exception (thus no need for FlattenedThrowable), and logs early on (edited)

derrabus [4:17 PM]
Does the exit code have any effect in a web request? Exceptions are logged, so we would log errors twice right now.

nicolasgrekas [4:18 PM]
that's some sort of failsafe I think
if the complex logic doesn't work, at least the lowlevel one in handleException might have

derrabus [4:20 PM]
Also, I don’t want to remove the error handler. We still need it for fatals and everything that could go wrong before or after request processing.

nicolasgrekas [4:20 PM]
ok, so what's the use case ?

derrabus [4:23 PM]
Okay, mainly for teaching purposes, I was trying to use some Symfony components outside of the context of a full-stack application, to show how the components play together. I found the fact that HttpKernel enables me to fail nicely on exceptions but not on erros really confusing.

nicolasgrekas [4:24 PM]
OK, so not something that justifies the change IMHO
"just" teach the next level: fatal error handling 🙂

derrabus [4:25 PM]
Yeah, probably. Unless somebody would try to build the next Silex or something, the current way does not hurt anybody.

nicolasgrekas [4:26 PM]
yep, and they would still need Debug to deal with fatal errors 🙂

derrabus [4:28 PM]
Yes. But with my change applied, they could use any other error handler. The odd thing about the current solution is, that only the error handler from Debug works properly on errors because it does that odd pusing the error through the event dispatcher thingy.

nicolasgrekas [4:29 PM]
that's a feature of the Debug error handler 🙂
any other can still handle Error and Fatal Error the way they want

derrabus [4:32 PM]
Yes, but theyhave to handle errors because the kernel does not catch them despite$catch = true. They could solve that by extending theHttpKernel class, yet it feels unnecessarily complicated. (edited)

nicolasgrekas [4:32 PM]
but they'd also have to deal with fatal errors if they want to build a real framework
so back to the current situtation

derrabus [4:34 PM]
Again, that’s “we need to use any proper error handler” Vs. “we need to use the error handler fromDebug“.

nicolasgrekas [4:35 PM]
why?
why any other one couldn't do it ?
my pov is that fatals and Error should reunited (edited)
vs Exception + Error
so any alternative error handler would just deal with fatals and uncaught errors (edited)

derrabus [4:39 PM]
Right, from that PoV the current way is understandable. And since I probably won’t convince you from my PoV that errors are just exceptions thrown by the php interpreter, my PR probably does not have a chance to go through. 😕

nicolasgrekas [4:40 PM]
sorry about that 🙂 error handling is a sensitive and complex topic
once it's stable, I'm very reluctant to change it
the current way proved stable, and removed many if not all blank pages
I'm conservative here, I know 🙂 that doesn't make you wrong... (edited)
(but my way it more "proved")

derrabus [4:43 PM]
Right. I don’t think that my change would raise the danger of blank pages though, since Debug’s error handler would still serve as a safety net as it does now.

nicolasgrekas [4:43 PM]
but it might raise the danger of not logged messages 🙂

derrabus [4:43 PM]
But okay, now I know your reasoning behind it. Thanks for taking the time. 😃

nicolasgrekas [4:44 PM]
thanks to you 🙂
I'm happy to share these, now I'm not alone knowing why this current design makes sense
hopefully 🙂

derrabus [4:47 PM]
I will stop tinkering with the error handling then. If there’s anything else I could help with before the feature freeze, let me know. 😉