Skip to content
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

[8.x] Improve content negotiation for exception handling #39385

Merged
merged 9 commits into from
Oct 28, 2021
Merged

[8.x] Improve content negotiation for exception handling #39385

merged 9 commits into from
Oct 28, 2021

Conversation

bilfeldt
Copy link
Contributor

@bilfeldt bilfeldt commented Oct 27, 2021

This is an update to the rejected PR #39361 which was incorrectly (my bad 🤦 ) only targeting AuthenticationException (pointed out by Taylor here).

This PR is almost identical but with two important changes:

  1. Uses shouldReturnJson() for all exceptions (done so in the main function render() but also in the protected functions called by this method).
  2. Added the exception as parameter to the shouldReturnJson method so that more complex logic can be performed by overriding this simple method. Requested by Taylor Otwell here

The new possibilities added by this PR is as already explained in PR #39361:

Well imagine an API request with header Accept: text/csv; application/json. For succesfull responses we return a csv response but in case of an exception then we return a json response. Currently this is not possible without overriding the render() method because $request->wantsJson() returns false (arguable not ideal but intentionally left out from this PR).

I would consider this PR as a non-breaking change since it merely extracts logic into a new method, but will let Dries and Graham discuss semantics about the proper definition of "non-breaking change" 😄

p.s. I was not able to edit PR #39361 hence this new PR - I apologize if it is in fact possible to edit a rejected PR (please share info about how this is done if anyone holds this info 👍 )

@bilfeldt bilfeldt changed the title Improve content negotiation for exception handling (updated to rejected PR #39361) Improve content negotiation for exception handling (updated version of rejected PR #39361) Oct 27, 2021
@GrahamCampbell GrahamCampbell changed the title Improve content negotiation for exception handling (updated version of rejected PR #39361) [8.x] Improve content negotiation for exception handling (updated version of rejected PR #39361) Oct 27, 2021
@driesvints driesvints changed the title [8.x] Improve content negotiation for exception handling (updated version of rejected PR #39361) [8.x] Improve content negotiation for exception handling Oct 27, 2021
@taylorotwell taylorotwell merged commit c2b6caf into laravel:8.x Oct 28, 2021
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.

None yet

2 participants