Fixes #15046 #15390
Fixes #15046 #15390
Conversation
dmirogin
commented
Dec 19, 2017
dmirogin
commented
| Q | A |
|---|---|
| Is bugfix? | no |
| New feature? | yes |
| Breaks BC? | I think worth mentioning this in upgrade |
| Tests pass? | yes |
| Fixed issues | #15046 |
| * @param int $line | ||
| * @return HeadersAlreadySentException | ||
| */ | ||
| public static function make($file, $line) |
There was a problem hiding this comment.
I dont want to change constructor params.
There was a problem hiding this comment.
Just prefer this style. But in this case it's redundant.
|
Thank you for putting effort in the improvement of the Yii framework. In order for the framework and your solution to remain stable in the future, we have a unit test requirement in place. Therefore we can only accept your pull request if it is covered by unit tests. Could you add these please? Thanks! P.S. If you have any questions about the creation of unit tests? Don't hesitate to ask for support. More information about unit tests This is an automated comment, triggered by adding the label |
|
I don't know how to test it, because cli SAPI doesn't support headers access. |
|
Right. Let's skip it this time. |
|
Merged. Thanks! |
| if (headers_sent()) { | ||
| return; | ||
| if (headers_sent($file, $line)) { | ||
| throw new HeadersAlreadySentException($file, $line); |
There was a problem hiding this comment.
This is a serious BC break - disallows setting flexibly custom headers anywhere in the code, e.g. when downloading files. Should be reverted to provide more flexibility rather than restricting & forcing users to set headers via Response object. Previous code was OK.
BTW - setting custom headers in response is also not so intuitive, since $_headers is private property (should not be public ?) and there is also not setter like setHeader(name, value) or massive setter setHeaders([]).
There was a problem hiding this comment.
You were not able to set them anyway. It was just silently ignored.
There was a problem hiding this comment.
We used to set various custom headers directly e.g.
header('Content-Type: application/octet-stream', false);
Now everybody who used to set inline headers will get Exception. With already 14th branch release there might be heaps of code to be refactored :-( It would be better to leave such a BC change for 2.1.X. I am not sure what you mean not able to set them anyway - it was possible via $response->getHeaders()->add(), right? But we did not use it coz it's sometime easier to write native PHP headers with second replace option.
There was a problem hiding this comment.
If there's now an exception than these headers weren't working well before i.e. some headers were set but not sent. That's clearly an error that should be fixed anyway. It was hidden before but it doesn't mean that it worked correctly. Quite the opposite.
