Skip to content

Status code from error responses #34731

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.

Sign up for GitHub

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

Jump to bottom
Closed
yceruto opened this issue Nov 30, 2019 · 1 comment
Closed

Status code from error responses #34731

yceruto opened this issue Nov 30, 2019 · 1 comment
Labels
Bug FrameworkBundle Status: Needs Review TwigBundle

Comments

@yceruto
Copy link
Member

yceruto commented Nov 30, 2019 •
edited
Loading

Symfony version(s) affected: 4.4 & 5.0

Description
While working in FriendsOfSymfony/FOSRestBundle#2036 I realized that in 4.4 we've changed the status code of the error response.

from 200:

symfony/src/Symfony/Bundle/TwigBundle/Controller/ExceptionController.php

Line 75 in 9b658ed

), 200, ['Content-Type' => $request->getMimeType($request->getRequestFormat()) ?: 'text/html']);

to $exception->getStatusCode():

symfony/src/Symfony/Component/HttpKernel/Controller/ErrorController.php

Line 43 in 9b658ed

return new Response($exception->getAsString(), $exception->getStatusCode(), $exception->getHeaders());

and I wondering what is the right approach here?

I'm not sure at this point if there is some spec/doc that talk about it.

wdyt?
@yceruto yceruto added the Bug label Nov 30, 2019
@yceruto
Copy link
Member Author

yceruto commented Dec 2, 2019

FOSRestBundle says 200:
https://github.com/FriendsOfSymfony/FOSRestBundle/blob/6c33b4d0caf2e04d875b79f9cbbb07dc16397ae2/View/ViewHandler.php#L247

Api-platform says: FlattenException::getStatusCode():
https://github.com/api-platform/core/blob/7a78de4895231907891fc77032cdf5b88f5306f5/src/Action/ExceptionAction.php#L65

/cc @xabbuh and @dunglas

@yceruto yceruto mentioned this issue Dec 5, 2019
Merged
@yceruto yceruto mentioned this issue Dec 23, 2019
Merged
yceruto added a commit that referenced this issue Dec 23, 2019
@yceruto
minor #35090 Add note about HTTP status code change (yceruto)
0bd742a 
This PR was merged into the 4.4 branch.

Discussion
----------

Add note about HTTP status code change

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #34731
| License       | MIT
| Doc PR        | -

> https://tools.ietf.org/html/rfc7807
> The "status" member, if present, is only advisory; it conveys the
   HTTP status code used for the convenience of the consumer.
   **Generators MUST use the same status code in the actual HTTP response,**
   to assure that generic HTTP software that does not understand this
   format still behaves correctly.

Commits
-------

429605b add note about HTTP status code change
@yceruto yceruto closed this as completed Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug FrameworkBundle Status: Needs Review TwigBundle
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@yceruto @carsonbot