Throw an Exception when the header is already been sent #3998
Conversation
| } | ||
| } else { | ||
| throw new Exception("Headers already sent in $filename on line $linenum\n"); | ||
| } |
There was a problem hiding this comment.
Looks like you are using spaces for indenting. CakePHP uses tabs. I can fix that up if you get stuck on it.
There was a problem hiding this comment.
Please help. I don't know what I am supposed to do? do I need to create a new pull request if Travis fails?
There was a problem hiding this comment.
@xinquanatcolezhu No. Just make the required change and make a new commit locally. Then push your updates to your github branch and this pull request will be automatically updated.
Please check the github help if you need more details.
| @@ -517,15 +517,20 @@ protected function _setContentLength() { | |||
| * @param string $name the header name | |||
| * @param string $value the header value | |||
| * @return void | |||
| * @throws CakeException When an header already been sent | |||
There was a problem hiding this comment.
"When a header has already been sent"
|
I am just a bit curious: In the discussion, it's mentioned that @xinquanatcolezhu is new to CakePHP. This raises a question: Is he doing something wrong? It sounds like he's using echo's and print's, when he should let CakePHP do the rendering. Also: The exception should be thrown no matter what. Don't bother checking "if (Configure::read('debug'))": when debug is 0, nothing will be logged or displayed. The ExceptionHandler will do the necessary debug checks. And as a last tip, I'd prefer: if (headers_sent()) {
// throw here
}
// send headers hereThere's no need for an |
|
@davidsteinsland Good point, I can clean this up and get it merged in as I see the additional errors being useful for new developers. |
|
@davidsteinsland "Bail early" is exactly what I mentioned in #3998 (comment) ;) Sounds good. We can probably throw the exception even with debug 0 as right now it would be a no-op either way. |
* Fix the various PHPCS errors. * Make the logic less complex.
|
Closing in favor of #4106 |
Please see the details of the discussion at issue #3990