Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Difference between displayErrorDetails and printing to error log #2393

Closed
edudobay opened this issue Feb 9, 2018 · 6 comments
Closed

Difference between displayErrorDetails and printing to error log #2393

edudobay opened this issue Feb 9, 2018 · 6 comments

Comments

@edudobay
Copy link
Contributor

edudobay commented Feb 9, 2018

Hi! I was configuring error reporting for an application and started to wonder about the displayErrorDetails setting: why is displaying errors to the client coupled to not showing them on the log? There are use cases during development where I might want to display the errors to the client but also have them at the log (e.g. if the client may be a mobile app or another application which might not give me a chance to look at the full response body).

Overall, it seems to me that switching displaying of error details and printing to the log are separate concepts and should be independent settings. Does this make sense to implement? Of course Slim is not a logging framework and the implementation for this should be kept simple. I would be happy to contribute a PR for this!

@odan
Copy link
Contributor

odan commented Feb 9, 2018

Hi @edudobay. I'm not sure whether I get your question right. Have you tried to implement a custom slim error handler? There you have the possibility to decide which (and when) error details are displayed to the client and can log all errors in detail using a logger (e. g. Monolog).

@edudobay
Copy link
Contributor Author

edudobay commented Feb 9, 2018

Hi! Yes, I had no problem implementing a custom error handler, but to keep things simple I wanted to reuse the logging behavior that Slim already provides out of the box. The problem is that I can't reuse just the logging part because it's coupled to the rendering part. If I turn on error rendering, logging gets turned off and that’s what I’d like to change.

My current solution is an ugly subclass that “fakes” the displayErrorDetails setting when delegating to the log writer. Of course this is coupled to the Slim implementation; the proposal for Slim 4.x at #2222 (AbstractErrorHandler) would break this, for example.

class ErrorHandlerWithLogging extends \Slim\Handlers\Error
{

    protected function writeToErrorLog($throwable)
    {
        $oldValue = $this->displayErrorDetails;
        $this->displayErrorDetails = false;
        parent::writeToErrorLog($throwable);
        $this->displayErrorDetails = $oldValue;
    }
}

The most obvious “pretty” solution for me would be for the constructor to take two flags, say displayErrorDetails and writeToErrorLog - and the latter could default to the opposite of the former when not provided, to guarantee backwards-compatible behavior. Something like this:

class Error extends AbstractError
{
    public function __construct($displayErrorDetails = false, $writeToErrorLog = null)
    {
        if ($writeToErrorLog === null) {
            $this->writeToErrorLog = !$displayErrorDetails;
        } else {
            $this->writeToErrorLog = (bool) $writeToErrorLog;
        }

        // ...
    }
}

Am I going too far suggesting this? Or is it possible that Slim could have this separate setting, at least configurable from the constructor of the Error handler class?

@odan
Copy link
Contributor

odan commented Feb 9, 2018

The problem is, that in Slim 4 the method writeToErrorLog is getting only called if displayErrorDetails is true. See here. That means overwriting the writeToErrorLog method in a extended class will not work in Slim 4.

Have you tried to implement a custom ExceptionMiddleware?

Example:

// Exception middleware
$app->add(function (Request $request, Response $response, $next) {
    try {
        return $next($request, $response);
    } catch (Exception $ex) {
        // logging etc...
    }
});

@l0gicgate
Copy link
Member

@odan despite not being a viable solution in Slim 4, someone will be able to simply extend the AbstractErrorHandler and stub the constructor and do whatever they want.

#2222 has been closed. The concepts of that PR have now been implemented in #2398.

If you have any suggestions, feel free to comment on that PR!

Thanks

@odan
Copy link
Contributor

odan commented Feb 17, 2018

@l0gicgate Now, that the PR #2222 has been closed, my previous answer is not valid anymore. I think the new Error middleware #2398 is the way to go. I will review it and give you a feedback as soon as possible.

@geggleto
Copy link
Member

closing as resolved with #2398

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants