Require sites.php to opt-in for multi-site support/functionality

Re-rolled with git format-patch

There is another file_exists which can be removed:

  if (file_exists(DRUPAL_ROOT . '/' . $confdir . '/sites.php')) {
    // This will overwrite $sites with the desired mappings.
    include(DRUPAL_ROOT . '/' . $confdir . '/sites.php');
  }

I'm interested in this issue as a continuation of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades. Though it's out of scope for this issue, it would be good to take into consideration further possible restructuring, such as removing the "sites/default" directory entirely or making it optional by putting files and settings.php in the root. The fewer directories we make our users navigate the better in my opinion.

Let's do this.

We don't need an installer requirement here, we can simply rename the example.sites.php into sites.php.

Minor adjustments.

How may site admins provide custom sites/sites.php files without hacking core and/or being overwritten by core upgrades?

Hm. You're right. But without a sites.php and this adjustment for conf_path(), you're not able to install Drupal in the first place.

Chicken and egg situation, kinda.

I like the direction of this patch though. The entire magic and string building/conversion (of HTTP port, subdirectory, etc) that currently happens to auto-determine the sites directory really ought to be replaced with less magic and concrete mappings/definitions in sites.php. Mappings should be precisely declared (and we could add some wildcard support to that), and you even have the $http_host and $script_name variables to define dynamic mappings on your own.

So the installer should create a sites.php file (or warn of its nonexistence) in the same way that it now creates (or warns about) a settings.php file?

Rather than requiring sites.php, couldn't we just say that multi-site is only enabled if you have sites.php? If you don't, then sites/default is all you get (well, plus sites/all). No directory scanning at all. If you do have sites.php, then use that map instead of directory scanning. Make all directories keywords, not magic-naming based on domain.

That does still leave in the on file_exists() for sites.php itself, but I'm not super worried about that. It still saves us a fair bit of logic that almost no one uses.

the other part of this would be ditching sites/all and re-introducing top level module and theme directories

I'm sorta working on that already. #562042: Search for install profiles in sites/[all|site]/profiles folders, and move core profiles into /core/profiles is the key patch to allow for that (possible) change.

Aside from that, I like both suggestions in #15 and #16. I agree we shouldn't perform all of that rather expensive site detection and directory scanning unless a flag isn't enabled somewhere.

To continue on #17, I've created:
#1724252: Allow and encourage top-level directories instead of /sites/all/* directories

+1 to #15 or #16 instead of the proposal in the issue title. sites.php (and multisite) is weird, and I'd rather only foist it on someone who needs that level of complexity.

KISS.

Clarifying issue title, and tagging for architectural review.

Also, sorry, need to remove that "i/o" since the slash in it completely hi-jacks D6's taxonomy term autocomplete handler.

Note tat I am sure there are people using 'catch all' method for extra large installations - as long as sites/domain.com exists, it works. It should still be possible to use something like a wildcard?

+1 for #20. Simple and to the point, and it doesn't add any unnecessary complexity for users that do want the multisite functionality.

+++ b/core/includes/bootstrap.inc
@@ -467,14 +467,17 @@ function conf_path($require_settings = TRUE, $reset = FALSE) {
   $uri = explode('/', $script_name);
   $server = explode('.', implode('.', array_reverse(explode(':', rtrim($http_host, '.')))));
+  // @todo How much of this crazy code is still required with sites.php mappings?
   for ($i = count($uri) - 1; $i > 0; $i--) {
     for ($j = count($server); $j > 0; $j--) {
       $dir = implode('.', array_slice($server, -$j)) . implode('.', array_slice($uri, 0, $i));

Honestly, I'm happy to eliminate it entirely. I never actually understood it, and it's incompatible with dev/staging/prod domains anyway.

16 days to next Drupal core point release.

Well, @meba made a good point in #22 for why we might want to keep it.

It essentially allows you to do one of these two things:

A) Use a sites.php array key containing 'example.com'

B) sites/example.com

...as a wildcard/catch-all site to route any subdomain into the example.com site.

So let's keep that for now -- the idea is to only opt-in into the multi-site functionality anyway; there's not really a need to change/destruct the actual current multi-site behavior/functionality, at least not as part of this issue. ;)

Fair enough, follow-up issue. We still end up skipping it in the 99% case anyway.

Quick adjustment: Removed the @todo, since I think it's bogus/irrelevant/needless.

That said. The entire string-futzing and concatenation of $confdir in there is weird and needless.

The $confdir variable dates back to a time in which this site detection code was part of drupal_settings_initialize(), in which we are purposively setting the $confdir variable, so that it can be used within settings.php.

That $confdir variable is needless in sites.php, since it only ever contains "sites", and because sites.php is only supposed to return a domain-to-sitename mapping in the $sites array.

Hm. I'm not sure about $confdir being needless. We should run that past a few Drupal hosting companies. Barry Jaspan and Matt Cheney will both be at DrupalCon.

I'd prefer to keep this patch to only the minimum necessary to implement the change, personally, and discuss further optimizing the multisite pipeline elsewhere.

Sent an email out to Barry, Matt, and David Strauss, and directed them here to review this patch.

Also, unless I'm doing something wrong (always possible), this patch seems to break the installer (at least in the UI).

Can anyone else confirm?

(Wow, something is broken with the CSS for this page; the edit-comment-wrapper div is position:absolute, right on top of the rest of the comment form. Firebugging out the position:absolute makes everything useable again.)

Here are some thoughts.

It must be possible for the hosting environment to provide database credentials from outside of settings.php; users should never have to know or care what their db server/name/creds are. I believe Pantheon provides this info via environment variables, and Pressflow is patched to look for them there (I think David submitted a D8 patch for this). On Acquia Cloud, we provide an include file that we add to sites/default/settings.php on site import that loads the appropriate db creds for the environment (dev/test/prod) of the current page request. We support multi-site, so we provide a separate include file for each set of databases. e.g. If you create dbs site1 and site2 for multi-sites site1.com and site2.com (which means you have six actual mysql db instances, dev/test/prod for each), we provide two settings include files, site1-settings.inc and site2-settings.inc, that point to the correct db instance for the current page load. However, in that case, it is up to the user to create sites/site1.com/settings.php and include site1-settings.inc from there.

Cloud assumes that all settings.php files will exist in sites/*/settings.php, and we treat any such directory as a site directory: we create a files directory in our network filesystem for it, and symlink sites/*/files to it. It's a hack, of course. I'd be much happier to have a definitive way to know exactly what sites directory the site thinks exists, where its settings.php file is, what domain name(s) it is for, etc. Today, sites.php is optional. If we made it mandatory that it exist and list all possible domain names and directories containing settings.php, that would enable more intelligent/useful hosting functionality for supporting multi-sites.

Happy to talk more about this in Munich.

Barry, is that a verbose +1 to #27 or #28? :-)

Angie emailed me. I've looked over #28 and Barry's comments. This looks good to me, especially from a "convention over configuration" perspective.

As Barry mentioned, I'd really like to get environmental variable configuration support into core, but this hardly makes that any harder.

There is work going on over in #760992: Where should site specific modules, themes and settings be kept? using some of the principles from this issue.

Ok, so it seems like the change proposal is signed off.

However, @webchick is right in that this patch breaks the installer in some weird ways.

(I also managed to kill my dev site, since I forgot to create sites.php before testing the multi-site installer, hah! :))

Um. For some reason, I can't reproduce that issue anymore. Tried several installs for the default site and a multi-site, and they are working fine.

The cause seems to have been the missing sites.php file, which obviously screwed up the installer for me.

So I actually think we're back to RTBC, but it would be great if someone else could confirm this once more.

Sun, can you or someone else post steps to do the manual testing. It could be done by people attending core office hours.

Hm. I'd love to, but testing this is a bit more complex, as you need to set up additional DNS/host names and virtual hosts on your local machine, so the multi-site functionality is actually triggered... :-/

1. Set up one or two additional DNS/host names and virtual hosts that point to your existing D8 checkout/directory.
2. E.g., let's assume you've properly set up http://drupal.local, http://one.drupal.local, and http://two.drupal.local
3. Access http://one.drupal.local through your browser -- it should show you your existing site under drupal.local.
4. Access http://one.drupal.local/install.php through your browser -- it should show you an error message that Drupal is already installed.
5. Create an empty /sites/one.drupal.local/ directory
6. Access http://one.drupal.local/install.php through your browser -- it should show you an error message that Drupal is already installed.
7. Copy the file /sites/example.sites.php into /sites/sites.php (without changes)
8. Access http://one.drupal.local/install.php through your browser -- the installer should appear and a fresh installation should succeed - with a new settings.php and other files in /sites/one.drupal.local/.
9. Edit /sites/sites.php to add
   

   $sites['one.drupal.local'] = 'whatever';
   

10. Access http://one.drupal.local through your browser -- neither your original D8 dev site nor the freshly installed one should appear. Most likely, the installer should appear (but an error message is also possible).
11. Create an empty /sites/whatever/ directory
12. Access http://one.drupal.local/install.php through your browser -- the installer should appear and a fresh installation should succeed - with a new settings.php and other files in /sites/whatever/.
13. Access http://two.drupal.local through your browser -- I'm actually not sure what this will show, but yeah...

I hope these steps are correct as I did not perform them manually.

I have a local environment with an custom automatic DNS/virtual hosts (actually Nginx http servers) setup, so I did the tests as following:

drush dl drupal --package-handler=git_drupalorg
cd drupal-*
git checkout 8.x
cd ..
mv drupal-* 1055862

[created virtual hosts to 1055862.dn.net, one.1055862.dn.net and two.1055862.dn.net, all of them pointing to 1055862 dir]

[installed site from http://1055862.dn.net - no enough Drush Fu to do this by cli]
[installed with SQLite to sites/default/files/.ht.sqlite]

cd 1055862
wget http://drupal.org/files/drupal8.sites_.28.patch
git apply *.patch

Access http://1055862.dn.net - Shows my installed drupal site
Access http://one.1055862.dn.net/core/install.php - Shows "Drupal already installed"

mkdir sites/one.1055862.dn.net

Access http://one.1055862.dn.net/core/install.php - Shows "Drupal already installed"

cp sites/example.sites.php sites/sites.php

Access http://one.1055862.dn.net/core/install.php - Shows installation page
[installed site from http://one.1055862.dn.net]
[installed with SQLite to sites/one.1055862.dn.net/files/.ht.sqlite]

Added $sites['one.1055862.dn.net'] = 'whatever'; to end of sites/sites.php

Access http://one.1055862.dn.net - Shows the already installed site - FAIL
Access http://one.1055862.dn.net/core/install.php - Shows "Drupal already installed" - FAIL

mkdir sites/whatever

Access http://one.1055862.dn.net - Shows the wrong site, the first installed at http://1055862.dn.net - FAIL
Access http://one.1055862.dn.net/core/install.php - Shows installation page - FAIL

Access http://two.1055862.dn.net - Shows the site installed at http://1055862.dn.net - Don't know if this is the expected behavior
Access http://two.1055862.dn.net/core/install.php - Shows "Drupal already installed" - Don't know if this is the expected behavior

So reading that, this means the following problems exist with the current patch:

1. Conditions:
- the sites/default folder is hit by the 'example.com' domain.
- sites.php exists.
- sites.php has a $sites['sub.example.com'] = 'this-directory-doesnt-exist'; entry, i.e. a domain pointing to a non-existing directory.
Expectation: An error is shown, because sites.php is corrupt. I guess simply throwing an exception (without catching it) should be fine.
Actual result: The sites/default site, or example.com site, is shown.

2.
- the sites/default folder is hit by the 'example.com' domain.
- sites.php exists.
- sites.php has a $sites['sub.example.com'] = 'this-directory-exists-but-is-empty'; entry, i.e. a domain pointing to a directory, which contains no settings.php.
Expectation: The installer is shown to install the sites/this-directory-exists-but-is-empty site, or sub.example.com site, is shown.
Actual result: The sites/default site, or example.com site, is shown.

The fact that you get the example.com site if you hit sub.example.com with no entry in sites.php at all is actually wanted behavior.

@danillonunes: Awesome, thanks! :)

Even though you have some "FAIL"s (hah, great!) in there, I actually consider that as a positive test result for this patch, since we're not touching the actual site discovery process at all. It's perfectly possible that some of the expectations I stated are "my own" and do not actually match what the site discovery process is doing ;)

The important part is really just:

1) You were able to install.

2) You were able to enable multi-site handling after providing sites.php and install a second site.

So... I think it is safe to move this back to RTBC, since the only remaining task was to perform manual testing to make sure the multi-site functionality still works. And it does. :)

Hm. Not completely sure whether @tstoeckler's comment was/is in line with mine - is it, or not?

we're not touching the actual site discovery process at all

I guess you're right. I do think those are legitimate bugs, though, I think we should at least open follow-ups for that. Marking back to RTBC.

Ok, this seems to have been reviewed both conceptually by people with advanced needs, and through manual testing. I agree we should create follow-ups for the items identified in #40 and #41. Next logical step is #1757536: Move settings.php to /settings directory, fold sites.php into settings.php and then, unless I'm mistaken, I think we're done with Ye Great File Shufflage Of 2012? :)

Committed and pushed to 8.x.

We'll need a change notice for this.

@bjaspan + @David Strauss: Is there an issue already for the "hosting provider databases/settings.php override" of #32 already? I've learned what Pantheon is doing there and can only guess that Acquia's approach looks similar. I'd love to investigate and help to find a native/built-in solution in core to improve your situation, since I believe that hosted platforms are corner-stone for Drupal's adoption and success. Would be great to have a dedicated issue to discuss that.

We are now over thresholds on critical tasks, so this and other issues with outstanding change notices are blocking features in D8. Please fix.

Change notice added.

Here's the link:
http://drupal.org/node/1792924

It's also automatically added to the issue summary. :-) Also, I figure that notice can be edited in place once #1757536: Move settings.php to /settings directory, fold sites.php into settings.php gets sorted out.