If the database connect fails, the database layer calls
drupal_maintenance_page,
which calls drupal_get_path
which calls drupal_get_filename
which queries the database.

There are two solutions, add a $check_db arg to drupal_get_path or call drupal_get_filename directly from
theme_maintenance page. Since most normal activity requires database connectivity I've just written a patch to call drupal_get_filename directly.

I've also got an alter patch which extends drupal_get_path, but I'm not sure if minor API changes can make it in at this point.

I'm posting as critical, as the last thing we want users to experience when first install drupal is big confusing error messages for something they haven't configured yet... I'm calling it a bug since we shouldn't query the db if we unable to connect to it.

CommentFileSizeAuthor
#13 drupal_get_filename_0.patch3.57 KBrobertdouglass
#12 99030.patch.txt2.66 KBdopry
#10 drupal_get_filename.patch3.23 KBrobertdouglass
#6 99030-drupal_get_path.patch_1.txt1.77 KBdopry
#5 99030-drupal_get_path.patch_0.txt2.39 KBdopry
#2 99030-drupal_get_path.patch.txt1.69 KBdopry
#1 99030-drupal_maintenance.patch.txt847 bytesdopry

Comments

dopry’s picture

dopry commented
StatusFileSize
new 99030-drupal_maintenance.patch.txt847 bytes

Here is the patch to drupal_maintenance_page, that does not add any calling arguments.

dopry’s picture

dopry commented
StatusFileSize
new 99030-drupal_get_path.patch.txt1.69 KB

Here is the patch that extends drupal_get_path to include the $checkdb argument like drupal_get_filename which it calls. I prefer this patch. I leave it to reviewers to determine which is best...

dopry’s picture

dopry commented
Status: Active » Needs review

to reproduce...

-- make sure your db authentication information is *incorrect*
-- navigate to 'mysite' and see the errors from mysql_* and company not being able to
query the database if you're running mysql.

You may need E_ALL as well.

RobRoy’s picture

RobRoy commented
Status: Needs review » Needs work

Should be $check_db instead of $checkdb in #3.

dopry’s picture

dopry commented
StatusFileSize
new 99030-drupal_get_path.patch_0.txt2.39 KB

update version of the drupal_get_path patch with check_db instead of checkdb.

dopry’s picture

dopry commented
StatusFileSize
new 99030-drupal_get_path.patch_1.txt1.77 KB

maybe a version without my settings.php would be better. :)

RobRoy’s picture

RobRoy commented

Looks good. Skips the DB check as advertised. Nice code comments.

eaton’s picture

eaton commented
Status: Needs work » Reviewed & tested by the community

A big +1 on this. It would probably serve us well to hunt for more of these unspoken db assumptions in really low level drupal functions. There's no reason we should hit the DB on a no-DB error screen. ;)

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work

The code comments need more work. They are only accessible to Drupal experts, and not to Drupal newbies.

We should also improve that parameter in drupal_get_filename() where the code comment is copy-pasted from.

Please work a bit more on the code comments so the Average Drupal Developer can understand them.

robertdouglass’s picture

robertdouglass commented
StatusFileSize
new drupal_get_filename.patch3.23 KB

Ok, I wrote a book for it. Let me know if it is too much.

robertdouglass’s picture

robertdouglass commented
Status: Needs work » Needs review
dopry’s picture

dopry commented
StatusFileSize
new 99030.patch.txt2.66 KB

well I was having trouble coming up with better comments, so I just eliminated the argument and used active_db instead.

robertdouglass’s picture

robertdouglass commented
StatusFileSize
new drupal_get_filename_0.patch3.57 KB

Thhis patch merges in the docs that I wrote and makes the $files array static again.

dopry’s picture

dopry commented
Status: Needs review » Reviewed & tested by the community

I hope we have a a winner.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Cleaned up the documentation a bit and committed it to CVS HEAD.

Anonymous’s picture

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