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] Ensure Router returns a JsonResponse for Arrayable, Jsonable (...) #17875

Merged
merged 1 commit into from
Feb 21, 2017
Merged

[5.5] Ensure Router returns a JsonResponse for Arrayable, Jsonable (...) #17875

merged 1 commit into from
Feb 21, 2017

Conversation

lucasmichot
Copy link
Contributor

@lucasmichot lucasmichot commented Feb 10, 2017

For now only a normal Response, mimicing a JsonResponse is returned; this enforces a JsonResponse to be returned.


A much more performant way to achieve #17869 /cc @taylorotwell

@taylorotwell
Copy link
Member

What is the actual benefit of this?

@lucasmichot
Copy link
Contributor Author

lucasmichot commented Feb 10, 2017

They are many @taylorotwell , in a normal Response, you cannot get and set the encoding option, you cannot set the callback either.
Within your API middlewares you can never be sure to always expect a JsonResponse too.

This PR solves that.

@taylorotwell
Copy link
Member

I really feel like instead of all this header business you should just check for Arrayable or Jsonable in the prepareResponse method of the router.

@lucasmichot lucasmichot changed the title [5.5] Ensure Arrayable and Jsonable (...) returns a JsonResponse. [5.5] Ensure Router returns a JsonResponse for Arrayable, Jsonable (...) Feb 12, 2017
@lucasmichot
Copy link
Contributor Author

lucasmichot commented Feb 12, 2017

@taylorotwell , the check is now done in the Router; see a7edae8.

@GrahamCampbell
Copy link
Member

👎 This forces people to use json at the router level.

@GrahamCampbell
Copy link
Member

What if they wanted a streamed json resource too?

@taylorotwell
Copy link
Member

@GrahamCampbell I don't follow. We were returning Response before and this just changes Arrayable and Jsonable to convert to JsonResponse if a more explicit response type is not provided? Can you give an example of what you mean?

@taylorotwell
Copy link
Member

@GrahamCampbell ?

*/
public function prepareResponse($request, $response)
{
if ($response instanceof PsrResponseInterface) {
$response = (new HttpFoundationFactory)->createResponse($response);
} elseif (! $response instanceof SymfonyResponse) {
$response = new Response($response);
if ($response instanceof Arrayable ||
Copy link
Member

Choose a reason for hiding this comment

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

too many spaces

@GrahamCampbell
Copy link
Member

Ah, right, probably fine then.

@taylorotwell taylorotwell merged commit 3442b67 into laravel:master Feb 21, 2017
@lucasmichot lucasmichot deleted the feature/master/response-isjson branch April 29, 2017 19:20
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.

3 participants