Problem/Motivation

If _drupal_log_error() hits within a template, output buffer contents are printed before the maintenance page, causing broken layout.

Steps to reproduce

An easy "hack" to artificially reproduce this is to insert the following code inside theme():
(other places might also work)

function theme(..) {
  [..]
  static $thrown = true;
  // Capture $ob_contents in a variable for debugging.
  if (!$thrown && $ob_contents = ob_get_contents()) {
    $thrown = TRUE;
    throw new \Error('Fake error in theme().');
  }
  [..]

(Typically such errors would happen in arbitrary places due to programming errors, but it is simpler to generate it artificially.)

When visiting the site with a browser, the html source will contain the output buffer contents + the maintenance page, causing a broken layout.

Technical explanations

A demonstration of the effect with plain php: https://3v4l.org/6r9b4


function exception_handler($e) {
    print "Exception page\n";
    exit();
}

set_exception_handler('exception_handler');

ob_start();
print "A\n";
ob_start();
print "B\n";

throw new Exception();

Special considerations

  • 'Throwable' and 'Error' exist since PHP 7.0.0.
    Since then, fatal errors behave similarly to exceptions and go to exception handler instead of the error handler.
  • 'finally' is supported since PHP 5.5.0. We could use it, but I think we don't need to.
  • Initial output buffer level may be 1 or 0 depending on the 'output_buffering' setting in php.ini, see https://www.php.net/manual/en/outcontrol.configuration.php. It seems Drupal works fine with either value.
  • Drupal starts an output buffer level with ob_start() in _drupal_bootstrap_page_header(), and ends it in drupal_page_footer().

Proposed resolution

Different options:

  • Clear / end all output buffer levels, before printing the maintenance page, in _drupal_log_error().
    This would also close the output buffer from _drupal_bootstrap_page_header(), and the one from php itself. Not sure if that's good or bad.
  • Use try / catch in theme_render_template(), to always end the output buffer, even when a fatal error or exception occurs.
    This should be a safe improvement, and sufficient for the case discussed here.

Issue fork drupal-3191789

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

donquixote created an issue. See original summary.

drumm made their first commit to this issue’s fork.

drumm’s picture

("Issue fork creation failed!", for some reason)

Our monitoring caught this and I’ll be looking into it. The issue fork is now available.

donquixote’s picture

Our monitoring caught this and I’ll be looking into it. The issue fork is now available.

Thanks @drumm!

Proof of concept patch.

I am changing my mind. I think it would be better to try / catch / re-throw directly in theme_render_template().
I should change the issue title and summary to describe the problem instead of prescribing a specific solution.

donquixote’s picture

Title: _drupal_log_error() should clear output buffer before printing maintenance page » Error page layout broken due to leftovers from template output buffer
Issue summary: View changes

donquixote’s picture

I want to see how broken our html is..

donquixote’s picture

We want tests for this!

I think we should more broadly verify that html is correct.
I created #3191922: Collect DOMDocument errors in web test, instead of suppressing them for this purpose.
I also found #355185: Add HTML validation to functional tests which is for D8, but I think we should handle this for D7 independently.

For this issue we should keep it simple, and only verify that the doctype is not prepended by any junk html. This can be a simple string comparison.

donquixote’s picture

Sorry for all the noise from git. I still need to get used to the issue workspaces.

Either way, I think this is at a point where some feedback (reviews) would be nice.
I think the tech is ok, but perhaps we want to rename or fine-tune the tests.

donquixote’s picture

Status: Active » Needs review
donquixote’s picture

Issue summary: View changes
donquixote’s picture

Issue summary: View changes

Simpler example code.

donquixote’s picture

Some cleanup:

  • Created and closed the merge request !244 to hide the branch "3191789-drupallogerror-should-clear".
    (I don't know if there is another way to hide a branch)
  • Hiding the patch file.