Abstraction of Database connection error handling.

Setting this to "won't fix", since that was the agreed method for providing backports / features in older versions.

Aaron, please re-roll against HEAD, not including the custom implementation in settings.php (just include that in your comment / description), and create a new issue for it. Thanks!

Woah, this is a great little feature! Don't won't fix it until we get it in 6. :)

Benefits:
- Right now the word "Drupal" is hard-coded in these error messages. Particularly troublesome if you're trying to mask what platform you're running your site on for security reasons (yes, security by obscurity, but still... no sense in handing someone a golden key) or if you have your own distribution called SuperCheese.
- You're also spitting sensitive database information to the general public. And unlike regular error stuff that happens which you can select under error handling to not show on screen, this will show up for everyone who views your site. Silliness.

The only thing I'm not crazy about is requiring putting this in settings.php, simply because in the case of a multisite installation you'll have 40,000 of those. Is there another place it could go?

Also, it might be worth giving all lack-of-db-related errors this treatment... ex: unsupported database type, PHP support not enabled, can't select database, install/update messages.

Here's another approach... note that this doesn't actually work yet. ;)

The idea, though, is you place a custom function somewhere (probably settings.php, but could also be hacked into bootstrap.inc for multisite installs):

/**
 * Custom error handler.
 */
function custom_offline_error_handler($id, $error) {
  if ($id == 'db-connect') {
    $error['error'] = 'Please try again later.';
    mail('admin@example.com', 'Fix the db!', $error);
  }

  return $error;
}

From chx:

if (!function_exists('custom_offline_error_handler') || !($error = custom_offline_error_handler($id))) {
// put switch here
}

I like this concept.

+1.

Here is another approach. Not sure what people think.

It is along the lines of how the caching .inc file is pluggable via a $conf variable that points to an alternate file to use (see bootstrap.inc):

function _drupal_cache_init($phase) {
  require_once variable_get('cache_inc', './includes/cache.inc');

This can be done but not straightforward. We need to do:

$db_inc = variable_get('db_inc', './includes/db_' .$_GLOBALS['db_type']. '.inc');
// check if file exists first, then
require_once $db_inc;

This way, we have a default handler for each db_type (or maybe no handler, but sites can override it by custom stuff. A site would use one db type, so they only need one .inc.

What do people think?

-1 to killes' approach

killes did not comment here. You mean kbahey? They are so different you know? ;-)

LOL sorry I mean kbahey (I was talking to killes on IRC while posting this) I don't like your approach 'cause that defeats the purpose of db_set_active. (hint: look @ the docs for that)

No longer applies, bumping to Drupal 7

Is this still necessary given the new DB API? Error handling is now via exceptions, so it's an entirely different animal.

There was a move underfoot to provide D7 with a completely themable "database error, site offline" message. Doesn't that negate this approach? Having just skimmed this patch (#9) I find that it's trying to change the default output of a db_error and allow it to be overridden. Right? Seems to me that the themable site offline movement would handle this as well.

I don't think that's the use case necessarily.

Imagine if you had an error condition you wanted to display to user. Would you throw up the database error, "Access denied for user 'xx'@'x.x.x.x' to database 'xxx'". This isn't friendly, and reveals a LOT of information which you probably don't want to advertise.

An override patch like this (which might even be considered a default), would be to attach an error condition, could put up something like, "We're sorry, a database error occurred, therefore masking the error (nice for security), and maybe dropping some kind of ticket into my helpdesk to let me know that things are busting up something serious. :) That's very different from a site offline.

$db_inc = variable_get('db_inc', './includes/db_' .$_GLOBALS['db_type']. '.inc');
// check if file exists first, then
require_once $db_inc;

The only problem I have with this is that this evaluation is being done on every database request. This could have some implications for larger sites, where, at least from a performance standpoint (and Drupal needs it), simple would be better. The idea is great, I'm just wondering if there is a less expensive way to write it.

Given the last comment, this is at least critical. Perhaps. Let's ensure and prove it's not.

This is a feature request, so I'm going to bump it to Drupal 8. If someone from the security team wants to push it back to Drupal 7, make sure Drieschick will let us work on it at this stage of the game before doing so. :-) I'll defer to Drieschick/security folks here.

Downgrading all D8 criticals to major per http://drupal.org/node/45111

Probably 8.1 at least.

Is this still relevant?
I stumble on this after being surprised that when your DB server is down you get a blank page with error instead of some nicer page.