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

[5.5] Fixed json responses #20313

Merged
merged 4 commits into from
Jul 30, 2017
Merged

[5.5] Fixed json responses #20313

merged 4 commits into from
Jul 30, 2017

Conversation

GrahamCampbell
Copy link
Member

@GrahamCampbell GrahamCampbell commented Jul 29, 2017

The current code breaks the exception handler returning json responses: https://github.com/laravel/framework/blob/5.4/src/Illuminate/Foundation/Exceptions/Handler.php#L223.

Here are the headers before and after:

image

Clearly this is wrong. The laravel response class should never be removing content types provided by the user!

@GrahamCampbell GrahamCampbell changed the title Fixed json responses [5.5] Fixed json responses Jul 29, 2017
@taylorotwell
Copy link
Member

We would need a description.

@GrahamCampbell
Copy link
Member Author

GrahamCampbell commented Jul 29, 2017

We would need a description.

Ha, see slack. I'll copy it over. Give me a min. :P Also, the test I've added shows what this fixes. :trollface:

@GrahamCampbell
Copy link
Member Author

Description added. Was jet lagged af when I PRed this (still am). :D

@taylorotwell
Copy link
Member

I feel like this had to there for a reason and we are probably breaking something by removing it. I would like to find out what that is. /cc @themsaid

@m1guelpf
Copy link
Contributor

@taylorotwell Maybe you should start on the PR this was added... #18314

@taylorotwell
Copy link
Member

@lucasmichot any comment on this?

@GrahamCampbell
Copy link
Member Author

I feel like this had to there for a reason and we are probably breaking something by removing it.

The other PR didn't provide a real reason?

@GrahamCampbell
Copy link
Member Author

The reason given was:

image

and yet it only works after this PR is merged, because we can't pass real json anymore.

@GrahamCampbell
Copy link
Member Author

I think the original PR was stupid anyway, because it forces everything to json, and doesn't let you opt-out.

@taylorotwell taylorotwell merged commit 9369141 into master Jul 30, 2017
@GrahamCampbell GrahamCampbell deleted the json branch July 30, 2017 00:07
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

3 participants