That case is a bit more complicated as at least 305 status should have special handling. 300 and 304 might also deserve more attention. Also, the case with 3xx status and no location header should be handled IMHO.

@weltling I don't think anything needs to be done if a location header isn't present for a 3xx status as the expected behaviour re the specs in that case is that you get the page body and handle the 3xx in your own application. We could throw an error of some variety, but other parts of fopen don't in similar circumstances.

You could argue the same for 300, Wikipedia suggests 305 may not even be handled by browsers:
http://en.wikipedia.org/wiki/List_of_HTTP_status_codes#3xx_Redirection

I'm unsure about how to handle 304.

Ok, that's right, they always say SHOULD have location. So if there is none, userspace will get the original response (no redirection).

Regarding 300 - according to the specs it's completely valid to just pick the first best location. Where 305 (in the light of what they say about security) is a specific case and shouldn't be redirected as the location points to the proxy, not to any http destination.

304 as i get it is about a request made using if-modified-since and/or e-tag headers. The response even isn't supposed to contain the body. In fact here is nothing about redirection.

I'd say it'd be rather make sense to check against a white list, excluding 305 and 304. That are effective 300, 301, 302, 303 and 307 statuses. It'd be also more efficient to move the response code check to the start of the conditions, so it even don't gets to the php_stream_context_get_option() if there is no redirection.

@weltling 👍 aye that makes sense, I'll make up a patch later.

@1stvamp ping

@lstrojny pong, and patched.

@weltling can you verify again?

@lstrojny @1stvamp yep, imho that should be

(response_code >= 300 && response_code < 304 || 307 == response_code)

then it were complete (see my prev comment)

Ah good point, forgot about 307 when I was trying to come up with a good test.
Will fix now.

Wes

On Tuesday, 15 January 2013 at 9:27 AM, Anatoliy Belsky wrote:

|| 307 == response_code

Done.

ping @weltling @lstrojny

Wes

On Tuesday, 15 January 2013 at 9:27 AM, Anatoliy Belsky wrote:

|| 307 == response_code

@1stvamp ups, that has to be literally in the brackets

if ((response_code >= 300 && response_code < 304 || 307 == response_code) && ......)

otherwise it doesn't what you want

@weltling doh, just me being tired and stupid, not had enough coffee yet. :)

Wes

On Tuesday, 15 January 2013 at 9:27 AM, Anatoliy Belsky wrote:

@lstrojny (https://github.com/lstrojny) @1stvamp (https://github.com/1stvamp) yep, imho that should be
(response_code >= 300 && response_code < 304 || 307 == response_code)
then it were complete (see my prev comment)

—
Reply to this email directly or view it on GitHub (#236 (comment)).

@1stvamp yep, now it's atomic
Ah, just another small thing - to be compatible with C90 the response_code declaration has to be moved to the top of the block. In this case to the top of the function, as far as i can see.

@weltling done :)

Wes

On Tuesday, 15 January 2013 at 9:59 AM, Wes Mason wrote:

@weltling doh, just me being tired and stupid, not had enough coffee yet. :)

Wes

On Tuesday, 15 January 2013 at 9:27 AM, Anatoliy Belsky wrote:

@lstrojny (https://github.com/lstrojny) @1stvamp (https://github.com/1stvamp) yep, imho that should be
(response_code >= 300 && response_code < 304 || 307 == response_code)
then it were complete (see my prev comment)

—
Reply to this email directly or view it on GitHub (#236 (comment)).

@1stvamp @lstrojny fine now for all i care :)

Comment on behalf of stas at php.net:

merged