Using wrong flag to write to error log in ErrorHandler

[mohammadhzp]

In ErrorHandler at line 262, the condition is checked against the wrong flag.

the condition must check against $this->logErrorDetails rather than $this->displayErrorDetails. so the line would be:

if (!$this->logErrorDetails) {

If the bug is valid, I can create a pull request to fix it.

More details: It seems writeToErrorLog() is used to save the logs properly somewhere and $this->displayErrorDetails is used by the renderer to show the error details in realtime on the client

[akrabat]

In this case, the test against displayErrorDetails being set to false is correct.

The idea here is that we we add a tip to the log to remind the developer that if they want to see the error in their browser, they need to set displayErrorDetails to true.

There's an argument that we could add a new suppressDisplayErrorDetailsTip that defaults to false that allows for a dev to hide this message as they intentionally have displayErrorDetails set the way they want it to be.

[mohammadhzp]

Please don't just close the issue. it is not resolved nor completed.

I get the idea here. but I'm not sure you are correct. for example, there is no need for suppressDisplayErrorDetailsTip for sure.

Let me elaborate.
When a log is added, We want to either:

• Display it
• Write it somewhere.
• Or both

Also, regarding the details, We may want to either:

• Show details of a log when displaying it or skip details and display the log without details
• Write the log to a backend with or without details (regardless of how we display it).

So, what if I don't want to display the details of a log but I want to write details of the same log to send it to a backend?

We need to understand the purpose of the flags:

The displayErrorDetails flag is for DISPLAYING logs.

The logErrorDetails flag is for writing the log to a destination.

Back to my first post. the condition I mentioned IS NOT meant for display and the developer must have used logErrorDetails to determine if we need log details. but the developer incorrectly used displayErrorDetails to determine if we need log details for a log which is never going to be displayed.

Pay attention, the function name is writeToErrorLog. That will clarify the incorrect flag usage.

Practical Example:
Right now, I have a JSON serializer and I want to add log details. but when JSON is created, the tip is added to the end of the JSON, resulting in an incorrect JSON object.

I can send a pull request about this.

[mohammadhzp]

Please reopen @akrabat

[akrabat]

As per your original report, the code is not using the wrong flag as the writeToErrorLog function is intended to add an additional string to the logged error if displayErrorDetails is false.

That is, the suggestion to change line 262 to if (!$this->logErrorDetails) { would be incorrect as that's not the intention of the code.

[akrabat]

Having said that, you have now provided additional information about an actual problem you have that you did not provide in the original report which shows that there is an issue to be addressed. It would have been helpful if you have provided the information about the production of invalid JSON at the start as we can now determine the correct resolution to your actual problem.

[mohammadhzp]

I'm still uncertain about your understanding regarding the intention of the code.

You can check where the function is called.
If you check, you'll get to line 132. as you can see, it is wrapped inside an if condition, the condition become true only if we want to write the log.

On line 135 you can see that a response is returned and writeToErrorLog has nothing to do with displaying.

Am I wrong? please guide me so that I can help to fix it.

[akrabat]

As I now understand it, the actual problem is that we are adding a plain text string to the output provided by an error renderer, where that renderer's output may be a formatted output that is not plain text.

My immediate ideas for resolution to this are that we could do one of:

1. Remove lines 262 to 265 completely
2. Add a new property suppressDisplayErrorDetailsTip and change line 262 to if (!$this->suppressDisplayErrorDetailsTip && !$this->displayErrorDetails) {. This would allow the develop to disable this addition to the logged error message if they need to.
3. Move the code that adds this message to the PlainTextRenderer, but we can't do that as we'd have to change the signature of ErrorRendererInterface::__invoke() which is a massive BC break! Hence we'd need a new Interface and have to test for it in writeToErrorLog and so on. i.e. quite a lot of work.

[akrabat]

I'm still uncertain about your understanding regarding the intention of the code.

You can check where the function is called. If you check, you'll get to line 132. as you can see, it is wrapped inside an if condition, the condition become true only if we want to write the log.

On line 135 you can see that a response is returned and writeToErrorLog has nothing to do with displaying.

Am I wrong? please guide me so that I can help to fix it.

You are correct that writeToErrorLog() is only called if $logErrors is true.

The intention of the if (!$this->displayErrorDetails) { test in writeToErrorLog() is to help the user work out why they can't see the error in their browser. i.e. if someone does not set $displayErrorDetails to true, then their browser will not show any useful information while they are developing and this is a hint to help them work that out.

[mohammadhzp]

Look at line 261. The second param is $this->logErrorDetails. the if statement below the line uses $this->displayErrorDetails which is a different flag.

Now look at line 300. The second param is $this->displayErrorDetails, which matches the if statement on line 262, therefore we must move the if statement on line 262 to here.

writeToErrorLog() is intended for writing to a log backend, and respond() is intended for displaying the error on the browser.

In response(), Response is returned. so that indicates that writeToErrorLog() has nothing to do with displaying an error on the browser.

If you guys are certain about keeping the tip. It can happen on line 303. we can simply append the tip to the body.

I'll open a PR tomorrow.

[mohammadhzp]

This is how I'd change writeToErrorLog() and respond() :

 protected function writeToErrorLog(): void
    {
        $renderer = $this->callableResolver->resolve($this->logErrorRenderer);
        $error = $renderer($this->exception, $this->logErrorDetails);
        $this->logError($error);
    }

protected function respond(): ResponseInterface
    {
        $response = $this->responseFactory->createResponse($this->statusCode);
        if ($this->contentType !== null && array_key_exists($this->contentType, $this->errorRenderers)) {
            $response = $response->withHeader('Content-type', $this->contentType);
        } else {
            $response = $response->withHeader('Content-type', $this->defaultErrorRendererContentType);
        }

        if ($this->exception instanceof HttpMethodNotAllowedException) {
            $allowedMethods = implode(', ', $this->exception->getAllowedMethods());
            $response = $response->withHeader('Allow', $allowedMethods);
        }

        $renderer = $this->determineRenderer();
        $body = call_user_func($renderer, $this->exception, $this->displayErrorDetails);
        if ($body !== false) {
            if (!$this->displayErrorDetails) {
                $body .= "\nTips: To display error details in HTTP response ";
                $body .= 'set "displayErrorDetails" to true in the ErrorHandler constructor.';
            }
            /** @var string $body */
            $response->getBody()->write($body);
        }

        return $response;
    }

[akrabat]

The important thing is that

if (!$this->displayErrorDetails) {
    $body .= "\nTips: To display error details in HTTP response ";
    $body .= 'set "displayErrorDetails" to true in the ErrorHandler constructor.';
}

is output to the log, not the the response.