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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cosmicdreams’s picture

FileSize
594 bytes

Could it be this simple?

cosmicdreams’s picture

Status: Active » Needs review
Crell’s picture

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

cosmicdreams’s picture

FileSize
652 bytes

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.

cosmicdreams’s picture

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?

Crell’s picture

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

cosmicdreams’s picture

so then....... RTBC?

Crell’s picture

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

catch’s picture

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

cosmicdreams’s picture

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.

Niklas Fiekas’s picture

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:


    // 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:

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:

   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.

/**
 * 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.

moshe weitzman’s picture

Still an issue?

Niklas Fiekas’s picture

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.

Niklas Fiekas’s picture

Issue summary: View changes

spelling and grammatical errors