Uncomment the inclusion of the local setting file

Since many projects now work with VCS, uncommenting this would result in overwritten file the next time Drupal core will get updated.

No, it would overwrite only the settings.default.php not settings.php. I do not agree with this issue. Just as many (if not more) sites don't need this and it will be expensive for them.

So,is this issue invalid?

I am just one opinion. Incidentally, it went in with #1118520: Add inclusion of a settings.local.php file in settings.php.

I feel this is an advanced configuration for those that know what they are doing so it should be commented out.

1. settings.php should never be committed to the VCS, so Since many projects now work with VCS, uncommenting this would result in overwritten file the next time Drupal core will get updated. is not a convincing reason I'm afraid :).
2. file_exists() in the condition that encapsulates the inclusion of that file is very inexpensive so it would not impact performance. — this is not true, any file system hit (any I/O, really) is very expensive.

So I'm afraid all of these things are really done intentionally, after very careful consideration.

settings.php and settings.local.php are never intended to leave a specific machine. They're not intended to be shared. If you want to share a bunch of settings that aren't machine-specific, create a settings.something.php file and commit that, and have each machine's settings.php include that.

In our deploy process we build the site every time, and we also sometimes use the settings.local.php - therefor we need the lines to be uncommented. I don't really see the case here for having them commented at all - if the settings.local.php exists it should be included.

No need to have the lines commented out as I see it.

Anyway, I am re-opening this ticket, and adding a small patch.

This is a major issue on any non-trivial application which may be installed and tested across many different environments in a development lifecycle. Certain configuration will always be infrastructure-dependent , and having to manipulate settings.php in every instance is painful. Aegir, for example writes settings.php itself and includes a facility like this.

file_exists() is cached in the statcache, and has absolutely negligible performance impact.

Let's quote https://secure.php.net/manual/en/function.clearstatcache.php

You should also note that PHP doesn't cache information about non-existent files. So, if you call file_exists() on a file that doesn't exist, it will return FALSE until you create the file. If you create the file, it will return TRUE even if you then delete the file. However unlink() clears the cache automatically.

The actual performance problem though would result into having less caching enabled by default.

IIRC there is a possibility to run a hook after settings.php is written, so a distribution could decide to unconditionally load a settings.local.php file e.g.

This is a decision of fast-by-default. By default Drupal puts e.g. mysql data directly in settings.php, so a settings.local.php is not needed.

If you need it in your workflow, just commit a 'default-settings' settings.php to GIT or add a distribution specific hook.

However as 90% of sites will never need a settings.local.php, giving them an unnecessary I/O hit, would be good to avoid.

If this should be a performance issue - could someone point out a test that shows that this really is a performance issue? I can not see how one file_exists could have any impact on the application as a whole.

A none existing file should not have any impact on caching - it's just a FALSE on a response.

And, there are a bunch of `file_exists()` in core already.

The difference is that this file_exists() would run for every request (even for page cache hits).

I think @catch and others have nice linkable anecdotes of the problems of just one file_exists()?

Off course it depends on what you have in the file you include if you use file_exists(). But a none existing file should have zero measurable impact on a application at the size of Drupal 8.

Testing locally (I know that not the best place to test, but better there than nowhere ;-)), I am getting no differences in performance if the file exists or not.

That's because your local webserver is not a multi-head webserver. Once you use multiple webservers, you need them to be in sync. That means a shared file system. Try using NFS or Gluster. Then it will be very noticeable.

It can also be a problem on a single server (think shared hosting) when the HDD/SSD is in heavy use and even doing that single file_exists() call can take many milliseconds.

I know that it is different environments. But the function itself should have none measurable impact on a drupal 8 site. But for fairs sake, I will try to get some live-testing up just for fun, I will come back with some results. If we have not tested the case, it seems strange, from my point of view, to assume that this minor thing would have an overall impact.

It has been measured, time and time again, and this is why Drupal 8 doesn't have any file_exists() calls in the critical path.

Ok, could you point me to the testing on this for Drupal 8 - I must have missed it.

Let's use tools for that: $ git log --all --grep="file_exists" points to #752730: Remove file_exists() during bootstrap which has an strace
and a blog entry: #752730-11: Remove file_exists() during bootstrap #752730-13: Remove file_exists() during bootstrap

I can not see the relevancy here? We are talking about one file_exist() - that should not have the impact that is raised in #752730. Or am I missing something obvious?

I tend to agree with MiSc here. I think Wim Leers use case is much less represented than the use case of a small team working on projects trying manage configuration. I think it would be just as easy to say that a team dealing with NFS could just comment that line settings.php.

I'd prefer the flexibility of having it uncommented by default. Yes it would require changes and education but overall I would prefer it this way and I know others would as well.

As for performance, from what I can see on most sites we are talking very trival performance issues. It's not like we don't include an .htaccess though by default when apache very clearly states "You should avoid using .htaccess files completely if you have access to httpd main server config file. Using .htaccess files slows down your Apache http server. Any directive that you can include in a .htaccess file is better set in a Directory block, as it will have the same effect with better performance." Yeah it's slower but it's easier to work with and helps folks when learning.

Having it uncommented by default and having an install option to install to settings.php or local.settings.php should be included in my opinion.

Updated patch

I'm agree with MiSc. For now settings.php contains two kinds of settings:
1) settings that can (and probably should) be same on different environments. For example CONFIG_SYNC_DIRECTORY, update_free_access, file_chmod_directory, file_chmod_file, public and private files path etc. Also different contrib modules like "fast 404" might require own settings in this file.
2) settings that can/should be different on different environments. Usually these are settings that are added to the file during the drupal installation: $databases array, hash_salt, install_profile.

On my opinion, drupal should have 2 fies with settings - settings.php and settings.local.php (or probably will be better to rename it to settings.env.php). In this case 1st kind of settings can be included to VCS. And new installation of site's instance will have all required configuration from the beginning. Also in this case result of file_exists() will be cached (or even this condition will be unnecessary)

But this solution requires some updates for installation process, because now settings always will be added to the settings.php file during the drupal installation.

Updated patch - I still see no valid reasons raised here to have this commented out.

+1 for uncommenting the file_exists() too.

If you run file_exists() against a non-existing file, there is no caching from PHP at all - the filesystem will check whether the file exists on every single request to Drupal, because it can never guarantee that it's not just been written. See the docs on https://www.php.net/manual/en/function.clearstatcache.php

How bad this is probably depends on the individual file system, but since settings.php is loaded for the internal page cache, even a small change can have a measurable impact on those pages.

Also since this issue was created, sites are built with composer which includes scaffolding for settings.php, so it's a lot easier to manage stuff like this in git.

Marking this works as designed.