Edit report at http://bugs.php.net/bug.php?id=51749&edit=1
ID: 51749
Comment by: theimp at iinet dot net dot au
Reported by: theimp at iinet dot net dot au
Summary: header("Location:") changing HTTP status
Status: Open
Type: Bug
Package: HTTP related
Operating System: Debian Lenny
PHP Version: 5.3.2
New Comment:
The patch no-http-status-code-overwrite-on-location-header (last
revision 2010-05-29 20:00 UTC) is a patch to main/SAPI.c from 5.3.2 (04
Mar 2010).
The relevant changes are lines 661 to 665 (661 to 668 in the original),
and 675 to 678 (678 in the original).
I deliberately chose the simplest method I could; it simply checks to
see if the status is currently 200, and if so, then it updates the
status code as before. Otherwise, it does not.
As mentioned, the behavior that is the subject of this bug has been a
non-problem for a very long time. I am more and more inclined to agree
with mike at php dot net that "fixing" it just might not be worth it,
when compared against the possibility of failures related to
backwards-compatibility expectations. But aharvey at php dot net does
have a valid point too.
A method I considered, but rejected as too inefficient (considering the
benefit), involved creating a new variable to store the status when set,
and only write it just before the headers are sent.
Another similar method that I considered, which was more efficient, but
that I rejected for being too dangerous and fault-prone, was making the
default HTTP response value 0 (or any other invalid value) and, again,
updating it to 200 if it was still set to 0 (meaning that it hadn't been
updated) before the headers were about to be sent.
These changes involve much more significant modification for very little
additional benefit. The only case where they would improve the patch
code is if you set the HTTP code to some number, and then set the HTTP
code back to 200, and then sent a Location or similar header; you might
expect it not to change (because you've explicitly set it), but it would
change (because it's currently 200, and that's the only condition that
is checked).
(Also, the code on lines 665-666 of the original is redundant in the
current code because it would always be performed again at 752).
Please note that I am not necessarily saying that I think this patch
should be applied. In fact, after reading the source code, I think that
- if anything - my complaint really should simply be a documentation bug
(ie; that setting a Status header, last, is the preferred method of
setting the response code, and clarifications related to the correct use
of http_response_code).
(Should have read the source code first, I guess. For example, it's now
painfully clear that PHP follows RFC 3875 as closely as possible).
Thank you for your consideration.
Previous Comments:
------------------------------------------------------------------------
[2010-05-20 09:17:43] m...@php.net
Heh, you're quite patient either ;)
I think the most SAPI-portable way to set the HTTP status is:
header("Status: NNN", true, NNN);
Still, special headers like Location and WWW-Authenticate might override
it again, so it's best to set the HTTP status at last.
If you want to know more about the "hack", visit sapi_header_op() in
main/SAPI.c
Cheers
PS: patches are always welcome, even if the just cause a discussion
without following changes
------------------------------------------------------------------------
[2010-05-20 08:51:06] theimp at iinet dot net dot au
> header("HTTP/1.1 XXX") is just a hack
I did not realize this. So does that mean that, per RFC 3875, we're only
supposed to set the Status header and hope that the server does what we
expect?
The documentation specifically lists this "hack" as the correct way to
supply the Status-Line and, therefore, the Response Code. But I'm not
going to argue with you about this.
> I see no hard reason to introduce other hacks to support a hack in the
first place.
Well, that's fair enough.
> You are writing *pages*
I apologize. I tend to use far more words than I have to, in
anticipation of explaining myself poorly otherwise. I will try to be
more concise. Many of the details I felt were essential for
understanding how the fix SHOULD be handled, distinct from my personal
preferences.
> about code that's *years* old and worked that way for a long long
time, and there's very little chance to become that changed.
I understand from this, that you do not want to fix this in the way I
have suggested, which is fair enough; it doesn't seem to bother anyone
else and has trivial workarounds, and its very status as a bug is more
an matter of opinion than fact.
I'm bad at interpreting subtlety, though, and so I am not clear if I
should also conclude that:
1. This bug is closed, we've got more important things to fix, and this
is technically not even broken. Stop bothering me!
or:
2. If you want it fixed so bad, then submit a patch yourself and we'll
see if it's not too convoluted to keep.
I am not trying to be offensive, and understand that you weren't,
either.
Thank you for your patience.
------------------------------------------------------------------------
[2010-05-18 09:47:14] m...@php.net
Maybe, but header("HTTP/1.1 XXX") is just a hack and I see no hard
reason to introduce other hacks to support a hack in the first place.
You are writing *pages* about code that's *years* old and worked that
way for a long long time, and there's very little chance to become that
changed. You know, PHP is an acronym for BC, or was it the other way
round...
------------------------------------------------------------------------
[2010-05-12 14:19:48] theimp at iinet dot net dot au
@ mike at php dot net
I did more testing, and yes, if you use the additional parameters on the
occasion that you send the location header, the special handling of the
Location header does not override the explicit behavior.
So:
header("HTTP/1.1 503 Service Unavailable", true, 503);
header("Location: http://www.php.net/";);
Does not work; but:
header("HTTP/1.1 503 Service Unavailable");
header("Location: http://www.php.net/";, true, 503);
Does work.
Obvious, in hindsight. But very confusing at first. The documentation
for http_response_code simply says: "Forces the HTTP response code to
the specified value"; I read that as forcing the response code
irrespective of any other later action other than another
http_response_code. It's not quite a documentation bug, since it's not
strictly wrong, but it should probably be changed, because it is easy to
read other than as intended. I would accept changing this bug to a
documentation bug.
@ aharvey at php dot net
The functionality I expected exists; I simply did not understand it. But
I do agree with what you say also; it is questionable whether it should
behave the way that it does even aside from other functionality.
To really know how this should be treated requires, I think,
consideration of the points I have previously mentioned. Presumably,
this specific behavior was put into PHP for a reason, and it was not
changed much when the opportunity last arose. I do not know the specific
goals of the PHP project in this respect.
I would not have written this bug report if I had realized the correct
way to use the http_response_code parameter.
Also, the workaround mentioned in bug #25044 is still possible.
Finally, I had not properly considered RFC 3875 when I first created
this bug report. If I had, I would probably have decided that the
behavior is deliberate and was not expected to be fixed.
The http_response_code is confusing, since it can be set on unrelated
headers, making it difficult to track down the code that sets it since
it could be a place other than where you set the Response Line itself
(in principle, any header; practically, any Location header in addition
to the Response Line header).
Also, the HTTP Response Code that you want to set must be known at the
time you want to set the Location header, which might not be known at
that time. It might have already been set in another function; there is
no way to retrieve the response code that you have set, so you have to
remember it yourself by assigning it to a variable and re-using that
variable at the time you set the Location header.
For example:
...
if ($_SERVER["HTTPS"] != "on") {
setstatus(426);
setlocation("https");
}
...
function setstatus($status)
{
switch ($status)
{
...
case 426:
header("HTTP/1.1 426 Upgrade Required");
break;
...
}
}
...
function setlocation($scheme)
{
switch ($scheme)
{
...
case "https":
header('Location: $scheme://$url');
break;
...
}
}
A better solution may have been, rather than to add the
http_response_code parameter, to have added a force_response() function
to optionally set the HTTP Response Code, which would not be
overwritten. But we are long past the point that it makes sense to add
such a function; it would add no new functionality and would deprecate
existing uses of unrelated code.
While I do think that PHP should not set the Response Code after a
Location header, there are still reasons to consider this behavior
appropriate (standards compliance and backwards-compatibility), which I
have already discussed at length.
On the other hand; fixing this "bug" in a way similar to the one I
suggested would make PHP more robust and make it much more likely that
people get the behavior that they expect after they read all of the
relevant documentation. It may also help with bug-finding in the case of
incorrect header output, and simplify some functions, depending on how
they have been designed.
Thank you all for taking the time to consider this bug.
------------------------------------------------------------------------
[2010-05-12 09:49:04] ahar...@php.net
That's not actually the point of the bug: the point is that if you've
already changed the HTTP status code, you probably don't want a Location
header silently changing it later to a 302.
I do think the bug's a valid one, FWIW.
------------------------------------------------------------------------
The remainder of the comments for this report are too long. To view
the rest of the comments, please view the bug report online at
http://bugs.php.net/bug.php?id=51749
--
Edit this bug report at http://bugs.php.net/bug.php?id=51749&edit=1
