Add support for an optional onError callback

[filisko]

Hi,

This adds support for an optional PSR-3 compliant logger.

The logger is passed as an optional parameter to the constructor. And it also includes a logCallback option to fully customize how logging happens (see the changelog and readme).

Thanks!

[filisko]

@oscarotero this is done, approve and merge it when you can ;)

[oscarotero]

I think the logger shouldn't be passed in the constructor because it's an optional feature. It should be a method (for example, $middleware->logger(...)).

[shadowhand]

Dependencies belong in constructors... how else can we have dependency inversion?

[oscarotero]

Yes, I agree. But in this case it's an optional feature. People may want to log the errors or not, so I see this as a configuration, not a core dependency of the middleware.

That being said, maybe you're right in your comment about the premise of this feature and it would be better to simply create a specific middleware for this.

@filisko ?

[shadowhand]

A constructor parameter with a default value of null has no cost. (With PHP named parameters, it doesn't even cost writing null to fill it in.) A method that has to accept a parameter to update the object means the object is no longer readonly. Thus, a method for an optional is far more expensive to create, maintain, and operate than a constructor parameter.

[oscarotero]

I think logger() could accept a callable or a LoggerInterface, so we don't need two methods for the same purpose.
Other option is ->logger($logger, $callback), but to me is more elegant to have only one handler for logging (the callback can have access to the logger by itself).

[filisko]

ok, but then it wouldn't be about logging anymore, but a hook for when an error happens, right? Because inside the callback you can put anything, there is no LoggerInterface enforcement anymore so we should change the name.

How does this look like? onError

$errorHandler->onError(function( Throwable $error, ServerRequestInterface $request) use($logger, $other): void {
          $logger->error('Whoops!');
         // $other stuff...
});

[oscarotero]

It looks good to me. Maybe an onError callback is enough to cover all casuistry and we don't need a specific method for logger.

[oscarotero]

If the logger is callback, I'd do $this->logger($error, $request). The callback is a callable with access to a logger (or anything else).

[oscarotero]

Probably I'd include some info about the request: url, method, and uuid (https://github.com/middlewares/uuid).

[filisko]

yes, funny that I added it and then forgot to put it back

[shadowhand]

I am actually going to reject the premise of this PR entirely and say that:

Logging exceptions and formatting exceptions are two entirely processes with entirely different goals. This entire PR has become a complicated way to avoid writing a custom middleware:

    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
    {
        try {
            return $handler->handle($request);
        } catch (Throwable $exception) {
            $this->logger->critical('Uncaught {error}', [
                'error' => $exception->getMessage(),
                'exception' => $exception, // If you use Monolog, this is correct
            ]);

            throw $exception;
        }
    }

[filisko]

@oscarotero I like what @shadowhand said, thanks (will add you more frequent here 😄 ) !

I'll just leave the .gitattributes change.