Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

JSON error responses should have relevant HTTP status codes #3890

Closed
timkelty opened this issue Feb 22, 2019 · 3 comments
Closed

JSON error responses should have relevant HTTP status codes #3890

timkelty opened this issue Feb 22, 2019 · 3 comments
Labels
content administration 🤓 features related to content administration enhancement improvements to existing features

Comments

@timkelty
Copy link
Contributor

timkelty commented Feb 22, 2019

Throughout Craft's controllers, anywhere that returns JSON with errors, the response is a 200.

It really should be 4xx or 5xx, so it can be properly handled.

A good solution might be to add this to craft\web\Controller::asErrorJson, and use that more universally.

I'm thinking something like this might be ideal:

public function asErrorJson($data, int $statusCode = 500): YiiResponse
{
    $data = is_string($data) ? ['error' => $error] : $data;
    
    return $this->asJson($data)
        ->setStatusCode($statusCode);
}

Or perhaps something else to unify all JSON responses (success, error, errors, message, etc: #2495)

@brandonkelly
Copy link
Member

Agree this would be better. However it would be a breaking change for anyone creating these requests using jQuery Ajax methods, as the success method would no longer be called on non-200 responses. (An improvement to be sure but will have to wait until Craft 4.)

@brandonkelly brandonkelly added enhancement improvements to existing features site development 👩‍💻 features related to website/API development labels Feb 23, 2019
@timkelty
Copy link
Contributor Author

Thanks @brandonkelly.

In the meantime I wonder if I can duck-punch craft\web\Controller with an override of asJson that checks for error(s) in the params – and load it in config/app.php?

@brandonkelly
Copy link
Member

You could hack it in by creating a module with this:

use yii\base\Event;
use yii\web\Response;

Event::on(Response::class, Response::EVENT_BEFORE_SEND, function(Event $e) {
    /** @var Response $response */
    $response = $e->sender;

    if (
        $response->format === Response::FORMAT_JSON &&
        !empty($response->data['error']) &&
        $response->getStatusCode() === 200 &&
        \Craft::$app->getRequest()->getIsSiteRequest()
    ) {
        $response->setStatusCode(400);
    }
});

@brandonkelly brandonkelly added this to the 4.0 milestone Mar 21, 2019
@brandonkelly brandonkelly added content administration 🤓 features related to content administration and removed site development 👩‍💻 features related to website/API development labels Oct 21, 2019
@craftcms craftcms locked and limited conversation to collaborators Jun 22, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
content administration 🤓 features related to content administration enhancement improvements to existing features
Projects
None yet
Development

No branches or pull requests

2 participants