-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Warn on http_response_code() after header('HTTP/...') #18962
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
@bukka Ping. 🙂
This is a BC break so you will need RFC for this.
I think it would be good to sync this behaviour but there is some risk as it's been there for long - if I understand correctly, this is also the case for FPM - just Apache does not do that.
Basically the implementation can currently rely on the fact that header has got the priority and this might suddenly change the behaviour. You could argue that this is a fix but in this case it's not like the things do not work. It's more about the priority.
IMO, this doesn't seem intentional, and nothing in the documentation 1 indicates http_response_code() wouldn't work after using header(). I understand this can have BC breaks, which is why I targeted master instead of PHP-8.3. I can shoot a quick e-mail to internals@ if you like.
Footnotes
Yeah this for sure not intentional but it doesn't mean that some code does not rely on it. It might be even relying on it accidentally and it will be extremely hard to figure out why the behavior suddenly changed.
If it was just CLI server, then I would mind changing it but if it's in FPM as well, then it might impact production code so in such case I'm against this silent change. I would much prefer warning approach in such case.
Would you object to this even if internals does not? I'm not particularly keen on a multi-year endeavor to fix this. In that case, I'd leave this up to somebody else.
Yeah I don't think this is the right way to fix it. I'd prefer going the warning path as I mentioned in the actual issue.
624a83c to
fc15db4
Compare
Ok. Adjusted.
a8495eb to
8a9bdcf
Compare
Does this look ok then?
Fixes phpGH-18582 Fixes #81451 Co-authored-by: Jakub Zelenka <bukka@php.net>
@php/release-managers-85 are you cool with this?
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.
RM approval, no technical review performed
The wording might be slightly misleading, the gateway specification being used in most SAPI implementations is CGI, in CGI you can send Status: rather than HTTP. It's probably better to choose wording that covers both cases.
Fixes GH-18582
Fixes #81451