-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[BrowserKit] fixed BC Break for HTTP_HOST header #26244
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
Conversation
…haviour for HTTPS server parameter
Thanks for the fix. |
Yes, this fix (if its correct) should be merged to all maintained versions. I have edited description to clarify. |
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.
I finally reviewed this PR, it makes sense to me!
this is great news! |
Thank you @brizzz. |
This PR was merged into the 2.8 branch. Discussion ---------- [BrowserKit] fixed BC Break for HTTP_HOST header | Q | A | ------------- | --- | Branch? | 2.7, 2.8, 3.x, 4.x | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes/no | Fixed tickets | #22933 | License | MIT | Doc PR | n/a The situation well described in the original issue. I will add only: - #10549 - makes server parameters to take precedence over URI. - #16265 - partially revererts #10549. Makes server parameters do not affect URI. But this is only true for `Client::request()`. It is still possible to set host for URI by `Client::setServerParameters()` when URI is realative (see examples below). I propose a compromise solution: add to HTTP_HOST header power to override URI when it is relative. Proposed solution: - if the request URI is relative, then use the HTTP_HOST header passed to Client::request() to generate an absolute URI - if the request URI is absolute, then ignore the HTTP_HOST header (as it now works) - do the same with HTTPS server parameter Profit: - fix BC Break - the documentation will be correct - http://symfony.com/doc/2.8/routing/hostname_pattern.html#testing-your-controllers - https://symfony.com/doc/2.8/testing.html#testing-configuration Before: ``` $client->setServerParameters(['HTTP_HOST' => 'example.com']); $client->request('GET', '/'); $this->assertEquals('http://example.com/', $client->getRequest()->getUri()); $client->request('GET', '/', [], [], ['HTTP_HOST' => 'example.com']); $this->assertEquals('http://localhost/', $client->getRequest()->getUri()); ``` Fixed (see last line): ``` $client->setServerParameters(['HTTP_HOST' => 'example.com']); $client->request('GET', '/'); $this->assertEquals('http://example.com/', $client->getRequest()->getUri()); $client->request('GET', '/', [], [], ['HTTP_HOST' => 'example.com']); $this->assertEquals('http://example.com/', $client->getRequest()->getUri()); ``` Commits ------- 8c4a594 [BrowserKit] fixed BC Break for HTTP_HOST header; implemented same behaviour for HTTPS server parameter
It broke my tests, so it is a BC-break. |
@nicolas-grekas does this PR really make sense? this PR broke flexible testing of domain specific routes complete or is there a way to do this after the PR? see #32791 |
The situation well described in the original issue. I will add only:
Client::request()
. It is still possible to set host for URI byClient::setServerParameters()
when URI is realative (see examples below).I propose a compromise solution: add to HTTP_HOST header power to override URI when it is relative.
Proposed solution:
Profit:
Before:
Fixed (see last line):