Add inclusion of a settings.local.php file in settings.php

I like that.

I first thought, whether we should always create an (empty) settings.local.php in the install process to save the file_exists(), but since this is called exactly once per page-load it's probably not worth it. Especially as that would be extra trouble for a lot of users who don't actually use it.

Another question is whether this needs some form of in-code documentation or not. An example usage or something. I'm tending to say no, but not sure. Maybe it's sufficient to mention something like this in the upgrade docs.

Leaving this at needs review for some more opinions.

I think in-code documentation would be great. Not sure about everyone else, but for the longest time, the only thing I knew about most anything in settings.php was what was explained inline :)

Glad this got suggested, as I was just coming over to say the same thing!

Sounds good to me, but I agree with @patcon that we should flesh out the documentation here.

Whether it's a bug or a feature is outside the scope of my post, but...

drupal_rewrite_settings() has an undocumented $prefix variable passed into it. If specified, it concatenates the prefix to the settings.php file path. If we go with settings.local.php, then the $prefix var is a bit misleading -- there's no way we can write to that file without changing the logic of the function.

If we went with local.settings.php, that would be compatible with drupal_rewrite_settings(), but that may not be the intention.

Whichever way we go, it should probably be documented.

Well we have default.settings.php so local.settings.php seems to be consistent.

Well, I did some probing of drupal_rewrite_settings(), and I take back what I said earlier.

The $prefix option in drupal_rewrite_settings() does indeed work, but if you call it with the prefix, it will dump all settings to the $prefix.'settings.php file, not just the $settings variable that's passed in.

In short, using $prefix isn't used to add more config to settings.php, it's used to completely replace settings.php.

I'm resetting back to needs review in light of this.

The Aegir hosting system uses local.settings.php in sites/[site] to inject site specific settings. (The sites/[site]/settings.php file is built based on a global template, which uses $_SERVER vars.)

Yarrrrrrrrrr!

#9: drupal8.settings.local_.9.patch queued for re-testing.

Straight re-roll.

Thank you!

Regardless of the configuration system enhancements for D8, settings.php does not seem to go away anytime soon (and #1484690: Implement $conf overrides in the configuration system was fixed just recently to bring back conditional $conf overrides), so I'd still like to see this happening.

Anyone who does not like the additional filestat of the file_exists() can either remove the entire thing from settings.php or include it unconditionally - settings.php is fully customizable after all.

Reviewed and tested the patch and all looks good to me, one aspect I'm not sure of:

We currently warn uses if they leave settings.php writable after installation:

Configuration file Not protected
The file sites/default/settings.php is not protected from modifications and poses a security risk. You must change the file's permissions to be non-writable.

I wonder if we should include the same check on settings.local.php if it exists (since the security risk is the same). On the other hand, the only reason we warn for settings.php is because we explicitly request it to be writable during installation, which is not the case for settings.local.php, and we don't go checking other .php files for writability…

I don't see any reason to have this enabled by default, why not in there but commented out? It's a common pattern, but it's not worth the file system hit (file_exists() misses are not cached afaik) for sites that are never going to use it.

Alright, comment out by default.

Testbot issue. Passed on re-test.

All of the other options, that are commented out in settings.php use a leading '#', so let's do that here as well. The rest of them are all $conf overrides, so it's a bit different, but I think consistency makes sense here.

I like this patch! (please add this as a favorable Review) - agree with tstoeckler on leading # but except for that, I'm happy to see this patch.
Thank you Dave Reid, sun and mr.baileys!
-Kelly

Re-rolled against HEAD, and changed the inline comment syntax as requested.

Can't find anything wrong with that. I personally would have liked to see this uncommented by default, but I guess catch is right, that this it is not the default use-case.

Sorry can we change if existent to "if available", it's not exactly wrong but it's a bit flowery for a code comment I think.

Prego.

Thanks! Committed/pushed.

It might be better checking the existence of the file using is_file() and is_readable() instead of file_exists(). The latter will also return true if you have a directory named settings.local.php with the subsequent inclusion then throwing a warning. No big deal though as this is a very unlikely scenario.

We currently use file_exists() all over core in the exact same manner as this patch introduced. It might or might not make sense to turn a bunch of those into is_file() but that should be a different issue.

Follow-up issue: #1611686: Add test for unprotected settings.local.php

Maybe I'm not up on Drupal common practice, but shouldn't that followup simply be part of this issue? It would seem that this isn't complete until a test has been written, right? Like why have a separate discussion to follow for "those interested in writing tests"?

Or am I just too caught up in all that "agily" Definition of Done™ business? :)

patcon: You're probably right ... except in this case, the patch being developed in this issue has already been committed.

So the followup issue sort of becomes "Ooops, we forgot to add a test in x". ;)

Ah gotcha. regardless, you're all heroes. :)

The test we added in the other issue is a runtime requirements check on live sites. Could have been part of this, but it was an afterthought. There was no automated test for either issue, which is normally what gets the "no code without tests" rule.

Just a quick documentation note for the settings.local.php description: Instead of "and other things that should not happen on development and testing sites" it should probably read "and other things that should ONLY happen on development and testing sites," unless I'm misunderstanding the settings.local.php documentation's intent.

FYI: I opened #2419213: Rename settings.local.php to local.settings.php to be consistent with other filenames in hopes of making the file naming more consistent.

I've decided to use the PHP constant DIRECTORY_SEPARATOR instead of "/" because I am sharing this config between a GNU and a Windows server. I suspect the Windows server could interpret "/" correctly, at least in fairly modern versions of PHP. Can anyone confirm this?

Here's my snippet:

if (file_exists(dirname(__FILE__) . DIRECTORY_SEPARATOR . 'settings.local.php')) {
    include dirname(__FILE__) . DIRECTORY_SEPARATOR . 'settings.local.php';
}

@nerdcore: when building paths, you don't need to use DIRECTORY_SEPARATOR, forward-slash is fine. Linux uses forward slashes natively and Windows will accept either forward or backward slashes (PHP translates forward into backward for you so you can mix-and-match if you want). DIRECTORY_SEPARATOR is only needed when parsing a path given to you by the system.

#2535196: Uncomment the inclusion of the local setting file

This would be a nice to have back in D7.

I've ported the current code from D8's default.settings.php including having this section commented.

I think that's a good idea!

If we're going to do this, we probably need to test the file permissions on that file, like we did when we added the same feature to D8: #1611686: Add test for unprotected settings.local.php.

Ah, was completely unaware of that. It totally makes sense to do this in one shot for D7, thanks!

+++ b/sites/default/default.settings.php
@@ -584,3 +584,17 @@ $conf['404_fast_html'] = '<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML+RDFa 1.0//EN"
+# if (file_exists(__DIR__ . '/settings.local.php')) {

__DIR__ is not available in PHP 5.2 which is minimal required PHP version for Drupal 7.

Reroll of #43 in response to comment #47 (using file_exists(dirname(__FILE__) instead of __DIR__ because __FILE__ has existed since PHP 4.0.2)

Shouldn't this be include_once like in core?

boostrap.inc:drupal_settings_initialize() line 731 :

  if (file_exists(DRUPAL_ROOT . '/' . conf_path() . '/settings.php')) {
    include_once DRUPAL_ROOT . '/' . conf_path() . '/settings.php';
  }

Setting proper status; note that critique in #51 still applies...

I believe this should be on default.settings.php also. I'll reroll with changes suggested in #51.

Reverted back to the dirname instead of DIR changes from #49 and changes suggested in #51 re: include_once.

To comply to the remarks of #51, I rerolled #25 against Drupal 7.x

This patch in #60 does not apply cleanly for with Drupal 7.51. Rerolling the patch.

Here's the patch for Drupal >= 7.53

Test failures seem unrelated...

The 65: drupal-7.53-1118520.patch fails on drupal-7.64. I am trying to install the patch as part of an installation profile.

Can someone reroll the patch for drupal-7.64 or point me to directions on how I can do that? Thanks.

I've re-rolled the patch for Drupal 7.64.

@keramsey There are instructions for rerolling patches here: https://www.drupal.org/patch/reroll.

Thanks for the link and patch @zipymonkey!

Just applied patch in #72 to 7.67 for 2 of my sites (PHP 7.2 though) and it works great.
This is a pretty simple patch, is it not? I would love to see it committed.

I think we never addressed #45.

I Have tested and reviewed the #72 patch it works Perfect. Thanks.

Per #45, I ported the Drupal 8 hook_requirements changes to Drupal 7, in order to test the configuration file permissions.

I also looked at the current version of the tests/requirements in Drupal 8, but I decided against incorporating any of the other changes that have been made, such as adding an option to disable file permissions checking.

I tested to make sure the code displays the requirements correctly, but I didn't test it with patch #72 or with a non-empty settings.local.php file.

The patch in #72 is working for Drupal version 7.72

Rerolled the 7.x patch to work with Drupal 7.x-7.77.

@hmdnawaz @zipymonkey If you read #45 and #46, the tests I added are needed if this is to ever get committed.

I rerolled the patch to work with 7.79/7.80 and added the testings to system.install recommended by @solideogloria and @TravisCarden. I opened a merge request for review.

@zipmonkey

I have uploaded your latest patch here so that it can be used in projects from here.

@hmdnawaz You can get the raw patch file from the merge diff by adding ".patch" to the url. Example:

• Diff Link: https://git.drupalcode.org/project/drupal/-/merge_requests/586/diffs
• Patch Link: https://git.drupalcode.org/project/drupal/-/merge_requests/586/diffs.patch

@zipmonkey – I wanted to say the same but consider that there is no way (yet) to integrate commits in the Composer patch workflow.

Means https://git.drupalcode.org/project/drupal/-/merge_requests/586.patch can change if new commits get added and you wouldn't even notice when your local site runs with a different version of the patch while your live site runs on another – and maybe breaks because of it.

https://www.drupal.org/files/issues/2021-04-23/drupal-7.x-1118520-88.patch doesn't change.

See https://drupal.stackexchange.com/q/298567/15055 for example where this has been asked already.

It would be nice to be able to pin the MR patch to a certain commit. Like https://git.drupalcode.org/project/drupal/-/merge_requests/586-[LATEST-C... and later if more commits come in https://git.drupalcode.org/project/drupal/-/merge_requests/586-[UPDATED-... for example.

I rerolled the patch and attached a copy of the updated patched file.