Skip to content

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

Closed
wants to merge 2 commits into from

Conversation

iluuu1994
Copy link
Member

Fixes GH-18582
Fixes #81451

@iluuu1994
Copy link
Member Author

@bukka Ping. 🙂

@bukka
Copy link
Member

bukka commented Jul 21, 2025

This is a BC break so you will need RFC for this.

@bukka
Copy link
Member

bukka commented Jul 21, 2025

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.

@bukka
Copy link
Member

bukka commented Jul 21, 2025

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.

@iluuu1994
Copy link
Member Author

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

  1. https://www.php.net/manual/en/function.http-response-code.php

@bukka
Copy link
Member

bukka commented Jul 21, 2025

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.

@iluuu1994
Copy link
Member Author

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.

@bukka
Copy link
Member

bukka commented Jul 22, 2025

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.

@iluuu1994 iluuu1994 changed the title Allow http_response_code() to clear HTTP start-line Warn on http_response_code() after header('HTTP/...') Jul 22, 2025
@iluuu1994
Copy link
Member Author

Ok. Adjusted.

@iluuu1994
Copy link
Member Author

Does this look ok then?

Comment on lines +380 to +383
php_error_docref(NULL, E_WARNING, "Calling http_response_code() after header('HTTP/...') has no effect. This will change in PHP 9.0");
// TODO: Uncomment in PHP 9.0
// efree(SG(sapi_headers).http_status_line);
// SG(sapi_headers).http_status_line = NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be deprecation if it should change in 9.0 and it would also require RFC because the upcoming BC break. I think I would just add NOTICE for now and then leave it to anyone who want to do the RFC...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your stance on this issue is seriously baffling to me. Almost every bug fix has some risk of breakage. That's why we delay more risky fixes to master, including myself in this case. If that's not enough, delaying to the next major with a migration path should certainly be enough. I even offered a mailing list discussion. But if the fixing of obvious issues now requires RFCs, I don't know how we can get anything done. This is not only wasteful to the RFC author, but everyone who has to read it.

That should be deprecation

I made this a warning because such code is almost guaranteed to be misbehaving, rather than this being a feature that is being removed.

@iluuu1994 iluuu1994 closed this Jul 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http_response_code() does not override the status code generated by header()
2 participants