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.

Files: 
CommentFileSizeAuthor
#11 1591682-output-buffering-11.patch1 KBNiklas Fiekas
FAILED: [[SimpleTest]]: [MySQL] 36,805 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
#4 1591682_5_ob_flush.patch652 bytescosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 36,681 pass(es).
[ View ]
#1 1591682_1_ob_flush.patch594 bytescosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 36,675 pass(es).
[ View ]

Comments

StatusFileSize
new594 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,675 pass(es).
[ View ]

Could it be this simple?

Status:Active» Needs review

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).

StatusFileSize
new652 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,681 pass(es).
[ View ]

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.

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?

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. :-)

so then....... RTBC?

For me, yes, but let's get some further input from others first.

Original issue where this was added #477944: Fix and streamline page cache and session handling, I did not read it through yet.

I couldn't find where in that issue this issue was referenced but i did find this paragraph referencing ob_end_flush()

I wonder whether it would be more efficient to replace print drupal_render_page($return); in index.php with $output = drupal_render_page($return); and then pass that to drupal_page_footer(), and then add ob_end_flush(); print $output to the bottom of drupal_page_footer(). The whole page has already been generated in one string, and this change prevents the string from being put into the output buffer before being sent to the client. I haven't benchmarked it, though. The same approach can be used by drupal_serve_page_from_cache().

Which seems to call for the inclusion of some kind of ob_flush activity to be a dded to the drupal_page_footer() function.

StatusFileSize
new1 KB
FAILED: [[SimpleTest]]: [MySQL] 36,805 pass(es), 10 fail(s), and 0 exception(s).
[ View ]

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:

<?php
function 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:

<?php
   ob_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.

Status:Needs review» Needs work

The last submitted patch, 1591682-output-buffering-11.patch, failed testing.

Still an issue?

Status:Needs work» Closed (works as designed)

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: Consider whether HttpCache offers any significant benefit over the existing page cache.

Issue summary:View changes

spelling and grammatical errors