ErrorHandler interface limitations.

[luzrain]

ErrorHandler is pretty useless now, it does not give me the ability to implement my own Error handler.
By Error handler I mean my own logic that takes an exception and request as input arguments.

I would like to complete replace all of this default logic that hardcoded in AbstractHttpDriver now:

http-server/src/Driver/Internal/AbstractHttpDriver.php

Line 90 in 26001b1

private function handleInternalServerError(Request $request, \Throwable $exception): Response

with my own implementation and log exceptions by myself as I need, but now I can't.
I can't event intercept an exception to get access to the stack trace. Current ErrorHandler only gives access to exception code. Having only code in Error handler is absolutely not enough.

Here is an example how from my point of view ErrorHandler interface should look like.

interface ErrorHandler
{
    public function handleError(\Throwable $throwable, ?Request $request = null): Response;
}

Then the default ErrorHandler implementation could do the default logging that is now hardcoded in AbstractHttpDriver and generate Response for the browser, and I could completely replace the implementation having access to the exception itself as I need.

Are you interested in such changes? If so, I could work on it. But of course it will not be backwards compatible.
What do you think about it?

[kelunik]

Sounds good to me. We should probably name the new interface ExceptionHandler, so we can avoid the naming conflict. If an ErrorHandler is passed, we can likely automatically convert it into an ExceptionHandler and add the logging logic in there.

[trowski]

ErrorHandler was not designed for exception handling. It was designed to provide a response to bad or malformed requests, which is why it does not require an exception to be passed.

AbstractHttpDriver::handleInternalServerError() also was not meant to be the general purpose exception logger for an application. Rather it should only be hit during development or as the last possible point to catch an unexpected exception from the application rather than crashing the entire server. If we allow overriding the code handling these exceptions, we'll need the existing exception handler to catch exceptions from that user-defined handler.

The request handler provided to the HTTP server should be catching all exceptions and logging them as you desire. I don't see a reason to provide this functionality at a lower level, separate from the request handler.

[dam2k]

It should also be noted that Amp\Http\HttpStatus\DefaultErrorHandler make use of a blocking function ( file_get_contents() ) to read on resources/error.html

[trowski]

It should also be noted that Amp\Http\HttpStatus\DefaultErrorHandler make use of a blocking function ( file_get_contents() ) to read on resources/error.html

It is using a blocking function, but the file loaded is part of the source code and read only a single time, so is not much different than loading a source file via the autoloader.

[trowski]

Thinking further about this issue, perhaps we should provide a simple middleware which takes an ExceptionHandler interface such as the one @luzrain proposed and document how to use it. It would be a very simple middleware, but would help users understand how to handle uncaught exceptions from request handlers. We could add a parameter to the static builders on SocketHttpServer which accepted a nullable ExceptionHandler, attaching the necessary middleware automatically.

[luzrain]

Thinking further about this issue, perhaps we should provide a simple middleware which takes an ExceptionHandler interface such as the one @luzrain proposed and document how to use it. It would be a very simple middleware, but would help users understand how to handle uncaught exceptions from request handlers. We could add a parameter to the static builders on SocketHttpServer which accepted a nullable ExceptionHandler, attaching the necessary middleware automatically.

Keep in mind that if I want to completely redesign all kinds of error pages, implementing ExceptionHandler as middleware would not be enough. I would have to implement both ExceptionHandler and ErrorHandler, because in case when malformed request in occurs, middlewares are not called, request object is not even created. In this case ErrorHandler will create error page with 40x code.

[trowski]

I would have to implement both ExceptionHandler and ErrorHandler

This is true. I've opened a PR which demonstrates how an ExceptionHandler might delegate to an ErrorHandler for generation of the response, while providing logging of the exception details. See DefaultExceptionHandler. A single implementation of ErrorHandler then would be able to provide responses to both malformed requests and internal server errors (uncaught exceptions).