Slim: Logger not forwarded to own ErrorHandlingMiddleware

As written in documentaton the customErrorHandler should receive the logger of the ErrorMiddleware as 6th parameter. But unfortunately the customErrorHandler always receives null. This can be reproduced by just copying the code from the documentation. Did some research and it seems the logger is not correctly submitted to the handler in the line below.

https://github.com/slimphp/Slim/blob/5613cbb521081ed676d5d7eb3e44f2b80a818c24/Slim/Middleware/ErrorMiddleware.php#L127

I’ve added $this->logger locally on my server and now my customErrorHandler gets the logger as intended.
I’m not aware of side effects this could have so created an issue here and no pull request.

Best.

About this issue

  • Original URL
  • State: open
  • Created 4 years ago
  • Reactions: 2
  • Comments: 20 (16 by maintainers)

Most upvoted comments

One thing to remember is that it couldn’t have been implemented in the original PR since we would have broken ErrorHandlerInterface’s signature.

At this point, I think it may be worth pushing any further changes to the next major version instead where we can rework how we interface with logging entirely:

  • Get rid of $logError and $logErrorDetails in ErrorMiddleware and migrate those to the Logger class’s constructor.
  • Change ErrorHandlerInterface’s signature to accept LoggerInterface and get rid of the two aforementioned extraneous params there as well.

In the mean time, we should document this shortcoming in the docs. I think this may just be a trade-off of not breaking anything in a minor version.

@ddrv

I do agree that separating those concerns would make more sense:

  • We should remove logging entirely from ErrorMiddleware
  • Rename ErrorMiddleware to ErrorHandlingMiddleware
  • Create a new ErrorLoggingMiddleware
  • The ErrorLoggingMiddleware should be added before ErrorHandlingMiddleware and just catch -> log -> rethrow so ErrorHandlingMiddleware can catch and handle.

Relatively small changes for Slim 5 will at least make the upgrade path possible!