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

[HttpKernel] minor: add ability to construct with headers on http exceptions #22228

Conversation

@gsdevme
Copy link
Contributor

gsdevme commented Mar 30, 2017

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

This adds the ability to set the headers for the exception within the constructor.

With alot of the following exceptions its sometimes very useful to be able to set the headers. For example with a reverse proxy cache (Varnish) if you want to match the Retry-After with a Varnish cache header to protect the backend.

I see that setHeaders() did get added 6a1080f but that means the exception needs to be assigned to a variable and set and then thrown, it also doesn't merge with the existing header set in some of the constructors.

I've chosen to array_merge() where key/values were being set within the constructor as I think this is the most useful and 'correct' whereas setHeaders is explicit that its setting not amending or adding to.

@gsdevme gsdevme force-pushed the gsdevme:minor/add-ability-to-set-headers-on-exceptions branch from f4a21dd to c9fd589 Mar 30, 2017
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 3, 2017

All the array_merge could be replaced by a simple affectation: $headers['Foo'] = 'bar';

@gsdevme

This comment has been minimized.

Copy link
Contributor Author

gsdevme commented Apr 7, 2017

@nicolas-grekas Thanks! Good spot, will update.

@gsdevme gsdevme force-pushed the gsdevme:minor/add-ability-to-set-headers-on-exceptions branch from c9fd589 to 098006f Apr 7, 2017
@Nek-
Nek- approved these changes Apr 9, 2017
Copy link
Contributor

Nek- left a comment

LGTM

{
$headers = array('WWW-Authenticate' => $challenge);
$headers = array_merge($headers, array('WWW-Authenticate' => $challenge));

This comment has been minimized.

Copy link
@stloyd

stloyd Apr 9, 2017

Contributor

Why merge here & not direct set as in other places?

if ($retryAfter) {
$headers = array('Retry-After' => $retryAfter);
$headers = array_merge($headers, array('Retry-After' => $retryAfter));

This comment has been minimized.

Copy link
@stloyd

stloyd Apr 9, 2017

Contributor

Again, why merge?

@gsdevme

This comment has been minimized.

Copy link
Contributor Author

gsdevme commented Apr 9, 2017

An oversight, will fix the others up also.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 11, 2017

Status: needs work

@gsdevme gsdevme force-pushed the gsdevme:minor/add-ability-to-set-headers-on-exceptions branch from 098006f to 612fb59 Apr 15, 2017
@gsdevme

This comment has been minimized.

Copy link
Contributor Author

gsdevme commented Apr 20, 2017

Updated.

@xabbuh
xabbuh approved these changes Oct 8, 2017
@fabpot
fabpot approved these changes Oct 8, 2017
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Oct 8, 2017

Thank you @gsdevme.

@fabpot fabpot merged commit 612fb59 into symfony:master Oct 8, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
fabpot added a commit that referenced this pull request Oct 8, 2017
…ers on http exceptions (gsdevme)

This PR was merged into the 4.0-dev branch.

Discussion
----------

[HttpKernel] minor: add ability to construct with headers on http exceptions

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

This adds the ability to set the headers for the exception within the constructor.

With alot of the following exceptions its sometimes very useful to be able to set the headers. For example with a reverse proxy cache (Varnish) if you want to match the `Retry-After` with a Varnish cache header to protect the backend.

I see that `setHeaders()` did get added 6a1080f but that means the exception needs to be assigned to a variable and set and then thrown, it also doesn't merge with the existing header set in some of the constructors.

~~I've chosen to `array_merge()` where key/values~~ were being set within the constructor as I think this is the most useful and 'correct' whereas `setHeaders` is explicit that its setting not amending or adding to.

Commits
-------

612fb59 [HttpKernel] minor: add ability to construct with headers on http exceptions
@fabpot fabpot mentioned this pull request Oct 19, 2017
@gsdevme

This comment has been minimized.

Copy link
Contributor Author

gsdevme commented Mar 30, 2018

For future reference although this has a milestone of 3.4, its only in 4.0+

@gsdevme gsdevme deleted the gsdevme:minor/add-ability-to-set-headers-on-exceptions branch Mar 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.