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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

withJson should not set http status code implicitly #1737

Merged
merged 4 commits into from Feb 6, 2016

Conversation

Projects
None yet
4 participants
@GHKEN
Contributor

GHKEN commented Jan 14, 2016

I thought that withJson set http status code confusing user.
e.g.
$response->withStatus(500)->withJson([])
and
$response->withJson([])->withStatus(500)
are not equally.馃槩
We should fix withJson to intuitive.
Thank you for your consideration.

@AndrewCarterUK

This comment has been minimized.

Show comment
Hide comment
@AndrewCarterUK

AndrewCarterUK Jan 14, 2016

Contributor

Unfortunately this would be a backwards incompatible change (it would break the code of anyone using this method when they ran a composer update).

Contributor

AndrewCarterUK commented Jan 14, 2016

Unfortunately this would be a backwards incompatible change (it would break the code of anyone using this method when they ran a composer update).

@GHKEN

This comment has been minimized.

Show comment
Hide comment
@GHKEN

GHKEN Jan 14, 2016

Contributor

Thank you for the comment.
I thought that default status code to null is better if we need backwards compatibility.

Contributor

GHKEN commented Jan 14, 2016

Thank you for the comment.
I thought that default status code to null is better if we need backwards compatibility.

@silentworks

This comment has been minimized.

Show comment
Hide comment
@silentworks

silentworks Jan 14, 2016

Member

This is still a BC break. Also why wouldn't you do $response->withJson([], 500) if you need a different status code?

Member

silentworks commented Jan 14, 2016

This is still a BC break. Also why wouldn't you do $response->withJson([], 500) if you need a different status code?

@AndrewCarterUK

This comment has been minimized.

Show comment
Hide comment
@AndrewCarterUK

AndrewCarterUK Jan 14, 2016

The PSR coding standards used in this project state that this should be written:

$responseWithJson

AndrewCarterUK commented on Slim/Http/Response.php in 0884f71 Jan 14, 2016

The PSR coding standards used in this project state that this should be written:

$responseWithJson
@AndrewCarterUK

This comment has been minimized.

Show comment
Hide comment
@AndrewCarterUK

AndrewCarterUK Jan 14, 2016

Contributor

@silentworks I can see the issue.

$response = $response->withStatusCode(404)->withJson([]);

This code should return a response with a 404 status code.

Contributor

AndrewCarterUK commented Jan 14, 2016

@silentworks I can see the issue.

$response = $response->withStatusCode(404)->withJson([]);

This code should return a response with a 404 status code.

@silentworks

This comment has been minimized.

Show comment
Hide comment
@silentworks

silentworks Jan 14, 2016

Member

Please also note that withJson is a helper method, it should help you write less code for a specific purpose.

@AndrewCarterUK that should return a response with 200, if the developer wants it to return a status of 404 either do:

$response = $response->withStatusCode(404)
    ->withHeader('Content-Type', 'application/json;charset=utf-8')
    ->getBody()->write(json_encode([]));

or

$response->withJson([], 404);
Member

silentworks commented Jan 14, 2016

Please also note that withJson is a helper method, it should help you write less code for a specific purpose.

@AndrewCarterUK that should return a response with 200, if the developer wants it to return a status of 404 either do:

$response = $response->withStatusCode(404)
    ->withHeader('Content-Type', 'application/json;charset=utf-8')
    ->getBody()->write(json_encode([]));

or

$response->withJson([], 404);
@AndrewCarterUK

This comment has been minimized.

Show comment
Hide comment
@AndrewCarterUK

AndrewCarterUK Jan 14, 2016

Contributor

It 'does' return that status code, I wouldn't say that it should.

withJson should just give you your response 'with json'. Changing the status code is a non-obvious side effect that I can see causing problems (it already has enough to trigger this PR).

Also, by default the status code would be 200, so it's a truly pointless feature if that was the intent. All it could do is overwrite an implicit change.

Contributor

AndrewCarterUK commented Jan 14, 2016

It 'does' return that status code, I wouldn't say that it should.

withJson should just give you your response 'with json'. Changing the status code is a non-obvious side effect that I can see causing problems (it already has enough to trigger this PR).

Also, by default the status code would be 200, so it's a truly pointless feature if that was the intent. All it could do is overwrite an implicit change.

@silentworks silentworks added this to the 3.2.0 milestone Jan 14, 2016

@GHKEN GHKEN changed the title from withJson shoult not set http status code implicitly to withJson should not set http status code implicitly Jan 14, 2016

@akrabat akrabat self-assigned this Feb 5, 2016

@akrabat

This comment has been minimized.

Show comment
Hide comment
@akrabat

akrabat Feb 5, 2016

Member

Can we have a unit test for this please?

Member

akrabat commented Feb 5, 2016

Can we have a unit test for this please?

@akrabat akrabat merged commit a8b5da4 into slimphp:3.x Feb 6, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

akrabat added a commit that referenced this pull request Feb 6, 2016

@akrabat

This comment has been minimized.

Show comment
Hide comment
@akrabat

akrabat Feb 6, 2016

Member

Thanks.

Member

akrabat commented Feb 6, 2016

Thanks.

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