Opened 2 years ago
Last modified 3 months ago
#16889 reopened defect
Having a location header does not mean that there should be a redirection.
| Reported by: |
|
Owned by: | |
|---|---|---|---|
| Priority: | normal | Milestone: | 3.6 |
| Component: | HTTP | Version: | 3.1 |
| Severity: | normal | Keywords: | has-patch |
| Cc: | Viper007Bond, mike@… |
Attachments (3)
Change History (17)
Out of scope of this ticket but related in the code: It fails if the response contains more than one location header line. Just running over it.
Related: #16890 - a patch for that problem is available there.
curl_getinfo might not return string - user comment in http://www.php.net/manual/en/function.curl-getinfo.php
And there was an additional error in the if clause which prevented it from working.
- Keywords close added
Unfortunately, I don't think it's going to be possible to adhere to this specification.
cURL(CURLOPT_FOLLOWLOCATION), HTTP Extension, and Streams (therefor PHP Internally) all follow redirects on 200 (ie. non-3xx) responses, this seems to be done (From what I can understand) for compatibility reasons with older web servers (unsure of which ones specifically)
It's probably best to standardise on following location regardless of the response code as we're currently doing.
- Keywords close removed
- Milestone Awaiting Review deleted
- Resolution set to invalid
- Status changed from new to closed
based on my previous comment, and further research into if it's possible, I'm closing as invalid. It's simply not possible to adhere to it whilst relying on any current-generation HTTP API offered by PHP.
Replying to dd32:
Unfortunately, I don't think it's going to be possible to adhere to this specification.
cURL(CURLOPT_FOLLOWLOCATION), HTTP Extension, and Streams (therefor PHP Internally) all follow redirects on 200 (ie. non-3xx) responses, this seems to be done (From what I can understand) for compatibility reasons with older web servers (unsure of which ones specifically)
Most certainly by 6.2 Response Header Fields and 14.30 Location.
It's probably best to standardise on following location regardless of the response code as we're currently doing.
I now think so as well now. Thanks for your support so far.
- Milestone set to Awaiting Review
- Resolution invalid deleted
- Status changed from closed to reopened
I'm re-opening this to look into it again, hopefully things have changed enough, or at least changed that we can partially fix this.
In 3.4 the way Curl follows redirects has changed, it's moved from internal in curl, to being handled in PHP by us to make it more straight forward.
In addition, it's been pointed out that following Location headers causes problems for 201 (Created) requests, since WP_HTTP blindly follows the location, the result from the Created command is lost.
An example of a request which this fouls up is this the following Google Contacts API request (note, this output is taken directly from the curl command line binary)
> POST /m8/feeds/contacts/default/full/?access_token=...... > HTTP/1.1 > User-Agent: curl/7.21.4 (universal-apple-darwin11.0) libcurl/7.21.4 OpenSSL/0.9.8r zlib/1.2.5 > Host: www.google.com > Accept: */* > Content-Type: application/atom+xml > Content-Length: 465 > < HTTP/1.1 201 Created < Content-Type: application/atom+xml; charset=UTF-8 < Expires: Sat, 05 May 2012 01:36:29 GMT < Date: Sat, 05 May 2012 01:36:29 GMT < Cache-Control: private, max-age=0, must-revalidate, no-transform < Vary: Accept, X-GData-Authorization, GData-Version < GData-Version: 1.0 < Location: https://www.google.com/m8/feeds/contacts/user%40gmail.com/full/123/345 < Content-Location: https://www.google.com/m8/feeds/contacts/user%40gmail.com/full/123/345 < X-Content-Type-Options: nosniff < X-Frame-Options: SAMEORIGIN < X-XSS-Protection: 1; mode=block < Server: GSE < Transfer-Encoding: chunked <?xml version='1.0' encoding='UTF-8'?> ...
If WP_HTTP makes the request it'll follow the Location header and the client can't retrieve the XML payload.
One can pass 'redirection' => 0 to the request and it "just works" (at least in the case of the curl transport) so hopefully that's a sign we can properly fix this now
comment:9
Viper007Bond — 13 months ago
- Cc viper007bond added
comment:10
Viper007Bond — 13 months ago
- Cc Viper007Bond added; viper007bond removed
comment:11
mbijon — 12 months ago
- Cc mike@… added
comment:12
dd32 — 3 months ago
In 1231/tests:
comment:13
dd32 — 3 months ago
- Keywords has-patch added
- Milestone changed from Awaiting Review to 3.6
16889.diff is a patch which worksforme, and passes the unit tests (once I corrected the unit tests from attempting a 200 response (which PHP declines to send) to a 201) - Thats the first time in a long time, that I've seen 100% of the HTTP unit tests pass ;)
comment:14
Viper007Bond — 3 months ago
Worth having a filter in there?

First digit counts