[HttpKernel] Inline ValidateRequestListener logic into HttpKernel #19217
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cc5b5e6 to
951a203
Compare
| @@ -18,7 +18,7 @@ | |||
| "require": { | |||
| "php": ">=5.3.9", | |||
| "symfony/event-dispatcher": "~2.6,>=2.6.7", | |||
| "symfony/http-foundation": "~2.7,>=2.7.15", | |||
| "symfony/http-foundation": "~2.7,>=2.7.15|~2.8,>=2.8.8", | |||
There was a problem hiding this comment.
this will need to be "~2.8,>=2.8.8|~3.0,>=3.0.8|~3.1,>=3.1.2", when merging into 3.0
|
Ready |
|
ping @magnusnordlander for info |
951a203 to
08bb55f
Compare
| @@ -113,6 +115,14 @@ public function terminateWithException(\Exception $exception) | |||
| */ | |||
| private function handleRaw(Request $request, $type = self::MASTER_REQUEST) | |||
| { | |||
| if (self::MASTER_REQUEST === $type) { | |||
| try { | |||
| // This will throw an exception if the headers are inconsistent. | |||
There was a problem hiding this comment.
Maybe this comment is superfluous because the code is short and the exception message is very clear?
962e1f8 to
0691a22
Compare
| @@ -18,7 +18,7 @@ | |||
| "require": { | |||
| "php": ">=5.3.9", | |||
| "symfony/event-dispatcher": "~2.6,>=2.6.7", | |||
| "symfony/http-foundation": "~2.7,>=2.7.15", | |||
| "symfony/http-foundation": "~2.7.15|~2.8,>=2.8.8", | |||
There was a problem hiding this comment.
"~2.8.8|~3.0.8|~3.1,>=3.1.2" on 3.0
0691a22 to
9d3ae85
Compare
| @@ -18,7 +18,7 @@ | |||
| "require": { | |||
| "php": ">=5.3.9", | |||
| "symfony/event-dispatcher": "~2.6,>=2.6.7", | |||
| "symfony/http-foundation": "~2.7,>=2.7.15", | |||
| "symfony/http-foundation": "~2.7.15|~2.8.8", | |||
There was a problem hiding this comment.
"~2.8.8|~3.0.8|~3.1.2|~3.2" on 3.0 (really this time :) )
|
No objections from me :) |
nicolas-grekas
added a commit
that referenced
this pull request
Jun 29, 2016
…pKernel (nicolas-grekas) This PR was merged into the 2.7 branch. Discussion ---------- [HttpKernel] Inline ValidateRequestListener logic into HttpKernel | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #18688 #19216 | License | MIT | Doc PR | - I propose to inline the listener introduced in #18688 into HttpKernel. Commits ------- 9d3ae85 [HttpKernel] Inline ValidateRequestListener logic into HttpKernel
|
Why? I'm 👎 for this, that's ugly as hell! |
|
I agree that it's uglier. It does have an upside though, and that is that any project using HttpKernel will automatically get the client IP exceptions early, even if they don't use the listener in FrameworkBundle. When I implemented #18688 I considered an approach like this for that very reason. For me, the ugliness tipped the scale towards a listener though, but both approaches have merit. |
|
See #19233 |
fabpot
added a commit
that referenced
this pull request
Jun 30, 2016
…catch block (magnusnordlander, nicolas-grekas) This PR was merged into the 2.7 branch. Discussion ---------- [HttpKernel] Move handling of conflicting origin IPs to catch block | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19217 | License | MIT | Doc PR | - Commits ------- db84101 [HttpKernel] Add listener that checks when request has both Forwarded and X-Forwarded-For 1f00b55 [HttpKernel] Move conflicting origin IPs handling to catch block
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I propose to inline the listener introduced in #18688 into HttpKernel.