When any of my tests fail because they are calling a function which is not defined I get an HTML-styled error message converted to plain text. See attached image for an example.

Either 1) stripping the tags or 2) not converting it to plain text will definitely make the message a bit more informative (and pretty).

Running Garland that comes with Drupal 6.4 as my theme.

CommentFileSizeAuthor
#12 313902-screenshot.png68.61 KBc960657
#10 313902-2.patch1.48 KBc960657
#7 313902-1.patch6.36 KBc960657
#3 test_crash.tar_.gz544 bytesztyx
beautify_error_message.jpg263.73 KBztyx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

boombatower’s picture

boombatower CreditAttribution: boombatower commented
Project: SimpleTest » Drupal core
Version: 6.x-2.4 » 7.x-dev
Component: User interface » simpletest.module

This should be fixed in core and backported.

lilou’s picture

lilou CreditAttribution: lilou commented
Status: Active » Postponed (maintainer needs more info)

It seems that this bug comes from your custom module "translation review.

How can we reproduce it ?

ztyx’s picture

ztyx CreditAttribution: ztyx commented
FileSize
test_crash.tar_.gz544 bytes

Oh, I thought the it was self explanatory how to reproduce it - just call a function that does not exist in one of your test cases. For simplicity I am attaching a sample module that contains a test that calls a function that does not exist. You should get the same output as in the earlier attached screenshot.

The module is called "test_crash".

ztyx’s picture

ztyx CreditAttribution: ztyx commented
Status: Postponed (maintainer needs more info) » Active
drewish’s picture

drewish CreditAttribution: drewish commented

you shouldn't need a module for that, you should be able to trigger the error in a test case... just call NONEXISTANT_FUNCTION();

c960657’s picture

c960657 CreditAttribution: c960657 commented

I believe this problem is due to the html_errors ini setting being enabled. The big HTML table is generated by XDebug, but even without XDebug you get HTML tags in the error message when html_errors is enabled.

The PHP error message is generated in Drupal.ahahError() in misc/drupal.js where it is treated as plain text (it is inserted in the error string using @text rather than !text). I don't think it is safe to include the PHP error using !text, but I am not completely aware of the threat model here.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Active » Needs review
FileSize
313902-1.patch6.36 KB

This patch disables html_errors for all of Drupal. Would anybody miss those?

Currently drupal_error_handler($errno, $message, ...) doesn't deal properly with HTML encoding. Whether $message is HTML or plain text depends on html_errors, but the function doesn't check that setting. $message is passed directly to drupal_set_message($message, ...) that expects an HTML string, but in the watchdog() call it is inserted with %message, i.e. it is assumed to be plain text.

Currently, when html_errors is off, error messages from tests are currently not being displayed. This is because of the line
if (jQuery.trim($(xmlhttp.responseText).text())) {.
jQuery's $(somestring) allows somestring to be either an HTML string or a selector. If responseText doesn't look like HTML, it is treated like a selector - and that does not match anything, and thus the output isn't displayed. This bug is also fixed by this patch.

c960657’s picture

c960657 CreditAttribution: c960657 commented

Reroll. The patch is now simpler due to the rewrite of the error handling system in #304924: Extend drupal_error_handler to manage exceptions.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs review » Needs work

We now have html_error set to 0 in drupal_initialize_variables. I'm not sure if the other bits are really needed (especially the check_plain() of the error message...).

c960657’s picture

c960657 CreditAttribution: c960657 commented
FileSize
313902-2.patch1.48 KB

Sorry, I forgot to attach the actual updated patch :-(

c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Needs work » Needs review
c960657’s picture

c960657 CreditAttribution: c960657 commented
Title: HTML formatted error as plain text » DX: Show fatal errors in tests
FileSize
313902-screenshot.png68.61 KB

Updating title. With the disabling of html_errors no error messages are shown at all. I have attached an updated screenshot showing how errors are displayed with and without this patch.

I don't know why the testbed robot could not install the patch. AFAICT it still applies to HEAD.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

Committed to CVS HEAD. Thanks again. :-)

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.