Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel] Deprecate X-Status-Code for better alternative#19822
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
b742e97 to2eae976Compare2eae976 to001c2daCompare| $response =$this->httpKernel->handle($subRequest, HttpKernelInterface::SUB_REQUEST); | ||
| if (200 ===$response->getStatusCode()) { | ||
| $response->headers->set('X-Status-Code',401); | ||
| $response->setStatusCode(401); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@fabpot I'm not sure about this change, in the authentication listeners found in theSymfony\Component\Security\Http\Firewall namespace the response returned from theAuthenticationEntryPointInterface::start() method is set on the response. From what I can see theX-Status-Code is only ever used when handling the exception in the kernel, so I don'tthink this will have any adverse impact
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@fabpot interested on hearing your thoughts on the above? would be good to get this into 3.2 before the development window closes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This looks suspicious to me. You are saying that this can basically be removed, right? If that's the case, we probably need to understand what changed between the time the PR adding this was merged and now.
001c2da to75d9d7cCompare| parent::__construct($kernel,$request,$requestType); | ||
| $this->setException($e); | ||
| $this->allowSuccessfulResponse =false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This should be set on variable declaration not in construct.
75d9d7c to703108fComparejameshalsall commentedSep 2, 2016
The build failure in AppVeyor is not look related to these changes. |
703108f tocbcc2b4Comparelemoinem commentedSep 28, 2016
@jameshalsall If you could rebase your PR so the Conflict can be resolved, I think we could switch this to Reviewed! |
cbcc2b4 to51e8614Comparejameshalsall commentedSep 28, 2016
Rebased. |
lemoinem commentedSep 28, 2016
Status: Reviewed |
| /** | ||
| * @var bool | ||
| */ | ||
| private$allowSuccessfulResponse =false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why did you choose this name? This feature is not limited to successful response codes, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good point - I used this as it was proposed in the original issue, but I'll re-name it to something more appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@xabbuh updated this now toallowCustomResponseCode
0c17566 to96a8caaComparefabpot commentedOct 24, 2016
Some notes from my research about this (I wanted to understand why it was not used more in Symfony itself):
|
fabpot commentedFeb 16, 2017
@jameshalsall Do you have time to finish this one? |
jameshalsall commentedFeb 17, 2017
@fabpot I will get to this early next week - it's completely slipped my mind |
f9ce498 to6dbf9f7Comparejameshalsall commentedFeb 20, 2017
@fabpot this is ready for re-review now |
| // the developer asked for a specific status code | ||
| if ($response->headers->has('X-Status-Code')) { | ||
| @trigger_error(sprintf('Using the X-Status-Code header is deprecated, use %s::allowCustomResponseCode() instead.', GetResponseForExceptionEvent::class),E_USER_DEPRECATED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should beis deprecated since version 3.3 and will be removed in 4.0. Use ...
| try { | ||
| $event->setResponse($this->startAuthentication($event->getRequest(),$exception)); | ||
| if (method_exists($event,'allowCustomResponseCode')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The condition should be removed andcomposer.json (both forsymfony/security andsymfony/security-http) should be updated to only allowsymfony/http-kernel 3.3+. The reason is that you are usingsetStatusCode above anyway, so the code only works for patched versions oh http-kernel.
| $subRequest->attributes->set(Security::ACCESS_DENIED_ERROR,$exception); | ||
| $event->setResponse($event->getKernel()->handle($subRequest, HttpKernelInterface::SUB_REQUEST,true)); | ||
| if (method_exists($event,'allowCustomResponseCode')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Condition should be removed
| $listener->onKernelException($event); | ||
| $this->assertEquals('error',$event->getResponse()->getContent()); | ||
| if (method_exists($event,'isAllowingCustomResponseCode')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Condition can be removed
| $listener->onKernelException($event); | ||
| $this->assertEquals('OK',$event->getResponse()->getContent()); | ||
| if (method_exists($event,'isAllowingCustomResponseCode')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
can be removed
fabpot commentedFeb 22, 2017
Also, we need to work on a PR on Silex to be sure that the code proposed here works there as well. |
6dbf9f7 to01595e0CompareThis marks the X-Status-Code header method of setting a custom responsestatus code in exception listeners as deprecated. Instead there is nowa new method on the GetResponseForExceptionEvent that allows successfulstatus codes in the response sent to the client.
01595e0 tocc0ef28Comparefabpot commentedMar 1, 2017
Thank you@jameshalsall. |
fabpot commentedMar 1, 2017
@jameshalsall Can you finish the PR for Silex? Thanks. |
…ative (jameshalsall)This PR was merged into the 3.3-dev branch.Discussion----------[HttpKernel] Deprecate X-Status-Code for better alternative| Q | A || --- | --- || Branch? | master || Bug fix? | no || New feature? | yes || BC breaks? | no || Deprecations? | yes || Tests pass? | yes || Fixed tickets |#12343 || License | MIT || Doc PR |symfony/symfony-docs#6948 |This marks the X-Status-Code header method of setting a custom response statuscode in exception listeners for a better alternative. There is now a new methodon the `GetResponseForExceptionEvent` that allows successful status codes inthe response sent to the client.The old method of setting the X-Status-Code header will now throw a deprecation warning.Instead, in your exception listener you simply call `GetResponseForExceptionEvent::allowCustomResponseCode()` which will tell the Kernel not to override the status code of the event's response object.Currenty the `X-Status-Code` header will still be removed, so as not to change the existing behaviour, but this is something we can remove in 4.0.TODO:- [x] Replace usage of X-Status-Code in `FormAuthenticationEntryPoint`- [x] Open Silex issue- [x] Rename method on the response- [x] Ensure correct response code is set in `AuthenticationEntryPointInterface` implementations- [x] Ensure the exception listeners are marking `GetResponseForExceptionEvent` as allowing a custom response code- [x] In the Security component we should only use the new method of setting a custom response code if it is available, and fall back to the `X-Status-Code` methodCommits-------cc0ef28 [HttpKernel] Deprecate X-Status-Code for better alternative
iisisrael commentedJun 28, 2017
As a result of this change, PhpUnit functional tests no longer recognize redirects on ACL exceptions. When authenticated as a user without the required role, this fails: Functional tests have to resort to confirming that the response content matches the expected redirect page (e.g. the home page) instead of the tested forbidden page's content. This can also be seen in debug mode in the Symfony profiler - the HTTP status is 200, though a Sub Request exists with an Either of these changes to or: I can't tell what the original intent was for the change in this particular instance, so I'm not sure which would be preferred. |
… handler (jameshalsall)This PR was submitted for the master branch but it was merged into the 3.3 branch instead (closes#6948).Discussion----------Update docs for setting custom response code in exception handlerDocs update for the changes applied insymfony/symfony#19822Commits-------5f0becf Update docs for setting custom response code in exception handler
…obrev)This PR was merged into the 2.3.x-dev branch.Discussion----------Allow setting custom status code on exception responseCloses#1450.Prior and related PRs and issues:-symfony/symfony#19822-symfony/symfony-docs@5f0becf-symfony/symfony-docs#9336Commits-------7103511 Allow setting custom status code on exception response
…CodeIntroduced in5f0becfThe functionality was introduced insymfony/symfony#19822.The method got renamed during code review, but was not updated in the docs.symfony/symfony#19822 (comment)
…onseCode (hkdobrev)This PR was submitted for the master branch but it was merged into the 3.4 branch instead (closes#9336).Discussion----------Rename allowSuccessfulResponse to correct allowCustomResponseCodeIntroduced in5f0becfThe functionality was introduced insymfony/symfony#19822.The method got renamed during code review, but was not updated in the docs.symfony/symfony#19822 (comment)Commits-------0cda0d5 Rename missing allowCustomResponseCode to correct allowCustomResponseCode
Uh oh!
There was an error while loading.Please reload this page.
This marks the X-Status-Code header method of setting a custom response status
code in exception listeners for a better alternative. There is now a new method
on the
GetResponseForExceptionEventthat allows successful status codes inthe response sent to the client.
The old method of setting the X-Status-Code header will now throw a deprecation warning.
Instead, in your exception listener you simply call
GetResponseForExceptionEvent::allowCustomResponseCode()which will tell the Kernel not to override the status code of the event's response object.Currenty the
X-Status-Codeheader will still be removed, so as not to change the existing behaviour, but this is something we can remove in 4.0.TODO:
FormAuthenticationEntryPointAuthenticationEntryPointInterfaceimplementationsGetResponseForExceptionEventas allowing a custom response codeX-Status-Codemethod