[HttpFoundation] fixed Request::create() method #6995
Conversation
|
You should add a note in the changelog |
When creating a Request with Request::create(), some information can come from the URI and the server variable. Until now, it was not clear which information had precedence over the other and as a matter of fact, this method was not consistent. Now, information contained in the URI always take precedence over information coming from the server array. That makes sense as the server array is often copied from another existing Request object.
This PR was merged into the 2.2 branch. Commits ------- bc4a0e7 [HttpFoundation] fixed Request::create() method Discussion ---------- [HttpFoundation] fixed Request::create() method | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a When creating a Request with Request::create(), some information can come from the URI and the server variable. Until now, it was not clear which information had precedence over the other and as a matter of fact, this method was not consistent. Now, information contained in the URI always take precedence over information coming from the server array. That makes sense as the server array is often copied from another existing Request object. --------------------------------------------------------------------------- by vicb at 2013-02-07T14:42:15Z You should add a note in the changelog
| @@ -267,7 +270,7 @@ public static function createFromGlobals() | |||
| */ | |||
| public static function create($uri, $method = 'GET', $parameters = array(), $cookies = array(), $files = array(), $server = array(), $content = null) | |||
| { | |||
| $defaults = array( | |||
| $server = array_replace(array( | |||
There was a problem hiding this comment.
Why not just $server += array(...);?
There was a problem hiding this comment.
@rybakit do you have any idea on the perf impact ? (out of my own curiosity)
There was a problem hiding this comment.
@vicb, a simple benchmark - https://gist.github.com/rybakit/4737807
There was a problem hiding this comment.
Quite micro difference anyway (notice it's visible when looping more than 10000 times)...
There was a problem hiding this comment.
@rybakit, do you have XDebug enabled ? (If yes could you please re-test by disabling it).
Twice as fast is a notable difference IMO in favor of union.
There was a problem hiding this comment.
@rybakit could you please create a new issue to discuss this as the PR has been closed - you suggestion is applicable at several places in the code. Thanks.
| @@ -4,6 +4,7 @@ CHANGELOG | |||
| 2.2.0 | |||
| ----- | |||
|
|
|||
| * fixed the Request::create() precedence (URI information always take precedence now) | |||
There was a problem hiding this comment.
As a widely used and @api tagged class, this probably deserves a "[BC BREAK]"
|
This seems to change breaks the Webkit Client. When using HTTP auth and submitting a form using submit() the authorization is missing. |
|
@sstok Can you give us a small script that reproduces the problem? |
|
Will do. |
|
https://github.com/sstok/symfony-standard/tree/webkit-client-http-auth Its strange because the $crawler->filter() does not give any errors but isSuccessful() returns false. I will try the old composer.lock and see if the problem is still present. Edit: Using an older version: 6c00ffe does not have this problem. |
|
@fabpot any change you can take a look at this? OK it seems its working now. Nope, I was to optimistic. |
|
@sstok, the best would be to submit a "small" PR that reproduces the problem (ie a failing unit test that could be later integrated in Sf together with the fix) |
|
https://github.com/sstok/symfony-standard/tree/webkit-client-http-auth This is what I was able to make on short notice. I will try to make a UnitTest pull request for the Framework repo. OK that went a lot faster then simple testing code 😄 |
This PR was merged into the 2.2 branch. Commits ------- b240d1f [BrowserKit] added a test to make sure HTTP authentication is preserved when submitting a form Discussion ---------- [WIP]BrowserKit] added a test to make sure HTTP authentication is preserved | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | | License | MIT | Doc PR | Since #6995 BrowseKit no longer seems to preserve the HTTP authentication when submitting a form. This PR adds a test to demonstrate the failure. --------------------------------------------------------------------------- by vicb at 2013-02-13T12:49:16Z Thanks. Could you add a "[WIP]" prefix to the PR tittle and set "bug fix" to "no" for now ? --------------------------------------------------------------------------- by sstok at 2013-02-13T13:59:42Z done :+1: --------------------------------------------------------------------------- by fabpot at 2013-02-17T12:49:35Z This cannot be related to #6995 as your test does not involve any HttpFoundation classes.
When creating a Request with Request::create(), some information can
come from the URI and the server variable. Until now, it was not clear
which information had precedence over the other and as a matter of fact,
this method was not consistent.
Now, information contained in the URI always take precedence over
information coming from the server array. That makes sense as the server
array is often copied from another existing Request object.