Posted by Crell on May 20, 2012 at 11:19pm
7 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (works as designed) |
| Issue tags: | kernel-followup |
Issue Summary
Per #1463656-126: Add a Drupal kernel; leverage HttpFoundation and HttpKernel:
Also, not quite sure, but I think RequestCloseSubscriber::onTerminate() calls ob_flush() in all situations, although OB is only started in _drupal_bootstrap_page_header() if !drupal_is_cli().
That logic was ported over from drupal_page_footer(). Since we want to eliminate the ob_flush() anyway that may not be an issue, but if we do keep it, we might want to do it less often? If we keep it and do it for every request, we should document why we are doing so since it seems odd.
Comments
#1
Could it be this simple?
#2
#3
If that's the change we want to make, sure. The question is whether that's the change we want to make. :-) Also, global function a listener == bad (although there's no alternative mechanism yet).
#4
I suppose the crux of this issue is the question, "Where do we NEED the ob_flush()?" Perhaps a good way of discovering that would be to upload a patch with it removed and see what breaks.
#5
Now that's surprising. I assumed something was going to break. I'm guessing this is because we don't have the proper test to check for this.
That begs the question: by not having ob_flush here, what kind of problems do we anticipate seeing? Performance issues? Session variables that bleed over to other requests? Security issues?
#6
Since that call is after the actual page output, I think it would only flush out any content printed in hook_exit(), such as that generated by devel. And then that would only matter on systems that have output buffering enabled in php.ini. And even then, output is flushed at the end of the request anyway by PHP.
So, I have no idea why we need that line. :-)
#7
so then....... RTBC?
#8
For me, yes, but let's get some further input from others first.
#9
Original issue where this was added #477944: Fix and streamline page cache and session handling, I did not read it through yet.
#10
I couldn't find where in that issue this issue was referenced but i did find this paragraph referencing ob_end_flush()
Which seems to call for the inclusion of some kind of ob_flush activity to be a dded to the drupal_page_footer() function.
#11
Let's see.
$ grep -r "ob_start" .[... leaving out Symfony usage of ob_start ... ]
./includes/batch.inc: ob_start();
./includes/theme.inc: ob_start(); // Start output buffering
./includes/bootstrap.inc: ob_start();
./modules/php/php.module: ob_start();
batch.inc always cleans up, after itself:
<?php
// Error handling: if PHP dies due to a fatal error (e.g. a nonexistent
// function), it will output whatever is in the output buffer, followed by
// the error message.
ob_start();
$fallback = $current_set['error_message'] . '<br />' . $batch['error_message'];
$fallback = theme('maintenance_page', array('content' => $fallback, 'show_messages' => FALSE));
// [...]
// PHP did not die; remove the fallback output.
ob_end_clean();
?>
theme.inc too:
<?phpfunction theme_render_template($template_file, $variables) {
extract($variables, EXTR_SKIP); // Extract the variables to a local namespace
ob_start(); // Start output buffering
include DRUPAL_ROOT . '/' . $template_file; // Include the template file
return ob_get_clean(); // End buffering and return its contents
}
?>
php.module (<- yuck) too:
<?phpob_start();
print eval('[... deleted chars that the input filter can not handle ...]' . $code);
$output = ob_get_contents();
ob_end_clean();
?>
But wtf is bootstrap inc doing there? I can't find any matching ob_ operation, except that in the request terminate handler.
<?php
/**
* Invokes hook_boot(), initializes locking system, and sends HTTP headers.
*/
function _drupal_bootstrap_page_header() {
bootstrap_invoke_all('boot');
if (!drupal_is_cli()) {
ob_start();
}
}
?>
I want to see if anything breaks, when we remove that, too.
#12
The last submitted patch, 1591682-output-buffering-11.patch, failed testing.
#13
Still an issue?
#14
Looks like #1651010: Unification of drupal_exit() and drupal_page_footer() and performance boost for FPM enabled environments attacked this and then #1698108: Update Drupal's dependencies could remove all the hacks without breaking anything. I think this is fixed indeed. The worst thing that could happen is we notice something when doing #1597696: Switch page caching to HttpCache.