Logger not forwarded to own ErrorHandlingMiddleware

[GonozalIX]

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.

[l0gicgate]

Wow such an oversight on my part. I will open a PR to fix this asap. Thanks for reporting this!

[l0gicgate]

@elazar we screwed up injecting the LoggerInterface inside of ErrorHandler. It should have been passed via __invoke() instead of via the constructor: https://github.com/slimphp/Slim/blob/4.x/Slim/Handlers/ErrorHandler.php#L162

I'm not sure how to elegantly fix this without introducing a breaking change by removing it from the constructor: https://github.com/slimphp/Slim/blob/4.x/Slim/Handlers/ErrorHandler.php#L143

If we leave it in the constructor, then what happens during the invocation? Do we replace the logger that it was originally constructed with? We need to find an order of precedence.

@adriansuter thoughts?

[elazar]

[l0gicgate]

@elazar there's another problem here ErrorHandlerInterface would also get broken if we introduced a new parameter to it even though it's nullable. I'm not sure how we can elegantly fix this without breaking stuff.

[elazar]

@l0gicgate I'm not sure there's an elegant way to address this either, honestly. The functionality as it stands is broken regardless.

I'm not sure API changes would make the situation that much worse since they were originally written to avoid breaking BC anyway.

The constructor parameter currently defaults to null. If we remove it, I don't think PHP will complain about receiving an additional parameter value for any code still passing it to the constructor; it'll simply have no effect.

We can basically just relocate the parameter as-is to be a parameter of __invoke() instead. It'll still have a null default and still have a default populated just as it does in the constructor now.

Technically, yes, we're changing signatures, but not in a way that should break any calling code that wasn't already indirectly broken by this feature not working.

[l0gicgate]

I don't think PHP will complain about receiving an additional parameter value for any code still passing it to the constructor

Let’s test this to make sure

We can basically just relocate the parameter as-is to be a parameter of __invoke() instead. It'll still have a null default and still have a default populated just as it does in the constructor now.

I’m almost 100% sure that changing the signature of an interface will break any code implementing it regardless of if the additional parameter at the end is nullable or not. We should test this as well before making any further changes.

[l0gicgate]

Modifying the interface with a nullable parameter in fact breaks:

[l0gicgate]

On the other hand, an object instantiated with an extra constructor parameter does not break:

[elazar]

OK, so changing the interface isn't an option. That basically leaves us with changing how the feature works, and updating the docs accordingly.

We could, for example, add a check in ErrorMiddleware->handleException() to see if the return value of getErrorHandler() implements LoggerAwareInterface and, if so, inject the logger into it at that point, since ErrorMiddleware is already receiving it via its own constructor. The handler can do whatever it wants to do with the logger at that point.

This unfortunately excludes the use of lambdas, but I'm not sure there's another way that doesn't involve an API-breaking change. This would at least make working functionality available in some form now, and the API could be revisited in Slim 5.

[l0gicgate]

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]

@elazar @l0gicgate @akrabat In the addErrorMiddleware() method of Applicatoin, you can add error handling to the middleware stack and add error logging as separate middleware (so that logging occurs first, then handling). Then the logger will not be needed for the custom error handler.

[piotr-cz]

@ddrv Good point, also think it's good idea to separate error displaying and logging into two middlewares.

[l0gicgate]

@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.

[akrabat]

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

[ybelenko]

I would rename title of this issue. From the first sight it says "Slim internal ErrorMiddleware is broken" which is not. Then after a few comments I understand that it's related to custom handlers only.

Btw almost ~~10 days~~ 1 YEAR and 10 days passed from the issue open, but 6th argument is still in docs Adding Custom Error Handlers

[GonozalIX]

@ybelenko I guess you mean… 1 YEAR and 10 days.

[ybelenko]

@ybelenko I guess you mean… 1 YEAR and 10 days.

Wow... Yeap I didn't notice date year 🤣

[l0gicgate]

We do need to fix the docs. And yes, the issue has been open for over a year because this can't be resolved until Slim 5 without breaking things. Slim 5 isn't even in development.

Managing 10+ repos, with 2 full time jobs isn't easy, all help is welcomed.

[ybelenko]

We do need to fix the docs. And yes, the issue has been open for over a year because this can't be resolved until Slim 5 without breaking things. Slim 5 isn't even in development.

I'm not whining that bug not fixed, it's open source software without any warranties. It's just weird that docs not updated after a year, so users still can face the same bug because of a wrong line in doc example.

@l0gicgate, as opensource developer(without starred repos for now) myself, I appreciate your work.

[l0gicgate]

@l0gicgate, as opensource developer(without starred repos for now) myself, I appreciate your work

Thank you

@ybelenko updated the docs: https://github.com/slimphp/Slim-Website/pull/563