Warn on http_response_code() after header('HTTP/...') #18962
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 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. |
iluuu1994
changed the title
|
Ok. Adjusted. |
|
Does this look ok then? |
ext/standard/head.c
Outdated
| 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; |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think we have got quite clear rules when it comes to deprecations and BC breaks in major version. They should all go through the RFC because there is a BC impact on users. I don't know what the problem is about extending depracation RFC (except missing the deadline) for this if you intent to do such breaking change in the next major? There are bunch of other deprecations that are pretty clear (and much less impactful than this) but they are still being there so everyone can consider the impact and vote on them.
And I think we agreed that warning is a good thing and does not require RFC. The part that requires RFC is doing major version change so if you added just warning, I would be fine with that (even though I would probably go for NOTICE). Well you can even add warning and then add it to deprecation RFC and keep the warning as we had similar such cases. I suggested deprecation because it's the usual way for the major version changes but I won't object if you prefer warning.
There was a problem hiding this comment.
Apologies, my response was somewhat charged. But I stand by the conclusion.
I think we have got quite clear rules when it comes to deprecations and BC breaks in major version.
Sure, but this isn't just a deprecation of documented behavior. IMO it's not even questionable behavior, it simply seems like a bug. Yes, according to Hyrum's law all observable behaviors are part of your contract. However, I don't understand why this bug is different from all the other bugs we fix, often in patches and completely absent of a BC notice. I legitimately can't see many people setting status lines with header(), then calling http_response_code() and expecting the call to not take effect. I can see the opposite though.
In the past, we've handled similar bug fixes like I did here initially: If the bug is well-known, very old or just has a non-trivial impact, delay the fix to master so that it can be documented in the upgrading guide and checked by adopters accordingly. An RFC + multi-year deprecation + actual fix just seems completely overblown to me.
Trying to apply this logic to a different domain: Imagine if sort() on an array would not work if the same array was previously rsort()ed. Sure, fixing this behavior has a theoretical break for people who have come to depend on sort() not actually working, but it's much more likely that the intended code just misbehaves. If this bug were very old, postponing the fix to master seems prudent, as I tried here.
Anyway, It might make sense to raise this as an example on the mailing so we can decide on what to do with similar issues in the future.
There was a problem hiding this comment.
The main difference here is that this is currently silent behavior that does not break the code. As I said it's more set of priorities rather than something failing. That's why I think it really needs to emit notice or warning so users can fix their code rather than trying to figure out why the status code suddenly changed. And that's really the fix IMO.
What is proposed for 9.0 is not a fix but change of behavior IMHO - that's why I think that part needs to have an RFC. I don't think that part is any different that bunch of other deprecations that we have.
There was a problem hiding this comment.
I kind repeat myself but just to clarify. My primary objection was against changing this behaviour without reasonable way to notify the user. Basically silent change of response code in this case. This is not really that much about the actual BC break but about how this make user aware about the potential problem. You can take it as objection against the fix (which I can see in engine too btw - e.g. Dmitry often reject some solution if he sees problem in them).
If there are other cases where we would allow changing something that fundamental as response code without any notification, then I think I would object against them too. This is kind of subjective because it differs on case by case basis. So I'm not suggesting to apply this on every single bug. That's kind of why every developers are free to require RFC if they don't agree on the solution and don't find another solution that is acceptable for both.
In this case you agreed to change it to warning which I see as a fix on its own because the contract is that http_response_code() cannot be called after header() with response code line sent and we warn users if they do so. So this should resolve the bug because we found some sort of agreement.
But then there is that 9.0 change which is no longer a bug fix but a typical sort of change that goes through the deprecation RFC.
That's how I see this. :)
There was a problem hiding this comment.
I kind repeat myself but just to clarify.
Yes, we are going in circles indeed. I still don't agree with your representation of my argument:
My primary objection was against changing this behaviour without reasonable way to notify the user.
This goes for most bug fixes. 1 Why is this different? If the answer is impact, I would highly question that. There are at least a handful of unlikely conditions that must apply.
- The code itself must call
header()and thenhttp_response_code(), which is already unlikely given nobody reported this issue. - Not only must the code not be broken due to
http_response_code()not taking effect, it must actually be broken if it does take effect. - The user must miss the issue when reading the upgrade guide for the next major version.
Also: Warnings suck. They are only triggered when the relevant code path is reached, which makes them hard to discover. http_response_code() especially seems like something that would be used in untested error paths, silently not behaving the way you'd expect. Sometimes warnings are a necessary evil for things that cannot be caught at compile time or solved otherwise, but this issue can be solved.
That's kind of why every developers are free to require RFC if they don't agree on the solution and don't find another solution that is acceptable for both.
Sure, that's fair. But you asked for an RFC even with a warning, with delaying this to 9.0, and even if nobody from the mailing list objected. Do you expect anybody else to vote against this? Insisting on an RFC only makes sense when there's reason to believe there could be edge cases that need more consideration, or when there are possible objections.
I think I've said everything I wanted to. I'll read your response if you have one but I won't respond further.
Footnotes
There was a problem hiding this comment.
As I said initially this can be unintended change deep in the code so header just taking precedence. I would expect this sort of thing in huge monoliths (e.g. some 3rd party WP plugins or similar) where things just work because this is ignored. After this change, the response code would suddenly change and there would no way to figure out what actually happened without reading UPGRADING in detail. That sort of situations are the ones that I'm worried about.
I agree that warning suck but it's better than silent change IMHO.
I think I explained it but I will repeat it. I asked for the RFC with 9.0 BC break because it was additional change (not fix) that changes warning to a different behavior. I'm happy to add it to the 8.6 deprecation RFC myself. I don't really understand why it's such a big issue to just add it there as we do exactly the same for all such cases. This is just a normal process that we do and I don't remember any such change being done without RFC.
I just pushed updated version with warning only which is pretty much that you did without mentioning 9.0 which I don't think is necessary even if we decided to change it in 9.0.
There was a problem hiding this comment.
Why not at least keep the commented code?
There was a problem hiding this comment.
Ok comment with code and slightly modified wording restored.
Fixes phpGH-18582 Fixes #81451 Co-authored-by: Jakub Zelenka <[email protected]>
Fixes GH-18582
Fixes #81451