Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Copying description from #363911: Batch API needs to return useful error messages, since this issue didn't come up in my search results.

This issue came up while testing #363262: Missing index on url_alias table, but works for really any Batch API-related thing. When Batch API encounters an error, instead of something actually helpful such as "I'm sorry, that index already exists," it gives you the completely unhelpful:

An unrecoverable error has occurred. You can find the error message below. It is advised to copy it to the clipboard for reference.
Please continue to the error page
An HTTP error 500 occurred.
http://localhost/core/update.php?id=7&op=do

An easy way to replicate this is to rename system_update_7018 it to system_update_7999 and run it again.

This is getting masked right now because we removed the ability to select past updates from update.php. It also happens in SimpleTest from time to time. Needs to be fixed before release or the support requests will be unbearable.

Damien Tournoud’s picture

Status: Active » Needs review
FileSize
4.73 KB

Here is a first shot at this. The security impact of allowing HTML messages to be displayed should be evaluated.

Status: Needs review » Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
4.58 KB

Rerolled, and fixed the test ;)

Status: Needs review » Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
4.58 KB

Let's try again.

Status: Needs review » Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
4.71 KB

Status: Needs review » Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
4.82 KB

And an even better version.

burningdog’s picture

Status: Needs review » Reviewed & tested by the community

This works for me. Batch API wasn't handling a PDOException and was simply returning a "An HTTP error 500 occurred" error. Applying Damien's patch resulted in a useful error message (about the PDOException) being returned to the browser.

sun’s picture

Status: Reviewed & tested by the community » Needs work
+      // When called from javascript, simply output the error message.

Please use "JavaScript" here.

+    // Force display of error messages in update.php or if the proper error
+    // reporting level is set.

Given the complexity of the following condition, "proper" sounds like something no one should understand. Can we change this, so developers understand?

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
7.36 KB

Rerolled!

Error reporting levels are now defined using those three constants:

/**
 * Error reporting level: display no errors.
 */
define('ERROR_REPORTING_HIDE', 0);

/**
 * Error reporting level: display errors and warnings.
 */
define('ERROR_REPORTING_DISPLAY_SOME', 1);

/**
 * Error reporting level: display all messages.
 */
define('ERROR_REPORTING_DISPLAY_ALL', 2);

Status: Needs review » Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
7.38 KB

Oups. I messed-up with the constants a bit. This one should work.

sun’s picture

Hm. Don't we have these already?

/**
 * Log message severity -- Emergency: system is unusable.
 *
 * @see watchdog()
 * @see watchdog_severity_levels()
 */
define('WATCHDOG_EMERG', 0);

/**
 * Log message severity -- Alert: action must be taken immediately.
 *
 * @see watchdog()
 * @see watchdog_severity_levels()
 */
define('WATCHDOG_ALERT', 1);

/**
 * Log message severity -- Critical: critical conditions.
 *
 * @see watchdog()
 * @see watchdog_severity_levels()
 */
define('WATCHDOG_CRITICAL', 2);

/**
 * Log message severity -- Error: error conditions.
 *
 * @see watchdog()
 * @see watchdog_severity_levels()
 */
define('WATCHDOG_ERROR', 3);

/**
 * Log message severity -- Warning: warning conditions.
 *
 * @see watchdog()
 * @see watchdog_severity_levels()
 */
define('WATCHDOG_WARNING', 4);

/**
 * Log message severity -- Notice: normal but significant condition.
 *
 * @see watchdog()
 * @see watchdog_severity_levels()
 */
define('WATCHDOG_NOTICE', 5);

/**
 * Log message severity -- Informational: informational messages.
 *
 * @see watchdog()
 * @see watchdog_severity_levels()
 */
define('WATCHDOG_INFO', 6);

/**
 * Log message severity -- Debug: debug-level messages.
 *
 * @see watchdog()
 * @see watchdog_severity_levels()
 */
define('WATCHDOG_DEBUG', 7);

IMO, it does not make sense to introduce yet another set of severity levels. Their intention and purpose are the same, just the target (watchdog/dsm) differs.

Damien Tournoud’s picture

@sun: we are not introducting anything, those were already there, I'm only giving them names ;)

sun’s picture

Status: Needs review » Needs work

ok, agreed. Should be reworked in a separate issue.

-  return message.replace(/\n/g, '<br />');
-};
+  return message.replace(/\n/g, '<br />');;
+}

That, I don't understand. The old code was correct, so please revert. ;)

After that fix, mark as RTBC, please.

Damien Tournoud’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
7.31 KB

Done. It was a merging artifact.

Marking RTBC as requested by sun.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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