If output_buffering = Off then ob_end_clean fails to delete buffer
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | file system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | patch (reviewed & tested by the community) |
If output_buffering is turned off in php.ini and after a call to file_transfer in file.inc line 794 is called the following notice will be returned.
"notice: ob_end_clean() [ref.outcontrol]: failed to delete buffer. No buffer to delete. in .../includes/file.inc on line 795."
One possible solution, as seen in obendclean_sol2.patch, would be to suppress the error by prefixing ob_end_clean() with an '@'.
<?php
@ob_end_clean();
?>Another possible solution, as seen in obendclean_sol1.patch, would be to perform a check first.
<?php
if (ob_get_length() > 0) {
ob_end_clean();
}
?>I checked against Drupal 5 versions of file.inc and it appears to have ob_end_clean() called at the beginning of file_transfer() as well. The once difference that I could think of that would make this issue appear now is the recent changes to common.inc involving E_ALL.
I don't know if this is something that we even want to change but users who meet the above conditions may be subjected to such notices, for example, every time a new imagecache image is generated.
I've attached two patches one for each method discussed above.
Thanks,
-mpare
| Attachment | Size |
|---|---|
| obendclean_sol1.patch | 664 bytes |
| obendclean_sol2.patch | 628 bytes |
#1
Just chiming in as I was discussing this issue with mpare.
I believe the intention of the line was to make sure that output would be sent to the client rather than any output buffer that may be open...
Hmm, it should *probabaly* be closing as many buffers as needed, right?
<?phpwhile (ob_get_length()) {
ob_end_clean();
}
?>
The ob_end_clean() line was in the original version of file.inc. I'm guessing that the reason it's popping up now is that in the past, nobody had A) E_ALL enabled, B) output_buffering off, and C) filed an issue.
#2
it looks like it's used a few places in core now:
includes/batch.inc: ob_end_clean();
includes/common.inc: ob_end_clean();
includes/common.inc: ob_end_flush();
includes/file.inc: ob_end_clean();
includes/theme.inc: ob_end_clean(); // End buffering and discard
modules/system/system.module: ob_end_clean();
modules/system/system.module.orig: ob_end_clean();
#3
This should be the one place in core where ob_end_clean() is called without ob_start(). Here's another proposed fix -- use ob_get_level() instead of ob_get_length().
#4
Indeed ob_get_level is the correct function for this filter.
#5
+1
i don't see any easy way to unit test this since it's just a warning message so i manually verified it. added a file, enabling private file transfers, downloaded it to get the warning, then patch and downloaded again and had no warning.
#6
I checked over the other instances of ob_end* and it appears they indeed all call start/get_contents ahead of time.
Committed to HEAD. Thanks! Moving back for consideration for 6.x.