Accurate error message when settings unreadable

I can confirm this error.

How come your Drupal files had no read permissions set? Was that due to an FTP upload?

(This looks like a rare scenario to me -- therefore I'd like to understand what caused this.)

The same happens if for some reason settings.php is not present. The error message that catches the problem (described above) is not exactly helpful, although I agree that this should be a very rare problem.

Anyway, here is a patch against HEAD that adds a check to config_init() in bootstrap.inc and gives a more suitable error message. I have tested it quickly and it seems to work fine.

Needs review. I don't have an opinion about whether this has a critical or normal priority.

This is normal priority as it's a rare case. I also ran into this issue when debugging the installation system, and have proposed an alternative solution at http://drupal.org/node/101829 which also attempts to make the file readable (i.e. if the web server owns settings.php).

Here's what I came up with from the other bug report, it's a slightly different technique compared to #4.

This patch modifies bootstrap.inc to not assume that settings.php is readable, and attempts to set the readable bit for settings.php if it's unreadable. It displays a correct error message if no settings.php file exists or if the file exists but isn't readable - previously there would be several warning messages and then an error about an unsupported database type. It fixes drupal_verify_install_file() to check that the file exists before attempting to check any of the other masks - before it resulted in a php stat error in drupal_install_fix_file() because fileperms() requires the file to exist.

All this talk about optimization made me realize that instead of wasting a stat() via is_readable() for a very rare situation (settings.php is unreadable or doesn't exist), we should just add a @ to include_once() in bootstrap.inc. It's run on every page so I think this is preferable.

It works like a charm for me, as long as I chmod it to 400, when I try 000 I get the following error:

Fatal error: Unknown function: file_directory_path() in /.../includes/common.inc on line 1429

But I'm not sure if that is possible to fix.

Btw; small string offset against head. Nice patch =)

Re-rolled against head.

@forngren - I tried it with 000 and didn't get an error. In any case this patch is an improvement imo.

I still gets the same error, but I'm pretty sure my host is causing that :( It's impossible to make a patch like this one to work on 'all' installations

Someone else will have to review this one or I'll will do it when I am coming across another server setup. You will need someone else to review it anyway, cause I would not dare to RTBC this patch, I'm still to junior.

-  include_once './'. conf_path() .'/settings.php';
+  @include_once './'. conf_path() . '/settings.php';

Is that @ required? It's considered ugly in the World of Drupal. ;-)

IMO we should either use an @ or put the include_once in an if (is_readable()) block if we want to handle the (rare) case of settings.php not being readable, otherwise there will be an error.

In #8 I thought that using an @ might be a slight optimization over is_readable and switched, so we can either switch back to is_readable or I can add a comment to explain the @. I see that I need to remove that extra space for the string concatenation too.

Okay, I realized that there also needed to be a check for db_url within conf_init() and added it, only later seeing that zoo33 had included the same check in the original patch. This includes a more specific error message by testing whether the settings.php is unreadable or doesn't exist.

@forngren: I was able to get the same error and fix it by modifying drupal_maintenance_theme() to include file.inc. It's needed due to the drupal_add_css() but there is at least one execution path in which it isn't included beforehand. This is fixed in the updated patch.

Clearer title.

#10 fixed my dupe issue at http://drupal.org/node/105697.

Re-rolling with file.inc change removed, as this was fixed individually by http://drupal.org/node/102599

The proposed patch is bit clunky so I'm not going to commit this yet / as is. Maybe we can come up with a slightly more elegant approach.

Updated version removes the use of @, which is slow, and checks the return value of include_once rather than checking if db_url isset.

I also removed the changes to drupal_verify_install_file(), which aren't entirely needed and will be posted separately (with some improvements).

Any more suggestions on this?

Sorry, one more minor change.

Chris, when you said "one minor change", did you intend to attach a new patch?

Looking at the code, I wonder if it wouldn't be better to make this more transparent and to explicit checks. Rather than relying on include_once to throw an error, maybe we can simply call file_exists() before doing the include_once?

Or better, yet, maybe conf_path() should not return an invalid settings file. conf_path() already does a bunch of file_exists() so we could one more ... Assuming that we'll /fix/ conf_path(), we'd now be able to write:

  if ($settings = conf_path()) {
    // ...
  }
  else {
    // No settings file found.
  }

To me, that looks like a more elegant approach. Thoughts?

Okay I changed it to use explicit checks rather than waiting for include_once to throw an error.

conf_path() returns a directory, not a settings file, so it can't really be modified. Plus we want it to return the directory even if the settings file is unreadable, because we may be able to chmod it (drupal_verify_install_file() tries to do this).

Needs to be re-rolled due to bootstrap.inc changes.

Important bugfix, that still needs to be done.

Good point by meba. I'm still moving this to D7 and let others decide whether this would be a won't fix.

Ehm, here we go...

Sorry, without further information this issue can only be marked as won't fix.

Feel free to re-open this issue if you want to provide further information. Thanks.