drupal_get_filename() when database is down, does not deal with phptemplate themes

subscribing

Here’s a completely untested patch. :-p

Hmm… The patch definitely applies cleanly. Re-trying.

Why does the test bot hate me? I’m a nice guy.

Aha! davidstrauss explained in IRC that this means the Drupal installer couldn't run with this patch applied.

So, testbot does like me! It just has poor communication skills. "Failed to install HEAD" sounds like a bot failure; the error message should be "Drupal installer failed to run after patch applied."

Ok. I've actually run the installer with this patch.

[Edit: ignore the D6 patch; its broken because the syntax for calling drupal_system_listing() changed between D6 and D7. Masks in D7 require leading and trailing slashes.]

$mask = (($type == 'theme_engine') ? "/$name\.engine$/" : ($type == 'theme' ? "/$name\.info$/" : "/$name\.$type$/"));

Would it make sense to look for modules by their .info files as well?

Good point! That actually simplifies the patch. Since drupal_get_filename is only ever passed "theme, theme_engine, module", then we just need to special-case the theme_engine and look for .info files as the default. :-)

New patch needs review!

Just realized the $conf variable is now completely unused. New patch removes it.

Also, if you are reviewing the patch and wondering why the call to drupal_system_listing() looks the way it does, have a look at the way it is called in other places in Drupal (like in module_rebuild_cache): http://api.drupal.org/api/function/drupal_system_listing

Bot, please re-test. The patch should still be working.

New patch refactors drupal_get_filename() using a try/catch block per chx's suggestion in IRC. I'm not positive I see the utility over a non-catch/try version, so I've attached the alternative if anyone wants to compare. But I trust chx's code idea over my own. [Edit: …for good reason; see next comment.]

@Arancaytar: Apparently part of the system is expecting the return value of drupal_get_filename() to be pointing at directly at a module's .module file. So we can't return the path to the module's .info file. :-(

This new patch also adds some additional drupal_function_exists() calls since drupal_get_filename now depends on drupal_system_listing().

Finally, this new patch also adds a simpletest! My first. Testing bot, please be nice to me.

Ok. So the reason the try/catch block is better… Suppose the database is active, but there's a db failure because the tables aren't populated (like, if you were inside a DrupalUnitTestCase!), then the non-try/catch code would fail completely. :-p This is why I trust chx even when I don't understand him at first. :-)

This new patch gets rid of the evil "${type}s" string construct and shortens the code by one line (compared to previous patch).

bot was broken, afaik

I recommend drupal_get_filename('profile', 'test') for a real test.

   try {
     $file = db_query("SELECT filename FROM {system} WHERE name = :name AND type = :type", array(':name' => $name, ':type' => $type))->fetchField();
     if ($fie && file_exists($file)) {
       $files[$type][$name] = $file;
     }
   }
   catch (PDOException $e) {
   }
   if (!isset($files[$type][$name]))
   // filesystem failback.

Edit: the reason for this is that this way in case of any error you fail back to filesystem.

Here's a new patch with the improvements chx mentioned in #21 and #22.

Nice that you use drupal_function_exists to make sure you are not throwing a fatal error by calling a nonexisting function but in here when we know that we do not know ;) what state the database is in I would use function_exists instead. And note this in a comment.

Re-rolled.

Looks good.

FYI, this badly needs to be fixed in D6. Zen can't be used as a maintenance theme because I used drupal_get_path('theme', 'zen') to optimize/split up the template.php file.

Also, if you didn't notice while reading between the lines, chx helped me a lot with this patch. So credit goes to him as well.

+++ includes/bootstrap.inc	7 Jul 2009 02:37:23 -0000
@@ -575,20 +575,36 @@ function drupal_get_filename($type, $nam
+    try {

try/catch blocks are not typical in Drupal. It'd be good to document here why we're using it. The old code checking for db_is_active() was self explanatory.

+++ includes/bootstrap.inc	7 Jul 2009 02:37:23 -0000
@@ -575,20 +575,36 @@ function drupal_get_filename($type, $nam
+    catch (PDOException $e) {
+      // The database table may not exist.
+    }

That's great. Why? ;)

Also, why are we not doing something here and just falling through? Usually we would trigger an error. Let's explain why we're not.

+++ includes/bootstrap.inc	7 Jul 2009 02:37:23 -0000
@@ -575,20 +575,36 @@ function drupal_get_filename($type, $nam
+      $dir = $type . 's';

This is clever, but kind of a total hack. :P It only works because we use consistent naming conventions for themeS profileS and moduleS. But I guess that's ok...

However, how does this work for things in sites/all/modules or sites/all/modules/contrib? We've lost the $conf array data with this information?

+++ modules/simpletest/tests/bootstrap.test	7 Jul 2009 02:37:24 -0000
@@ -279,3 +279,37 @@ class HookBootExitTestCase extends Drupa
+ * Test hook_boot and hook_exit.

hook_boot() and hook_exit().

This review is powered by Dreditor.

Comments. That's what I write today. Lots and lots of comments.

This looks good, apart from the hook_boot/hook_exit comment was actually not about that hunk but the one added by the patch which, if I weren't so braindead right now, I would've realized was just flat-out wrong. :P Fixed this and committed to HEAD.

I really like this because it allows us to use drupal_get_path() consistently, rather than having to teach people tricks about when it sometimes works and other times doesn't.

Marking 6.x to be ported so Gabor can take a look.

Bah. It looks like when drupal_function_exists() was rolled back, that parts of this code weren't done properly. drupal_function_exists() did two things:

1. Checked if the function existed
2. Loaded the function definition if it didn't exist

The drupal_function_exists() in the patch in #29 were simply replaced with function_exists() calls, which doesn't do #2.

Here's a D7 patch that replaces the function_exists() calls with require_once statements that actually load in the function definitions that we need for this code.

And here's a D6 patch that backports everything.

Hmm, this adds two extra require_once to every bootstrap. Could we keep the function_exists() and require_once if it's not available?

@catch Ok! Fair enough. function_exists() doesn't have to do file stats, so calling it should be faster than require_once.

Re-rolled both D7 and D6 patches.

Thanks, that was my only complaint.

HEAD was broken. Resetting status.

Adding "quick fix" tag.

Committed to HEAD, thanks!

Marking down to 6.x for review.

The D6 patch in #33 still needs review.

/me scratches head. I could have sworn I removed the issue tags in my previous comment. :-\ That was kind of the whole point of it. Sorry for the unintentional spam. :-p

Ok, drupal_get_filename() is not something people normally use. But themes can't use module_load_include(), so they have to use its lower-level cousin drupal_get_path(). And drupal_get_path() is broken because of this issue.

Here's how to test this patch.

1. Set your theme to Garland.
2. Edit themes/garland/template.php and add this line after the <?php line:
   require './' . drupal_get_path('theme', 'garland') . '/color/color.inc';
   Functionally, this line does nothing useful; it just tests drupal_get_path() which calls drupal_get_filename().
3. View your site and notice that there are no errors or warnings displayed. Step #2 will not have broken anything.
4. Edit your site's settings.php file and (assuming you haven't already added $conf settings to that file), add this line to the end of it:
   $conf = array('maintenance_theme' => 'garland');
   That's the official way to set your maintenance theme in Drupal, fyi.
5. Now kill your database connnection by editing your settings.php and putting gibberish somewhere in your $db_url variable.
   $db_url = 'mysqli://gumby:ohnomrbill@localhost/drupal';
6. View your site and notice the maintenance theme does not appear. Instead you see a bunch of errors and warnings.
7. Apply the D6 patch in #33 above.
8. View your site and notice the maintenance theme displays just fine.

Been able to replicate this issue following the notes in #41 in D6.

The patch in #33 solved the problem.

patch -p0 < drupal-get-filename-341140-33-D6.patch 
patching file includes/bootstrap.inc
patching file includes/common.inc
Hunk #1 succeeded at 2784 (offset 9 lines).

attached re-rolled patch

Been able to replicate this issue following the notes in #41 in D6.

The patch in #33 solved the problem.

So… RTBC, then? :-)

I've confirmed the patch in #42 is a straight re-roll of my D6 patch in #33 and that it applies cleanly.

It does not look like this patch was extensively tested. By reading the patch, the pattern is not right when the type is not a theme or theme engine (ie. it looks for "dot then end of string, then "type" as a literal character sequence, then end of string again). Unlikely to match.

    $dir = $type .'s';
    if ($type == 'theme_engine') {
      $dir = 'themes/engines';
      $mask = $name .'\.engine$';
    }
    elseif ($type == 'theme') {
      $mask = $name .'\.info$';
    }
    else {
      $mask = $name .'\.$type$';
    }

Untested re-roll. Updates for string concat style changes and fixes the single quoted variable.

John Albin pointed out in irc that this is only for D6 and we don't necessarily need to update the code style. Not sure what the preference is now but here's one with oldie-style concatenation (if I got it right, I think I've erased that from my memory).

If drupal_load() is called in hook_requirements() or similar, the first check will pass and the db_query will run, but if we're currently installing from an install profile there's a point where we don't have a system table yet despite having a db connection.

As such, I'm @ing the db_query.
Rationalization:
* If the system table is missing, more errors will be along shortly, so you won't be missing anything.
* db_table_exists('system') is just plain silly.
* This is needed for the drupalorg_testing install profile.

We should add a comment for the @db_query(), otherwise it looks like Brandon tested this, and I don't see any other issues in the patch.

Added to the existing comment. There is a small performance hit from using @, but while drupal_get_filename() is called very often, it's extremely rare that it needs to query the database for a file, so this is acceptable I think.

This is a version of the patch compatible with -p0. PLEASE IGNORE THIS. This is only needed for http://drupal.org/project/drupalorg_testing.

Adding tests for this fix in D8 here: #1334862: Test broken: drupal_get_path doesn't work for theme engines and test coverage is poor, it seems it slipped into D7 without them, and promptly regressed, sigh.

Sorry, did not mean to change the status.

Performance problem:
drupal_system_listing() is expensive.
If the db is down, it is likely this function will be called plenty of times, for different module and theme names.
(or did I miss something?)

Solution:
Run drupal_sytem_listing() only once (or once for each "type"), with a wildcard regex. The result can be cached, either in drupal_system_listing() itself, or in a wrapper function. Further calls can then just look into the cache.

And something else:
It looks like the old code, in case the db is down, not only has just this problem with phptemplate themes, but also totally fails with modules and themes in subdirectories. Am I right?

One thing, function db_is_active() is in database.inc. With a normal site, the file is included with DRUPAL_BOOTSTRAP_DATABASE and that stage actives the db as well. The check "if (db_is_active())" appears useless because people still cannot use drupal_load() before DRUPAL_BOOTSTRAP_DATABASE.

FYI: I have this problem because I'm trying to use drupal_load() in cache inc file - #1381410: Use drupal_load() when it's possible.