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.
Comment | File | Size | Author |
---|
Issue fork drupal-3191789
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:
Comments
Comment #2
donquixote CreditAttribution: donquixote commentedProof of concept patch.
("Issue fork creation failed!", for some reason)
Comment #4
drummOur monitoring caught this and I’ll be looking into it. The issue fork is now available.
Comment #5
donquixote CreditAttribution: donquixote commentedThanks @drumm!
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.
Comment #6
donquixote CreditAttribution: donquixote commentedComment #9
donquixote CreditAttribution: donquixote commentedI want to see how broken our html is..
Comment #10
donquixote CreditAttribution: donquixote commentedWe 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.
Comment #13
donquixote CreditAttribution: donquixote commentedSorry 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.
Comment #14
donquixote CreditAttribution: donquixote at European Commission and European Union Institutions, Agencies and Bodies commentedComment #15
donquixote CreditAttribution: donquixote at European Commission and European Union Institutions, Agencies and Bodies commentedComment #16
donquixote CreditAttribution: donquixote at European Commission and European Union Institutions, Agencies and Bodies commentedSimpler example code.
Comment #19
donquixote CreditAttribution: donquixote at European Commission and European Union Institutions, Agencies and Bodies commentedSome cleanup:
(I don't know if there is another way to hide a branch)