Improve error logging by providing exception in context

[pdehne]

When logging errors in Slim\Handlers\ErrorHandler logError the logger object is declared to implement the LoggerInterface and the error function of this interface is used to actually log the error:

    protected function logError(string $error): void
    {
        $this->logger->error($error);
    }

The LoggerInterface error method has an optional second parameter named context that can be an array of anything. Supplying the exception in this array would improve error logging for some LoggerInterface implementations, e.g. with Monolog you could decide to log different kinds of exceptions differently, without the exception in the context this is not possible.

The only change needed would be:

    protected function logError(string $error): void
    {
        $this->logger->error($error, [ "exception" => $this->exception ]);
    }

Would it make sense to add this?

[t0mmy742]

From PSR-3 specification:

If an Exception object is passed in the context data, it MUST be in the 'exception' key. Logging exceptions is a common pattern and this allows implementors to extract a stack trace from the exception when the log backend supports it. Implementors MUST still verify that the 'exception' key is actually an Exception before using it as such, as it MAY contain anything.

It seems like it is perfectly legal to do that. Maybe we could add another parameter to Slim\Handlers\ErrorHandler to send or not the exception.

[solventt]

@pdehne I think what you suggested makes no sense. You can already control the exception details logging through the $logErrorDetails option (when using addErrorMiddleware()). The writeToErrorLog() method of Slim's ErrorHandler has:

$renderer = $this->callableResolver->resolve($this->logErrorRenderer);
$error = $renderer($this->exception, $this->logErrorDetails);

By default $this->logErrorRenderer = PlainTextErrorRenderer::class. And If we look at the PlainTextErrorRenderer class, we see, that exception details are extracted from the exception object if the $logErrorDetails option was provided. So when $this->logger->error($error) is invoked, the $error variable may contains the context you mentioned.

If you want to set up the log display in some other way - you are welcome to Slim's docs - It explains how you can define you custom error handler. Also you can use setLogErrorRenderer() to set you custom log renderer.

@l0gicgate What do you think?

[pdehne]

Please let me explain my use case a bit more. If this is not something you like to support I can find a way around that, no problem.

I would like to register the Monolog logger like so.

LoggerInterface::class => function (ContainerInterface $container) {
$logger = new Monolog\Logger\Logger();
// Configure the logger in complex ways
return $logger;
}

Then I would like to replace Slim's default logger with Monolog like so:

$errorMiddleware = $app->addErrorMiddleware(false, true, true, $container->get(LoggerInterface::class));

At the moment Sim's ErrorHandler does not provide the exception to Monolog as described above, so I can't configure Monlog to e.g. send 500 errors by mail but only log 404 errors to a file.

As @t0mmy742 explained, providing the exception to a LoggerInterface implementation is explicitly mentioned and allowed in the PSR3-Specification.

Would adding this have any drawbacks? I understand that there are other ways to make this work, e.g. by implementing my own Slim ErrorHandler, but if the exception would be provided it would work "out of the box" and Slim users would not need do this.

But anyway, as mentioned above, if you don't want to support this, that's fine too. In that case feel free to close this issue.

[odan]

I would mention here that this specific approach (rely on an array key of the logger context) would imply some kind of indirect "contract", which I do not consider optimal.

I think in your specific use-case you may consider implementing a custom ErrorMiddleware that catches only specific HTTP Exceptions and handles that individually, e.g. sending a mail, logging to a file etc. Then just "degrade" the Slim ErrorMiddleware as "fallback" for all other Exceptions (or remove it completely).

[pdehne]

Understood. Well, the PSR-3 specification would make this at least an official indirect contract :-)

I thought the change would be a good addition to Slim in general, that's why I opened the issue to suggest it. I understand now that this is probably not the case. I can still use this approach, so no worries. I will subclass Slim's ErrorHandler, override the logError method like shown above and replace Slim's default ErrorHandler with my subclassed one.

Feel free to close the issue.

[l0gicgate]

I think this is an interesting issue. Considering that the PSR-3 LoggerInterface method we use is:

public function error($message, array $context = array());

There is room for context to be added which in this case would be the exception itself.

An Interim Solution: You will need to extend ErrorHandler and override the logError() method and do this:

<?php

namespace MyApp\Handlers;

use Slim\Handlers\ErrorHandler;


class MyCustomErrorHandler extends ErrorHandler {
    protected function $logError(string $error): void
    {
        $this->logger->error($error, ['exception' => $this->exception]);
    }
}

index.php

<?php
use MyApp\Handlers\MyCustomErrorHandler;
use Slim\Factory\AppFactory;

require __DIR__ . '/../vendor/autoload.php';

$app = AppFactory::create();

// Build your container with your dependencies
$container = new Container();

// ...

// Add Error Middleware and Use Your Custom Error Handler
$errorMiddleware = $app->addErrorMiddleware(true, true, true, $container->get(LoggerInterface::class));
$errorMiddleware->setDefaultErrorHandler(MyCustomerErrorHandler::class);

// ...

$app->run();

We I guess we need to decided whether or not we pass the additional context to the LoggerInterface::error() method. I'm not opposed to it, you can always provide your own LoggerInterface and do whatever you want with it anyway. More context is probably better than less as it fits more use cases.