Issue with how WhoopsErrorHandler processes a Whoops object with multiple handlers in its stack

[alrav]

Hello all - I believe this issue becomes apparent when you use the WhoopsErrorHandler along with WhoopsFactory - which is configured to output AJAX errors in JSON format, based on the below configuration:

    'whoops' => [
        'json_exceptions' => [
            'display'    => true,
            'show_trace' => true,
            'ajax_only'  => true,
        ],
    ],

However, it always outputs in HTML instead.

The reason is because the WhoopsErrorHandler always pushes the injected PrettyPageHandler ($this->whoopsHandler) object to the top of the stack, even if other handlers were already configured in Whoops:

// in WhoopsErrorHandler::handleException
$this->whoops->pushHandler($this->whoopsHandler);

So you might have a Whoops handler stack like the one created by WhoopsFactory:

1. PrettyPageHandler
2. JsonResponseHandler

But, WhoopsErrorHandler will always add PrettyPageHandler to the top, which gets popped first and, therefore, not allowing JsonResponseHandler to be called. The final stack would look like:

1. PrettyPageHandler
2. JsonResponseHandler
3. PrettyPageHandler

[geerteltink]

I have tried to debug this as well I didn't get anywhere with WhoopsFactory. The only way I got this working properly was hacking the WhoopsErrorHandler. It needs some more work to add the json exception config.

// Config
'dependencies' => [
    'invokables' => [
        'Zend\Expressive\Whoops' => Whoops\Run::class,
        'Zend\Expressive\WhoopsPageHandler' => Whoops\Handler\PrettyPageHandler::class,
    ],
    'factories' => [
        'Zend\Expressive\FinalHandler' => Zend\Expressive\Container\WhoopsErrorHandlerFactory::class,
    ],
],

// Zend\Expressive\WhoopsErrorHandler
protected function handleException($exception, Request $request, Response $response)
{
    if (in_array('xmlhttprequest', $request->getHeader('x-requested-with'))) {
        $this->whoops->writeToOutput(false);
        $jsonHandler = new JsonResponseHandler();
        $jsonHandler->addTraceToOutput(true);
        $this->whoops->pushHandler($jsonHandler);
    } else {
        $this->prepareWhoopsHandler($request);
        $this->whoops->pushHandler($this->whoopsHandler);
    }

    $response
        ->getBody()
        ->write($this->whoops->handleException($exception));

    return $response;
}

[michaelmoussa]

I played around with this a bit too and all the issues I ran into stemmed from the fact that you must use a PrettyPageHandler in order to use the the built-in WhoopsErrorHandler. It's a typehinted constructor argument, which gets automatically pushed at error handling time, meaning that you can't (a) inject a different handler if you want to, or (b) push another handler onto the stack to supersede the PrettyPageHandler.

I think there's a better way to go about this, but unfortunately pretty much any approach would result in a BC break, so it would have to be targeted for a 2.x release. Here's the quick "hack job" version I came up with, which I can clean up and add tests to if the approach looks good.

// WhoopsErrorHandlerFactory
        /** @var Run $whoops */
        $whoops = $container->get('Zend\Expressive\Whoops');
        $whoops->pushHandler($container->get('Zend\Expressive\WhoopsPageHandler'));

        return new WhoopsErrorHandler(
            $whoops,
            $template,
            (isset($config['template_404']) ? $config['template_404'] : 'error/404'),
            (isset($config['template_error']) ? $config['template_error'] : 'error/error')
        );

Here, instead of passing the Whoops\Run instance and the WhoopsPageHandler (which must be a PrettyPageHandler), it just asks for the Whoops\Run instance, which will already contain whichever handler(s) you want to use. Of course, that means there's no requirement for what type of handler will be used, meaning you can do:

'Zend\Expressive\WhoopsPageHandler' => Whoops\Handler\PrettyPageHandler::class

or

'Zend\Expressive\WhoopsPageHandler' => Whoops\Handler\JsonResponseHandler::class

or

'Zend\Expressive\WhoopsPageHandler' => My\Custom\WhoopsHandler::class

Next, we'd need to change the WhoopsErrorHandler constructor:

    public function __construct(
        Whoops $whoops,
        // <--- no more handler here
        Template\TemplateRendererInterface $renderer = null,
        $template404 = 'error/404',
        $templateError = 'error/error',
        Response $originalResponse = null
    ) {
        $this->whoops = $whoops;
        parent::__construct($renderer, $template404, $templateError, $originalResponse);
    }

Finally, in the prepareWhoopsHandler(...) method that gets called right before writing the response:

    foreach ($this->whoops->getHandlers() as &$handler) {
        if ($handler instanceof PrettyPageHandler) {
            $uri = $request->getUri();
            $handler->addDataTable('Expressive Application Request', [
               ... etc ...
        }
    }

It's certainly cleaner to assume that it'll be a PrettyPageHandler and just go ahead and call ->addDataTable(..), but that won't always be the case. What if it's a CLI application and I want to use the PlainTextHandler? Or an API response and I want to use the JsonResponseHandler? None of those support adding additional data like the PrettyPageHandler.

That's the high-level view of what this approach would entail. We could take this one step further and create a custom CallbackHandler that would ship with Expressive. The CallbackHandler could:

1. Accept some configuration to indicate what underlying handler(s) you want it to use.
2. Add additional information pertaining to the Request to the PlainTextHandler and JsonResponseHandler (which the current setup wouldn't allow for... we'd need some kind of output buffering to capture what those handlers emit, parse it, then inject the additional info we want).
3. Allow additional configurability by the developer.

Obviously, as part of this, we'd fix the issues with Whoops 1.x vs. 2.x flagged in the skeleton repo.

Thoughts?

[michaelmoussa]

@alrav I think the above (or some variation of it) will take care of the issue long-term, but do you have a workaround in place right now for your immediate problem preventing use of the JsonResponseHandler?

[geerteltink]

@michaelmoussa So just thinking out loud:

Can we assume that

• a xmlhttprequest request always needs to return the JsonResponseHandler
• a XML request should return the XmlResponseHandler
• a http request returns the PrettyPageHandler (if it exists in the config)
• otherwise (e.g. cli) fallback to PlainTextHandler

If so, I think it might be possible to rewrite the WhoopsErrorHandler without BC Break. However it would make the configuration useless, except for 'Zend\Expressive\WhoopsPageHandler'.

Basically detect the request type and push the right handler.

[michaelmoussa]

I don't think those are all necessarily safe assumptions.

JsonResponseHandler vs.XmlResponseHandler would probably depend on the Accepts header with a configurable default if no header is present. Just because the incoming request contains XML or JSON doesn't necessarily mean the response should.

PrettyPageHandler could make sense for an http request, but what if I'm making an http request for an API endpoint using curl or HTTPie? Pretty page would be unreadable.

I'm OK with a BC-breaking approach since we could release it in 2.0.0 without violating semver, but perhaps an approach we could take for a 1.x compatible workaround would be to check in the WhoopsErrorHandler to see if there is already a non-PrettyPageHandler at the top of the handler stack? If there is, then it was put there by the developer, and it's reasonable to assume it was put there because they wanted it to be used. If that's the case, we could skip pushing the additional handler onto the stack when handling the exception.

It's not as flexible as the other alternatives mentioned thus far, but at least it'd let something besides the pretty page handler be used while still using the components provided by Expressive

[geerteltink]

If it's gonna be a BC break we might want to wait since @weierophinney is planning rewriting the complete error handling anyway.

In the mean time... what if we change the constructor to this:

    public function __construct(
        Whoops $whoops,
        PrettyPageHandler $whoopsHandler = null, // <--- instead of removing, make the handler optional
        Template\TemplateRendererInterface $renderer = null,
        $template404 = 'error/404',
        $templateError = 'error/error',
        Response $originalResponse = null
    ) {
        $this->whoops        = $whoops;
        $this->whoopsHandler = $whoopsHandler;
        parent::__construct($renderer, $template404, $templateError, $originalResponse);
    }

This wouldn't qualify as a BC break right?

And then in the handleException method we check if the PrettyPageHandler is set or if it's already registered and return early in both cases. This would give the needed flexibility without a BC break.

[michaelmoussa]

This wouldn't qualify as a BC break right?

If we're just changing the argument to be nullable, then no, it wouldn't break BC even if someone is extending this class. The WhoopsErrorHandlerFactory would need to be changed in order to always push whatever the handler is onto the stack and simply never use that 2nd param (technically it could use the 2nd param if the handler is a PrettyPageHandler, but it'd probably be better to be consistent.

So we'd end up with something like this:

// WhoopsErrorHandlerFactory

        /** @var \Whoops\Run $whoops */
        $whoops = $container->get('Zend\Expressive\Whoops');
        $whoops->pushHandler($container->get('Zend\Expressive\WhoopsPageHandler')); // <--- can be any type of Whoops Handler

        return new WhoopsErrorHandler(
            $whoops,
            null, // <--- always null now, since the handler goes in on the stack
            $template,
            (isset($config['template_404']) ? $config['template_404'] : 'error/404'),
            (isset($config['template_error']) ? $config['template_error'] : 'error/error')
        );

then, in the WhoopsErrorHandler, we make that 2nd param optional and change the prepare method to only add the datatable if the handler is a PrettyPageHandler. I just tried this out and was able to get all of the different handlers to be used by changing only the Zend\Expressive\WhoopsPageHandler invokable setting.

I'm a little confused about $/src/Container/WhoopsFactory.php though. Seems the behavior here copies a bit what the WhoopsErrorHandlerFactory does as far as pushing handlers and such, only in this case it can use the whoops.json_exceptions settings. I tried my modified code while using the WhoopsFactory instead of the Whoops\Run invokable and it still worked, so I'm not entirely sure what the reason to use one over the other would be.

[alrav]

@xtreamwayz , @michaelmoussa - sorry for the delay as timezone issues =)

Regarding my workaround - I bypassed the WhoopsFactory (for the same reason you noted @michaelmoussa, where I didn't really have a use for it at the moment), and created a custom WhoopsErrorHandler similar to some of the approaches mentioned above since the application not only will require JSON output for AJAX, but also for any API requests with a relevant Accept header. It works similar to zfcampus/zf-content-negotiation concepts:

If (Accept is application/json) -> push a JsonResponseHandler
If (is an AJAX request) -> push a JsonResponseHandler (as a default, but this can be another content-type, etc.)
Else -> push a PrettyPageHandler

When experimenting with a local BC fix proto-type, I also went with a similar approach in modifying WhoopsErrorHandler::handleException to check if PrettyPageHandler was already present in the stack and is the same instance of the $this->whoopsHandler, and if so prepareWhoopsHandler; if not, then push $this->whoopsHandler property on to the stack as a fallback.

It started to feel as if $this->whoopsHandler was a nullable property just for fallback purposes and for prepareWhoopsHandler decoration.

[geerteltink]

@michaelmoussa Your solution works for 1 single handler. However what if you have an app that serves an api and html pages?

Ok, I've got a working solution that supports both version 1 and 2. It adds the JsonHandler if found in the config. It also makes the WhoopsFactory obsolete. And it detects PrettyPageHandlers and applies the request data: #384.

[weierophinney]

Fixed with #384.