Error Middleware Custom handler incorrect signature

[jelofson]

Hello, In your example of a custom error handler you have this:

// Define Custom Error Handler
$customErrorHandler = function (
    ServerRequestInterface $request,
    Throwable $exception,
    bool $displayErrorDetails,
    bool $logErrors,
    bool $logErrorDetails,
    ?LoggerInterface $logger = null
) use ($app) {
    $logger->error($exception->getMessage());

    $payload = ['error' => $exception->getMessage()];

    $response = $app->getResponseFactory()->createResponse();
    $response->getBody()->write(
        json_encode($payload, JSON_UNESCAPED_UNICODE)
    );

    return $response;
};

However, the ?LoggerInterface $logger = null will never be passed as the ErrorHandlerInterface does not accept a logger as the 6th param and the invoke of the handler looks like this:

return $handler($request, $exception, $this->displayErrorDetails, $this->logErrors, $this->logErrorDetails);

I am not sure if you want it to or not, but I thought I would let you know. In that example, you might have to get the logger some other way, such as in the use() statement.

[l0gicgate]

That is in fact correct. There is an error with the docs. I'm wondering if introducing an optional parameter at the end is in fact a breaking change 🤔

I'm also wondering how often this pattern is actually used. I would personally create a proper error handler and pass in the logger interface via dependency injection.

One of two solutions here, explore the possibility of passing the optional logger as the last parameter or change the docs and remove that from the example.

Open to feedback @odan @t0mmy742

[t0mmy742]

Adding LoggerInterface as an optional parameter is not a breaking change here.

• Slim\Handlers\ErrorHandler::_construct(): we can remove the optional LoggerInterface (maybe it's a BC, we should only deprecated it's use).

• Copy Slim\Handlers\ErrorHandler::getDefaultLogger() to Slim\Middleware\ErrorMiddleware

• Slim\Middleware\ErrorMiddleware::__construct():

  ...
  $this->logger = $logger ?? $this->getDefaultLogger();

• Slim\Middleware\ErrorMiddleware::handleException():

  ...
  return $handler($request, $exception, $this->displayErrorDetails, $this->logErrors, $this->logErrorDetails, $this->logger);

Changes to Slim\Handlers\ErrorHandler are breaking things, since there is a lot of public or protected. But we can easily keep old and new function definitions easily with optional parameters.

I would personally create a proper error handler and pass in the logger interface via dependency injection.

That's a possibility to get the LoggerInterface. But documentation need to be updated to explain both possiblities.

[jelofson]

I stumbled upon this when examining the custom error handler flow. I can only really see two options (though maybe my vision is not that great :) :

1. Just make a small change to the documentation showing a different approach to getting the logger (in the use, for example). This is probably easiest and lowest risk.
2. Modify the ErrorHandler __invoke() signature to include an optional $logger as the last param. Then, set $this->logger = the logger passed, if any.

Option 2 is more effort and higher risk and not really sure it's worth it. At least not at this point.

The ErrorMiddleware has an optional logger as the last parameter in the constructor. The only place this is referenced is when getting the default error handler, if the user did not define one. See getDefaultErrorHandler() in ErrorMiddleware.

[odan]

The documentation just needs to be fixed. So we should remove this line: ?LoggerInterface $logger = null from the example code.

Instead of a callback function, an object should be passed. The class of that custom error logger should then declare a Logger ( or a LoggerFactory) as dependency.

Here is an example:

A custom error handler class for the ErrorHandlerMiddleware:

<?php

namespace App\Handler;

use Fig\Http\Message\StatusCodeInterface;
use Psr\Http\Message\ResponseFactoryInterface;
use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface;
use Psr\Log\LoggerInterface;
use Slim\Exception\HttpException;
use Slim\Interfaces\ErrorHandlerInterface;

final class MyErrorHandler implements ErrorHandlerInterface
{
    private LoggerInterface $logger;
    private ResponseFactoryInterface $responseFactory;

    public function __construct(ResponseFactoryInterface $responseFactory, LoggerInterface $logger)
    {
        $this->responseFactory = $responseFactory;
        $this->logger = $logger;
    }

    public function __invoke(
        ServerRequestInterface $request,
        Throwable $exception,
        bool $displayErrorDetails,
        bool $logErrors,
        bool $logErrorDetails
    ): ResponseInterface {
        if ($logErrors === true) {
            $this->logger->error($exception->getMessage());
        }

        $statusCode = StatusCodeInterface::STATUS_INTERNAL_SERVER_ERROR;
        if ($exception instanceof HttpException) {
            $statusCode = (int)$exception->getCode();
        }

        $response = $this->responseFactory->createResponse($statusCode);
        $response->getBody()->write('An error has occurred');

        return $response;
    }
}

Usage:

<?php

use App\Handler\MyErrorHandler;
use Monolog\Handler\RotatingFileHandler;
use Monolog\Logger;
use Psr\Http\Message\ServerRequestInterface;
use Psr\Log\LoggerInterface;
use Slim\Factory\AppFactory;
use Slim\Middleware\ErrorMiddleware;
use Slim\Psr7\Response;

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

$app = AppFactory::create();

// Add Routing Middleware
$app->addRoutingMiddleware();

$errorMiddleware = new ErrorMiddleware(
    $app->getCallableResolver(),
    $app->getResponseFactory(),
    true, /* should be false in production */
    true,
    true
);

// Create a logger
$logger = new Logger('error');
$logger->pushHandler(new RotatingFileHandler('path/to/error.log'));

$myErrorHandler = new MyErrorHandler($app->getResponseFactory(), $logger);
$errorMiddleware->setDefaultErrorHandler($myErrorHandler);

// Register middleware
$app->add($errorMiddleware);