Switch from db_is_active() to proper exception catching

I don't mind taking a stab at this (after #325169: Move error/exception handler higher up in the bootstrap process gets dealt with), but I'm not convinced the solution is to ditch db_is_active() altogether. While I get that try/catch should be utilized for this sort of thing (especially in core), because Drupal doesn't really have a "strict" OO flow, database calls are not well encapsulated. Having a D6 db_is_active() makes sense to me in that light, as it's a bit of a crapshoot to know if any particular API function actually makes a database call or not.

Since there are only four instances of db_is_active() in core, what about:

1. converting the 4 instances to try/catch
2. making db_is_active() authoritative for the future availability of the database (to ensure it acts as it did in D6)
3. adding a comment to db_is_active() that it will likely be deprecated in the "near" future (as in Drupal 8)

Right but in D6 the logic is more db_is_available()ish. The four uses of it in core right now pretty much assume that the logic is "will the connection ever be available". My worry would be that contrib code relies on that, although I'd imagine the affected code by the logic change is rather small (but probably quite annoying to debug if it happens).

Or put it this way, since we're removing db_is_active() calls from code, it essentially is there solely for contrib code. We likely can't just remove it this late in the D7 cycle, and as such should it not act more like the D6 version?

The use of db_is_active() in #325169: Move error/exception handler higher up in the bootstrap process nearly caused a 30% regression for cached pages until I spent a decent amount of time benchmarking, then having to prove that the benchmarks weren't flawed, it has no use in its current state, so should be removed.

+++ includes/theme.inc	6 Feb 2010 02:40:01 -0000
@@ -587,16 +587,22 @@ function list_themes($refresh = FALSE) {
+      // If the database is not available, rebiuld the theme data.

Typo: rebiuld should be rebuild.

+++ includes/theme.inc	6 Feb 2010 02:40:01 -0000
@@ -2232,19 +2238,22 @@ function _template_preprocess_default_va
+  catch (DatabaseConnectionNotDefinedException $e) {

What does it mean for a database connection not to be 'defined'. Should it be DatabaseConnectionNotAvailableException instead of DatabaseConnectionNotDefinedException?

#719850: errors in update.php during bootstrap yield a blank page was duplicate, bumping this to critical based on WSOD over there.

Subscribe

@Crell some explanation you could find at #719850: errors in update.php during bootstrap yield a blank page

Ok, here is my try...

- Imho, we need to catch PDOException, not DatabaseNotDefinedExceptions. Because the latter is just thrown when the database is not defined at all.
- Changed DatabaseNotDefinedException to extend from PDOExecption instead of Exception so that it can be catched with PDOException too.
- try/catch doesn't work well for undefined functions/objects so I had to wrap a user_access() and a $user in an if clause
- The code in _drupal_maintenance_theme() does something in case the database is *not* available. I tried to do this by doing a simple, stupid SQL query but that's rather ugly. A better approach might be to simply try go forward and act once an exception happens on itself. But it might be complicated to actually handle it correctly from then on.

Another thing:
- The current "database down" maintenance page simply displays a PDO exception, that's not really nice but not something that needs to be handled here..

ups...

I tried applying this patch to fix my "WSOD on update.php" problem and it didn't work. If I go to line 326 of update.php and change ini_set('display_errors', FALSE); to TRUE I get:

Fatal error: Call to undefined function _system_rebuild_theme_data() in /Volumes/Home/Users/stephenkarsch/webs/d7.stevekarsch.com/public_html/includes/theme.inc on line 586

(same error as before I applied the patch).

I will try to dig in but I think I'm in over my head! I'm happy to test though.

@Berdir take a look at #719850: errors in update.php during bootstrap yield a blank page

EDIT: _system_rebuild_theme_data() could be unavailable

This patch looks good to be. Would be great to get sign-off from Crell. From the comments above, it sounds as if he'd be in support, but still.

Hm, ok. I just dislike extending all exceptions directly from Exception, but that's not something for this issue :)

Unless we actually do something different in those two catch's, I assume we can just simply catch Exception. Will do a reroll soon...

I am working on a re-roll, just a short question...

I found out that the new exception actually can't be thrown at all.

Looking at the following code...

    if (!isset(self::$connections[$key][$target])) {
      // If necessary, a new connection is opened.
      self::$connections[$key][$target] = self::openConnection($key, $target);
    }

    if (empty(self::$connections[$key][$target])) {
      throw new DatabaseConnectionNotDefinedException('The specified database connection is not defined: ' . $key);
    }

The second if is never going to be TRUE, because if the connection isn't set, we enter the first if which then bails out with a Exception in openConnection():

      // If the requested database does not exist then it is an unrecoverable
      // error.
      if (!isset(self::$databaseInfo[$key])) {
        throw new Exception('DB does not exist');
      }

So we can simply drop the conditional stuff and optionally throw the DatabaseConnectionNotDefinedException inside openConnection(). There is also a similiar case, when there is no driver:

      if (!$driver = self::$databaseInfo[$key][$target]['driver']) {
        throw new Exception('Drupal is not set up');
      }

Opinions?

Attached is a re-roll with the proposed changes from above.

This is getting quite a bit off-topic, we should eventually create a follow-up issue, especially if there is a disagreement on how to name the Exceptions or something like that :). There are also notices about undefined array indexes when commenting out the driver in settings.php to test that the exception gets thrown.

Simply doing a catch (Exception $e) now, since we probably don't act differently based on the thrown exception.

subscribe

Looks good to me, very small nitpick with:

// user_access()/$user might not be available, default to FALSE.

I had to do a double take because that looked like a path, could we expand it a bit like the comment further down? Apart from that looks RTBC, strongly agree about not bikeshedding exception names in this issue.

Ok..

I decided to move the user.module stuff out of the try/catch as it is going to fail anyway and added the defaults to the initial defaults array. It does look a bit strange in the patch but I think it's easier to understand when looking at the resulting code.

Comments might still need to be improved though :)

+++ includes/database/database.inc
@@ -1628,6 +1627,17 @@ class FieldsOverlapException extends Exception {}
 /**
+ * Exception thrown if an undefined database connection is requested.
+ */
+class DatabaseConnectionNotDefinedException extends Exception {}
+
+/**
+ * Exception thrown if no driver is specified for a database connection.
+ */
+class DatabaseDriverNotSpecifiedException extends Exception {}

These should probaly extend from RuntmeException, just like PDOException
(see also: http://php.net/manual/spl.exceptions.php)
This should allow a more specific catch.

Powered by Dreditor.

You are most probably right, but see the middle paragraph in #23, I don't think this is the correct issue for doing that. Instead, we should probably change all Exceptions in database.inc change to extend from RuntimeException.

Committed to CVS HEAD. Thanks.

I see no commit message at http://drupal.org/project/cvs/3060

Also follow-up for maintenance mode #766100-6: Undefined function _system_rebuild_theme_data when trying to run update.php from D6 -> D7

Patch still applies :)

A hunk from _drupal_maintenance_theme() should be if (!class_exists('Database', FALSE)) {

Details in #766100-10: Undefined function _system_rebuild_theme_data when trying to run update.php from D6 -> D7

Re-rolled minus the theme.maintenance.inc change - we can just do that in #766100: Undefined function _system_rebuild_theme_data when trying to run update.php from D6 -> D7.

#36: die.patch queued for re-testing.

Since this is just a re-roll without the hunk that was committed elsewhere, and was already RTBC before apart from that hunk, I'm setting it back.

#36: die.patch queued for re-testing.

Another nitpick,

       if (!$driver = self::$databaseInfo[$key][$target]['driver']) {
-        throw new Exception('Drupal is not set up');
+        Throw new DatabaseDriverNotSpecifiedException('Driver not specified for this database connection: ' . $key);
       }

it should be throw new DatabaseDriverNotSpecifiedException..., right?

Reroll with changes:
1) "Throw" => throw
2) in list_themes() try-catch moved inside if() because only system_list() depends on DB

One problem, there's still a call to db_is_active() in theme.maintenance.inc

mfb: This was fixed already in #766100: Undefined function _system_rebuild_theme_data when trying to run update.php from D6 -> D7

Re-roll after #722978: Invalid header count for empty message in theme_table().

Same patch, just fix ofset

There's still a call to db_is_active() in theme.maintenance.inc -- but this patch removes the function.

Apparently my cvs is on crack. I mean, cvs update -D does bad things. RTBC

(sorry :)

It looks like Dries meant to commit this earlier and missed it. Oops!

Committed to HEAD.

Looks like we need to document this change in the 6 => 7 module upgrade docs.

Added placeholder http://drupal.org/update/modules/6/7#db_is_active
Needs explain better when use class_exists() [bootstrap] and when exception catching [after bootstrap]

Crell I've used this once in my custom module so I think this needs details mostly about replacing within bootstrap.

#779494: Notices on install screens is probably related to this.