Skip to content

Conversation

@NeoIsRecursive
Copy link
Contributor

@NeoIsRecursive NeoIsRecursive commented Oct 8, 2025

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.

Copy link
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?

$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
Member

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
Contributor Author

@NeoIsRecursive NeoIsRecursive Oct 17, 2025

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
Member

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
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
Contributor Author

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

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

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
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
Member

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

@NeoIsRecursive
Copy link
Contributor Author

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Json responses from exceptions and validation errors

4 participants