Small fix for CHttpRequest to send valid content-length and output

[Ragazzo]

For more info: issue #1279 and #1249

[cebe]

This reverts the fix for #1249. Can you explain how it solves #1279? They are also different issues that need separate handling, so what you are doing here is not the way to go.

Btw: use tab indentation, not spaces.

[cebe]

Seems you changed something while I was commenting, but still not okay.

[Ragazzo]

As u can see in issue 1279 i was solving problem with valid content output like in issue 1249, but also in current version ob_get_length() will alwayse be false because of php.ini settings (buffer size), so i think this PR can solve this 2 problems, why do u think they are different? did u see 1279?
yes, did some changes, first PR in github, what is wrong with it on your thought?

[cebe]

did you understand what #1249 is about? it kills output genereated by Yii::app()->end(). So the code has to stay there.
They are definitively not the same issues.

[Ragazzo]

Hm, yes, was wrong, so, maybe then leave ob_* near Yii::app()->end() and delete if ob_get_length() because this brokes valid content-length(no header in request) ? I was getting error with content when use some debug toolbar, so it was not using onEndRequest(just dumped data after all render) thats why i could not understand what u were talking about, now got it. What do u think about to remove the if check buffer length row?

[Ragazzo]

@cebe made new PR with fixes from #1249 and no "if", is it ok?

[cebe]

You are still replacing CHANGELOG line, please add a new one for your issue.
Will check your fix later.

[Ragazzo]

@cebe done, is it ok?

[Ragazzo]

@cebe can u merge it, so i can use it in current master branch?

[cebe]

@Ragazzo will merge it when I find time to check it.

[samdark]

Better to descibe it like CHttpRequest::sendFile() now always sends valid content-header.

[Ragazzo]

@samdark now ok. hm, is it wrong that "push" to my "repo" was displayed here, i am talking about "b80f229"?

[samdark]

It's expected. The branch you've pull-requested is tracked by github so when you'll do any changes these will be in the pull request automatically.

[cebe]

@samdark just seen that you added the line removed here in b029edc do you remember any details about the problem? Imo it should always send the Content-Length-header independent of output buffering, so I'd merge this PR but would be cool to know what problem was with output buffering to not re-introduce it now.

[cebe]

Oh, original code comes from 54a24af#L8L551 I am afraid we can't easily find the reason for the existence of this line.
@qiangxue @mdomba any idea?

[cebe]

Sorry for posting that much. Found the original issue: http://code.google.com/p/yii/issues/detail?id=788
Will re-check it.

[cebe]

I checked this code with the following actions:

    public function actionDownload()
    {
        Yii::app()->request->sendFile('testfile.txt', str_repeat('aslkdufzas87dfzaosdlfhjaosdiufza', 32)); // file size 1024
    }

    public function actionDownload()
    {
        ini_set("zlib.output_compression", "On");
        Yii::app()->request->sendFile('testfile.txt', str_repeat('aslkdufzas87dfzaosdlfhjaosdiufza', 32)); // file size 1024
    }

    public function actionDownload()
    {
        ob_start("ob_gzhandler");
        Yii::app()->request->sendFile('testfile.txt', str_repeat('aslkdufzas87dfzaosdlfhjaosdiufza', 32)); // file size 1024
    }

Checked with PHP 5.3.15 Chrome, Firefox, Opera, IE and Safari all working fine.
So I can't see a reason why if(ob_get_length()===false) should stay. We might need to check it for PHP 5.2.x, but I currently don't have that version available here.

[samdark]

@cebe this is to make sure all buffers are already flushed and no buffering is active. I think to see why it's there you have to either turn on output buffering in php.ini or set your custom handler for it.

[cebe]

@samdark I did the test with output_buffering On and the last action registers the ob_gzhandler which caused no problems with the Content-Length-Header. I can't reproduce the problem that made if(ob_get_length()===false) necessary with php 5.3. Maybe it only exists in php 5.2? would be cool if someone could check that.

[samdark]

Check #4

[cebe]

I've seen that and what I can tell is that the Content-Length header is fine even when and output handler is enabled. At least for php 5.3

[samdark]

Hmm, there should be a reason. @qiangxue http://code.google.com/p/yii/source/detail?r=1822 seems to be your fix. Can you remember how to reproduce it?

[mdomba]

Here is an idea http://stackoverflow.com/questions/815961/how-to-determine-the-content-length-of-a-gzipped-file

[cebe]

@qiangxue can you comment on this? #1523 (comment) I'd like to have this fixed in 1.1.13

[qiangxue]

I think it's because when there's output_handler, it is possible the output is compressed, and as a result the content length may not be the actual one.

[cebe]

As I said in this comment: #1523 (comment) I tested all common output handlers and they all give the correct output length.

[samdark]

If I remember, it was a kind of combination of ob_gz_handler set in php.ini and deflate. Something like https://bugs.php.net/bug.php?id=51898 but not quite so.

[samdark]

Anyway, if we can't figure out and it works currently with all possible combinations, I think we can fix not sending content-length in general and deal with consequences if there will be any during RC time.

[cebe]

ob_gz_handler and deflate isn't a combination that should be used in webserver config, so I think we should not add workarounds for it.
So I think we can merge this.

[samdark]

Agree. Better of two evils.

[cebe]

Thanks for working on this @Ragazzo!

[Ragazzo]

@cebe hm, thank you and other core-developers for working on this one, more work was made by them, when discussing this solution. Anyway will do on weekend correct PR about "range-header" that was mentioned here, just learning the w3c docs about correct behavior of handling this situation.