Multiple exceptions result in invalid JSON

[chocolatkey]

When multiple exceptions are thrown and then handled by the HttpErrorHandler, the resulting response contains invalid JSON, for example:

{
    "statusCode": 500,
    "error": {
        "type": "SERVER_ERROR",
        "description": "DateTime::__construct(): Failed to parse time string (1569885313) at position 8 (1): Unexpected character"
    }
}{
    "statusCode": 500,
    "error": {
        "type": "SERVER_ERROR",
        "description": "ERROR: DateTime::__construct(): Failed to parse time string (1569885313) at position 8 (1): Unexpected character on line 117 in file C:\\xxxx\\vendor\\paragonie\\paseto\\src\\JsonToken.php."
    }
}

[chocolatkey]

Actually, the same thing happens if it's using the Slim ErrorHandler:

[chocolatkey]

It looks like what is happening is that the error is being rendered a first time in index.php by $response = $app->handle($request);, and then a second time in the $shutdownHandler, which runs on script exit

[chocolatkey]

A primitive fix is to do test for any previous errors:

// Run App & Emit Response
$response = $app->handle($request);
if(!error_get_last()) {
    $responseEmitter = new ResponseEmitter();
    $responseEmitter->emit($response);
}

but there's probably a better way

[johi]

I stumbled into the same thing, when implementing a user registration use case. In my case I added a user to my db with an already existing email that has a unique constraint, which resulted in an uncaught exception, the following lookup on the user by failed with UserNotFoundException. But then I realized, that all exceptions must be caught in the code because they ultimately have to be caught in src/Application/Handlers/HttpErrorHandler.php. So make sure you create the right Exception classes that your code will ultimately throw one of the Http*Exception classes, and you should be fine.

I think the behaviour is quite right for unhandled exceptions, as it makes you aware of these cases during development and implementing the right Exceptions makes you code more robust in all cases, so I think the way you are trying to fix it, is trying to fix a non-issue as I would personally prefer all possible exceptions to be caught and handled by the excellent HttpErrorHandler. I hope it makes sense.

[l0gicgate]

Actually this is because of the ResponseEmitter's following lines:
https://github.com/slimphp/Slim-Skeleton/blob/master/src/Application/ResponseEmitter/ResponseEmitter.php#L28-L30

It should be replaced with this:

if (ob_get_length() > 0) {
    ob_end_clean();
    ob_start();
}

I don't have time to raise a PR right now, but we should actually fix this bug and write a test case for it. Feel free to do so if you can.

[chocolatkey]

@l0gicgate I have replaced those lines with your code, and the double response still happens. ResponseEmitter emit looks like this:

public function emit(ResponseInterface $response): void
    {
        // This variable should be set to the allowed host from which your API can be accessed with
        $origin = isset($_SERVER['HTTP_ORIGIN']) ? $_SERVER['HTTP_ORIGIN'] : '*';

        $response = $response
            ->withHeader('Access-Control-Allow-Credentials', 'true')
            ->withHeader('Access-Control-Allow-Origin', $origin)
            ->withHeader('Access-Control-Allow-Headers', 'X-Requested-With, Content-Type, Accept, Origin, Authorization')
            ->withHeader('Access-Control-Allow-Methods', 'GET, POST, PUT, PATCH, DELETE, OPTIONS')
            ->withHeader('Cache-Control', 'no-store, no-cache, must-revalidate, max-age=0')
            ->withAddedHeader('Cache-Control', 'post-check=0, pre-check=0')
            ->withHeader('Pragma', 'no-cache');

        if (ob_get_length() > 0) {
            ob_end_clean();
            ob_start();
        }

        parent::emit($response);
    }

A response after an exception:

{
    "statusCode": 500,
    "error": {
        "type": "SERVER_ERROR",
        "description": "Argument 1 passed to ParagonIE\\Paseto\\Parser::getPublic() must be an instance of ParagonIE\\Paseto\\Keys\\AsymmetricPublicKey, null given, called in C:\\xxx\\TicketManifestAction.php on line 57"
    }
}{
    "statusCode": 500,
    "error": {
        "type": "SERVER_ERROR",
        "description": "ERROR: Undefined variable: public_key on line 57 in file C:\\xxx\\TicketManifestAction.php."
    }
}

[l0gicgate]

@chocolatkey try this instead. I'm not sure if you were on master or not I remember we fixed this:

if (ob_get_contents()) {
    ob_clean();
}

[chocolatkey]

@l0gicgate That's what it was initially in the version I have, like you linked here: https://github.com/slimphp/Slim-Skeleton/blob/master/src/Application/ResponseEmitter/ResponseEmitter.php#L28-L30

edit: hmm it's not happening anymore now that I went back to that

[l0gicgate]

Fix in #134