Skip to content

feat(router): support giving back json when a request fail#1629

Closed
NeoIsRecursive wants to merge 16 commits intotempestphp:mainfrom
NeoIsRecursive:feat/json-exception-responses
Closed

feat(router): support giving back json when a request fail#1629
NeoIsRecursive wants to merge 16 commits intotempestphp:mainfrom
NeoIsRecursive:feat/json-exception-responses

Conversation

@NeoIsRecursive
Copy link
Copy Markdown
Contributor

@NeoIsRecursive NeoIsRecursive commented Oct 8, 2025 •
edited
Loading

Fixes #1619

Okay, so this is very wip, but I wanted to get it out to see if I'm thinking about this correctly before doing more work on this 🙂

The error "shape" is taken from Laravel, which I think it is pretty easy to work with:
invalid & generic.

But it should probably be discussed how it should look (maybe problem details or some other easier "standard").

EDIT:

Another thing im thinking about is this:

if ($response->status->isServerError() || $response->status->isClientError()) {
    throw new HttpRequestFailed(
        request: $request,
        status: $response->status,
        cause: $response,
    );
}

Maybe the errors that are handled by the middleware (in forward) shouldn't be cast to HttpRequestFailed?
ConvertsToResponse, ValidationFailed and RouteBindingFailed.

kniziol
kniziol reviewed Oct 10, 2025
View reviewed changes
brendt
brendt reviewed Oct 11, 2025
View reviewed changes
brendt
brendt requested changes Oct 17, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@brendt brendt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add tests for this?

packages/router/src/Exceptions/HttpExceptionHandler.php
$throwable instanceof CsrfTokenDidNotMatch => $this->renderErrorResponse(Status::UNPROCESSABLE_CONTENT),
default => $this->renderErrorResponse(Status::INTERNAL_SERVER_ERROR),
$request->accepts(ContentType::HTML, ContentType::XHTML) => $this->handleHtml($throwable),
$request->accepts(ContentType::JSON) => $this->jsonHandler->render($throwable),
Copy link
Copy Markdown
Member

@brendt brendt Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we have HtmlHttpExceptionRenderer then for consistency?

Copy link
Copy Markdown
Contributor Author

@NeoIsRecursive NeoIsRecursive Oct 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, absolutely!

Would it make sense to remove the DeveloperExceptionHandler and in the HtmlHttpExceptionRenderer handle to show either whooosh or the custom view?

Tests for this (entire PR) is on my todo 🙂

Copy link
Copy Markdown
Member

@brendt brendt Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to remove the DeveloperExceptionHandler and in the HtmlHttpExceptionRenderer handle to show either whooosh or the custom view?

Maybe, but I'd say out of scope for this PR?

@brendt
Copy link
Copy Markdown
Member

brendt commented Nov 8, 2025

Wanted to check up: what needs to be done for this one, and can I help? Just want to make sure we don't loose track: it's a good feature and I'd like to see it merged 👍

@NeoIsRecursive
Copy link
Copy Markdown
Contributor Author

NeoIsRecursive commented Nov 8, 2025

Wanted to check up: what needs to be done for this one, and can I help? Just want to make sure we don't loose track: it's a good feature and I'd like to see it merged 👍

Hi, I haven't had a lot of time (nor energy tbh) the last few weeks, but I do have some stuff locally that I'll clean up and push up asap :)

Whats left:

  • Tests for the "exceptionRenderers"
  • Router tests
  • Make sure that the accepts guard has all the correct html content types

And I did try to remove the throwHttpExceptions thinggy but it triggered a lot of other components to fail so I'll leave that to someone else 😅

@github-actions
Copy link
Copy Markdown

github-actions bot commented Dec 9, 2025

This pull request is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the Stale This issue has seen no activity for a while label Dec 9, 2025
@github-actions
Copy link
Copy Markdown

github-actions bot commented Dec 10, 2025

This pull request was closed because it has been inactive for 1 day since being marked as stale.

@github-actions github-actions bot closed this Dec 10, 2025
@brendt brendt reopened this Dec 10, 2025
@github-actions github-actions bot removed the Stale This issue has seen no activity for a while label Dec 11, 2025
@brendt
Copy link
Copy Markdown
Member

brendt commented Dec 12, 2025

Let's close for now since the PR seems inactive. Happy to revive it when there's time :)

@brendt brendt closed this Dec 12, 2025
@innocenzi
Copy link
Copy Markdown
Member

innocenzi commented Dec 12, 2025

I might take a look at this at the same time I take a look at #1805

@NeoIsRecursive
Copy link
Copy Markdown
Contributor Author

NeoIsRecursive commented Dec 13, 2025

I might take a look at this at the same time I take a look at #1805

That would be great!
I have unfortunatly not had the time to work more on this.

Though ive been thinking about it and Im wondering if doing it like dotnet does might be nice? (register a exception handler, if it returns false, try the next, until the default one is ran)

@innocenzi innocenzi mentioned this pull request Dec 15, 2025
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brendt brendt brendt requested changes

@innocenzi innocenzi innocenzi left review comments

+1 more reviewer

@kniziol kniziol kniziol left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Json responses from exceptions and validation errors

4 participants

@NeoIsRecursive @brendt @innocenzi @kniziol