Problem

  1. The new directory layout in D8 hides most parts of the advanced multi-site feature from users that don't need it, which is a good thing:

    Top-level /modules, /themes, and /profiles directories can be used for site-wide extensions.

  2. However, settings.php must still be placed into /sites/default/settings.php.

    This is unnecessary and forces the multi-site concept onto users that shouldn't need to care about it.

  3. In addition, because the multi-site concept is enabled by default, additional calls to file_exists() are required on all sites (whether sites.php exists), even if there is only one site, which harms performance.

Goal

  • Simplify the bootstrap by requiring /settings/settings.php to exist, and explicitly opt-in for the multi-site functionality.

Proposed solution

  1. Require a top-level, global /settings/settings.php to exist in any case (set up by the installer).

  2. Merge /sites/sites.php into the top-level /settings/settings.php, and turn the $sites variable into an explicit flag for enabling the multi-site functionality.

    This means:

    1. If the root /settings/settings.php does not define the $sites variable at all, then there is no multi-site functionality and no site directory discovery process.
    2. If the root /settings/settings.php defines an empty $sites = array();, then the multi-site functionality is enabled and the site directory is discovered on every request.
    3. If the root /settings/settings.php defines site aliases in the form of $sites['localhost.example'] = 'example.com';, then the specified site aliases are resolved during the site directory discovery (no change).

    This also means:

    1. When the multi-site functionality is enabled, then the root /settings/settings.php acts as if it was a /sites/all/settings.php, which is a concept that does not exist thus far — i.e., the root /settings.php can contain shared settings for all sites.
    2. The root /settings/settings.php is always loaded first. The site-specific sites/foo/settings.php has access to all of the global variables and is able to override them in a granular way.
  3. → (Implicitly) Eliminate /sites/default. (DX++)

    If the multi-site functionality is not enabled, the root directory of your Drupal site becomes the site directory itself:

    /files
    /modules
    /profiles
    /themes
    /settings/settings.php
    

    /files is moved (back) to the document root (as in D6 and below).

    /sites no longer exists by default.

    Note: This patch has to retain /sites/default/settings.php due to #2206501: Remove dependency on Drush from test reviews

  4. Introduce a new Site singleton class to replace conf_path().

    This is required, because if the root directory is the site directory, then a string concatenation like the following would result in an absolute filesystem path:

    // If the sire directory path is empty (root directory), then the resulting
    // filesystem path would become absolute; i.e.: "/some/file"
    unlink($site_path . '/some/file');
    

    To prevent that, all calls to conf_path() are replaced with the following:

    $site_path_relative = Site::getPath();
    $site_path_absolute = Site::getAbsolutePath();
    $some_file_relative = Site::getPath('settings.php');
    $some_file_absolute = Site::getAbsolutePath('settings.php');
    
    // The argument is a relative subpathname, so this works, too:
    $some_file_relative = Site::getPath('files/translations/foo/bar.txt');
    
  5. Move /sites/default/default.settings.php into /core/default.settings.php, so it is updated when Drupal core is updated.

.

.

.


Original summary by @naxoc

This is a split-out from #760992: Where should site specific modules, themes and settings be kept? to move the settings file to the root folder where modules/ themes/ and profiles/ are also going at some point.

It would go for "default" - but not multisite settings files that would still go in the sites folder.

The existence of a sites.php can trigger trying to find multisite settings as suggested in #1055862: Require sites.php to opt-in for multi-site support/functionality

CommentFileSizeAuthor
#273 1757536-nr-bot.txt148 bytesneeds-review-queue-bot
#258 interdiff-1757536-242-258.txt45.71 KBricardoamaro
#258 issue1757536_4.patch48.59 KBricardoamaro
#257 interdiff-1757536-242-257.txt45.17 KBricardoamaro
#257 issue1757536_3.patch49.13 KBricardoamaro
#256 interdiff-1757536-242-256.txt37.42 KBricardoamaro
#256 issue1757536_2.patch52.43 KBricardoamaro
#254 interdiff-1757536-242-254.txt37.01 KBricardoamaro
#254 issue1757536.patch52.47 KBricardoamaro
#251 core-n1757536-250.patch26.39 KBdafink
#242 move_settings_php_to-1757536-242.patch110.19 KBeporama
#240 move_settings_php_to-1757536-240.patch0 byteseporama
#233 1757536-233.patch108.02 KBvprocessor
#214 1757536-213.patch83.07 KBderheap
#207 1757536-206.patch83.89 KBderheap
#205 1757536-205.patch83.36 KBderheap
#197 1757536-197.patch97.11 KBderheap
#191 1757536-191.patch69.85 KBderheap
#186 1757536.186.diff70.21 KBderheap
#184 1757536.site_.184.patch68.83 KBderheap
#167 site.167.patch94.02 KBsun
#166 interdiff.txt3.91 KBsun
#166 site.166.patch94.07 KBsun
#165 drupal8-site-1757536-165.patch93.19 KBJalandhar
#163 interdiff.txt907 bytessun
#163 site.163.patch93.19 KBsun
#162 interdiff.txt34.29 KBsun
#162 site.162.patch92.62 KBsun
#159 interdiff.txt3.2 KBsun
#159 site.159.patch89.05 KBsun
#153 site.153.patch88.44 KBsun
#152 interdiff.txt1.89 KBsun
#152 site.152.patch88.43 KBsun
#150 site.150.patch87.88 KBsun
#147 site.147.patch87.48 KBsun
#145 site.145.patch87.42 KBsun
#140 interdiff.txt9.38 KBsun
#140 site.140.patch91.18 KBsun
#139 interdiff.txt11.45 KBsun
#139 site.139.patch84.08 KBsun
#130 interdiff.txt940 bytessun
#130 site.130.patch83.34 KBsun
#128 interdiff.txt30.55 KBsun
#128 site.128.patch83.55 KBsun
#124 interdiff.txt8.39 KBsun
#124 framework.site_.124.patch58.46 KBsun
#119 interdiff.txt914 bytessun
#119 framework.site_.119.patch57.7 KBsun
#117 interdiff.txt3 KBsun
#117 framework.site_.117.patch57.79 KBsun
#100 interdiff.txt3.95 KBsun
#100 framework.site_.100.patch56.82 KBsun
#98 interdiff.txt13.34 KBsun
#98 framework.site_.98.patch53.73 KBsun
#96 interdiff.txt18.31 KBsun
#96 framework.site_.96.patch52.97 KBsun
#92 simpletest-help.interdiff.txt3.75 KBtstoeckler
#86 interdiff.txt763 bytessun
#86 framework.site_.86.patch73.78 KBsun
#84 interdiff.txt32.46 KBsun
#84 framework.site_.84.patch73.7 KBsun
#82 interdiff.txt1.76 KBsun
#82 framework.site_.82.patch56.57 KBsun
#80 interdiff.txt4.15 KBsun
#80 framework.site_.80.patch56.13 KBsun
#75 interdiff.txt2.85 KBsun
#75 framework.site_.75.patch55.86 KBsun
#71 interdiff.txt19.08 KBsun
#71 framework.site_.71.patch55.76 KBsun
#69 interdiff.txt29.85 KBsun
#69 framework.site_.69.patch57.95 KBsun
#68 interdiff.txt21.61 KBsun
#65 1757536-65-settings-file.patch110.77 KBtstoeckler
#65 1757536-60-65-interdiff.txt46.84 KBtstoeckler
#60 1757536-60-settings-file.patch72.03 KBtstoeckler
#58 1757536-58.patch71.99 KBtstoeckler
#49 framework.settings.49.patch20.83 KBsun
#44 framework.settings.44.patch20.61 KBsun
#42 framework.settings.42.patch21.42 KBsun
#34 framework.settings.34.patch21.49 KBsun
#34 interdiff.txt2.58 KBsun
#32 framework.settings.32.patch20.66 KBsun
#32 interdiff.txt14.71 KBsun
#10 framework.sites_.10.patch11.34 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

I conceptually support this very much, but not sure if it's possible given the permission changes that need to be done to settings.php and sites.php to protect it. Needs testing.

naxoc’s picture

Yeah, the settings file in the root folder might cause some permissions problems. It also (potentially) makes the code in conf_path have to either return a path with a trailing slash or some other workaround.

I really dont like the "default" folder, so here is another suggestion:

We put settings.php in sites/. That will only work for a single-site setup. The minut someone wants to do a multisite, they have to make a sites.php (same location) and move all sites into folders.

Is that even more weird than having a default/ folder?

mariomaric’s picture

Another idea: sites/default -> site

So we would have in root folder, based on other related issues, this situation:

- core/
- modules/
- profiles/
- themes/
- site/ - settings.php + files/
- .htaccess
- README.txt
- example.gitignore
- example.sites.php - Activate for multisites.
- index.php
- robots.txt
- web.config
lpalgarvio’s picture

site & sites at the root folder don't mix very well.

i propose instead:
- core/
- modules/
- profiles/
- themes/
- default/ - default site
- sites/ - multi site
- index.php

or

classic behavior:
- core/
- modules/
- profiles/
- themes/
- sites/ - multi site
- sites/default - default site
- index.php

mariomaric’s picture

Of course it wouldn't be nice mix, but, in that case site folder will not exist at all - every site will be obligated to define its own root in site.php file - and documentation (in site.php e.g.) could recommend (or dictate?) to place those root directories into sites folder - and completely ignore site directory.

naxoc’s picture

I vote for:

sites/settings.php
sites/sites.php

The existence of sites.php makes drupal check for other folders and ignore settings.php. If an install has multisite - each site has to go in a separate folder.

Would that work?

Crell’s picture

"This file means something, unless this other file is there and then it doesn't mean anything but this other one does". That seems even more confusing to me than "if you're not doing multisite, just ignore this 'default' word in the path for now."

Let's also remember another reason that we moved everything to /core in the first place last year: It means the top level directory contains /core (don't need to backup) and /sites (all of your stuff). Moving sites/all back up to the top level has already broken that simplicity; moving settings.php to the root just breaks that even worse. I can't say that fills me with joy.

webchick’s picture

The simplicity is now that /core is the only directory you need to worry about. Everything else is your stuff.

sun’s picture

If I understood @catch correctly, then he'd like to:

  1. Move /settings.php to the top-level.
  2. Require /settings.php to exist in all cases. (removing one or more filestats)
  3. Have multi-site functionality enabled through the top-level /settings.php with an explicit flag along the lines of:
    /**
     * Flag to enable multi-site functionality.
     *
     * Uncomment this line to enable Drupal's multi-site functionality,
     * which allows you to serve multiple different sites from the same
     * code-base.
     */
    # $multisite = TRUE;
    

    Or alternatively - just simply move the entirety of $sites and sites/sites.php into /settings.php; meaning, if $sites is defined in it, the multi-site functionality gets enabled, otherwise nada.

Aside from that, I don't think that touching/changing the /sites/default directory in any way should be part of this issue and discussion. The only relevance would be that /sites/default/settings.php would be moved into /settings.php for the most parts, but not fully. The important difference is that the top-level /settings.php would equal a /sites/all/settings.php, which doesn't exist in our current multi-site directory concept/setup.

sun’s picture

Status: Active » Needs review
FileSize
11.34 KB

And here's how that would look like. :)

The installer most likely needs additional tweaks, but applying this patch to an existing site and copying /sites/default/settings.php into /settings.php works :)

lpalgarvio’s picture

from what i understood so far, i have mixed reactions.

so we're having a single file for general configuration, the top-level default.settings.php (today present in D7 at /sites/default/default.settings.php), which would include any default parameters to be used in new sites, plus the settings for multi-site, thus not requiring an additional sites.php (today present in D7 at /sites/sites.php, working as a means to add alias, as exempt of auto-discovery).

the pros: a single file to be included, no need to lookup/scan for another file. might also simplify things for new users (but are new users to drupal, people that will use multi-sites right out of the box?).
the cons: if that default.settings.php changes periodically when core changes, you have to either edit it again, do a diff, or replace it -- with the last one, you loose the changes you did when extracting the core files from the archive (extra care needed, like for .htaccess, robots.txt and web.config). if you ignore changes from core, drupal might not load (some crucial changed config), if you loose your changes, your multi-sites might not work. plus, keeping sites.php, it's just one file.

moving on, as such, this top-level settings.php, like the previously proposed changes to sites.php, would have a $sites array, as present in D7, but this time not an exempt, but a requirement?, so that drupal wouldn't have to scan all the time for other settings.php in each sub-level directory.

the pros: avoiding extra lookups/scans with each bootstrap is a good thing.
the cons: some users might actually want that behaviour. could be solved with a $multisite_autodiscovery boolean to enable/disable auto-discovery/scanning/lookingup for sites (defaults to FALSE).

and, as specified before, the proposed $multisite boolean added to it, which enables/disables multi-sites. if it's set to true, multi-site environment works (defaults to FALSE).

the pros: as most drupal installations have only the default site. less code to be parsed each time.
the cons: none i believe.

in terms of variables, that would sum up to:
$multisite boolean to enable/disable multi-site (defaults to FALSE).
$multisite_autodiscovery boolean to enable/disable autodiscovery of sites (defaults to FALSE).
$sites array to specify sites (whether exempt or requirement).

and now a question:
then each site has it's own settings.php file, like before -- including the default site (so that this top-level default.settings.php acts like in D7) -- or the default/main site settings.php and the top level default.settings.php would be the same thing? a little lost there - but previous comments here and on #760992: Where should site specific modules, themes and settings be kept? point to that.

sorry for crossing over issues. i'm not sure where this is headed or that everyone was thought on all points.

edit:
picking up where i left before,

case 1 (/sites/default moves to /default, default.settings.php/sites.php moves to /):

core/
modules/
profiles/
themes/
default/ (default site)
         files/
         settings.php
sites/   (multi site)
         abc.example.org/ (another site)
                  files/
                  settings.php
default.settings.php
sites.php
index.php, etc

default site is outside /sites, no longer requiring the /sites dir, which can be entirely optional, and all config files are on top-level. a strong fit.

case 2 (/sites/default moves to /default, default.settings.php/sites.php stays at /sites):

core/
modules/
profiles/
themes/
default/ (default site)
         files/
         settings.php
sites/   (multi site)
         default.settings.php
         sites.php
         abc.example.org/ (another site)
                  files/
                  settings.php
index.php, etc

although /default is not inside /sites, the configs are inside it. does not add up much. also, if /sites dir is optional, it is in reality infact still required.

case 3 (/sites/default remains at same place, default.settings.php/sites.php moves to /):

core/
modules/
profiles/
themes/
sites/ (multi site)
         default/ (default site)
                  files/
                  settings.php
         abc.example.org/ (another site)
                  files/
                  settings.php
default.settings.php
sites.php
index.php, etc

using the same structure in D7, but having the config files on /. would make more sense to keep configurations all together, next to the default site (ie on /sites). if we look at /sites as optional, does not makes sense.

case 4 (classic behaviour, all config in /sites):

core/
modules/
profiles/
themes/
sites/ (multi site)
         default/ (default site)
                  files/
                  settings.php
         abc.example.org/ (another site)
                  files/
                  settings.php
         default.settings.php
         sites.php
index.php, etc

clear separation of sites and configs, from user modules/themes and core. but less easy for new users. a strong fit.

case 5 (default.settings.php = settings.php for default site, everything at /):

core/
modules/
profiles/
themes/
files/
sites/ (multi site)
         abc.example.org/ (another site)
                  files/
                  settings.php
settings.php
sites.php
index.php, etc

everything is mixed and there is no clear distinction of default site vs stuff for all sites vs multisite. this is horrible IMO.

sun’s picture

Wow... ;) Let me try to address some concerns:

a single file for general configuration, the top-level default.settings.php (today present in D7 at /sites/default/default.settings.php), which would include any default parameters to be used in new sites, plus the settings for multi-site, thus not requiring an additional sites.php (today present in D7 at /sites/sites.php, working as a means to add alias, as exempt of auto-discovery).

Mostly correct.

  1. The top-level settings.php is always used and required. If you don't use multi-site, then it is the only settings.php.
  2. If you use multi-site, then the settings of the top-level settings.php can be overridden by a site-specific settings.php (including sites/default/settings.php for the default site). This makes the settings.php override consistent to all the other sites/all override logic. All variables that are defined in the top-level settings.php are accessible in the site-specific settings.php, so you can precisely adjust and manipulate what needs to be overridden.
  3. Previous point also means that you can share the same global settings through the top-level settings.php and only specify the actual different settings in the site-specific settings.php. Alternatively, you can also decide to leave the top-level settings.php empty (just declare $sites) and retain entirely different settings.php files per site. Freedom of choice. (I manually tested both already.)
  4. sites.php is completely merged into the top-level settings.php. To enable multi-site, you have to at least declare the $sites variable in settings.php. The $sites aliases variable can be empty. The site auto-discovery is still in place. (but only invoked when multi-site is enabled)

cons: if that default.settings.php changes periodically when core changes, you have to either edit it again, do a diff, or replace it -- with the last one, you loose the changes you did when extracting the core files from the archive (extra care needed, like for .htaccess, robots.txt and web.config). if you ignore changes from core, drupal might not load (some crucial changed config)

That's a good point. Although the situation exists today already, since default.settings.php lives in /sites/default — so even with the /core directory change, you have to manually ensure to update that file.

We could address that by moving default.settings.php into /core/default.settings.php. So the default.settings.php file is updated when core is updated. The file wouldn't be directly apparent anymore, but then again, that might not be too important.

What do you think about that?

(btw, doing this would pretty much empty out the entire /sites folder. The installer would create /sites/default, but we might be able to get rid of the /sites directory in the download package. May I say that this sounds exciting to me? :))

top-level settings.php [...] would have a $sites array, as present in D7, but this time not an exempt, but a requirement?

It is only a requirement if you want to use multi-site. 99% of Drupal users don't use multi-site.

(That is, because it is utterly complex to set up properly — and within those usages, the 80% use-case of multi-site is to just use the same code-base for multiple sites [without actually sharing tables/data between them], which is something that is a lot easier to achieve with pure git and devops practices nowadays. So IMHO we're talking about multi-site usage statistics of 20% of 1% in the first place...)

---

Lastly, with regard to the site directory changes you have in mind for the default site, I'm not comfortable with that. Doing so would ultimately make the multi-site scenario very confusing. As mentioned before, I think that moving /sites/default to a different/top-level directory should be an own issue and separate discussion.

lpalgarvio’s picture

awesome logic there =D
i wasn't aware that the parsing of settings.php was so advanced. that's great!

That's a good point. Although the situation exists today already, since default.settings.php lives in /sites/default — so even with the /core directory change, you have to manually ensure to update that file.

We could address that by moving default.settings.php into /core/default.settings.php. So the default.settings.php file is updated when core is updated. The file wouldn't be directly apparent anymore, but then again, that might not be too important.

What do you think about that?

(btw, doing this would pretty much empty out the entire /sites folder. The installer would create /sites/default, but we might be able to get rid of the /sites directory in the download package. May I say that this sounds exciting to me? :))

i wouldn't know the implications in the code, but it your point of view looks good!

multi-site clarified.

regarding the default site, i was thinking it was being moved as well, due to the change to settings.php.
since it's not, i think it doesn't need to be discussed =)

looking forward to test the new changes!!

sun’s picture

Title: Move settings.php for sites/default to the drupal root folder » Require settings.php to exist in the root folder and to explicitly opt-in for multi-site functionality
Assigned: Unassigned » sun
Issue tags: +Performance, +Needs architectural review

Coolio, glad to hear that your concerns are resolved. :)

This is quite a massive architectural change, so for now I'm not going to try to make this patch work just yet, but instead, tagging this issue accordingly, wrote a proper issue summary reflecting the current proposal, and also re-titling the issue accordingly. ;)

As soon as there is consensus that this is a good idea I'll happily make this patch work.

klonos’s picture

Seems like a plan to me. It achieves the directory structure simplification that most seem to want to see here and at the same time tries to keep old functionality where possible for those advocating "the old ways". I say it's a decent compromise. The issue summary speaks for itself and I don't disagree with any of the points made there. Thanx.

I'd set this to RTBC straight away so it could move on, but I'd really like to see what a person that worries about how this would hurt multisite installation UX has to say and if they insist on previous remarks/comments made in this issue. Anybody?

Crell’s picture

I'm tentatively OK with this. It's not quite as bad as I feared. :-) Some comments:

1) If we are keeping sites/default/modules and sites/default/files, but then having /settings.php, that puts one site's worth of "stuff" in multiple places rather than a single place.

2) What of the epic battle over .gitignore?

3) A lot of people, including drupal.org, are now using a settings.local.php file as well for server-specific overrides. We should think through how that fits into this model. I could see it still existing (and then we should discuss if we want to directly support it) or I could see an argument that the dual-settings.php setup renders it obsolete. I'm not sure here, but I'd like to see us think this through before we proceed.

sun’s picture

1) If we are keeping sites/default/modules and sites/default/files, but then having /settings.php, that puts one site's worth of "stuff" in multiple places rather than a single place.

That's a good point. However, I think there's nothing that would prevent you from putting your global settings.php into sites/default - e.g., you can move your top-level /settings.php into /sites/default/settings.php and replace the original with:

<?php
// Load /sites/default/settings.php.
// Only required when not defining $sites in this file.
require DRUPAL_ROOT . '/' . $conf_path . '/settings.php';

# EOF

Additionally, I think that the change of #1724252: Allow and encourage top-level directories instead of /sites/all/* directories made the situation of having "one site directory that contains all your custom stuff" a bit fuzzy anyway already. Regardless of that, I don't think there's anything that would block anyone from trying to put everything into one place.

2) What of the epic battle over .gitignore?

Not exactly sure what you mean with that? The top-level /example.gitignore probably needs a new entry for the top-level /settings.php, yeah.

3) A lot of people, including drupal.org, are now using a settings.local.php file

The settings.local.php approach is not affected by this change. The pattern is not actually part of conf_path() or Drupal's bootstrap — it is merely an addition within settings.php, which is commented out by default, but can be enabled - in all settings.php files, regardless of where they are located:

/**
 * Load local development override configuration, if available.
 *
 * Use settings.local.php to override variables on secondary (staging,
 * development, etc) installations of this site. Typically used to disable
 * caching, JavaScript/CSS compression, re-routing of outgoing e-mails, and
 * other things that should not happen on development and testing sites.
 *
 * Keep this code block at the end of this file to take full effect.
 */
# if (file_exists(DRUPAL_ROOT . '/' . $conf_path . '/settings.local.php')) {
#   include DRUPAL_ROOT . '/' . $conf_path . '/settings.local.php';
# }

(already in HEAD - pasted from default.settings.php)

Crell’s picture

Wow, I totally missed settings.local.php going in! Nifty!

Yes, /modules already makes that iffy, which is one of the reasons I didn't like that change. Putting settings.php up top only makes that worse. In Drupal 7, if you did things properly you knew "/sites is yours, otherwise don't touch". In D8 until recently, "/core is not yours, /sites is yours". With this change, it would be "/core is not yours, otherwise it could be yours". That's one of the reasons I'm still somewhat skeptical of this change.

Is the next issue going back to /files yet again? I worry that this piecemeal approach is going to leave us with something that makes no sense to anyone who wasn't in all of these issues (and not even all of us).

I cannot speak for the big hosters; I'm pretty sure Palantir's setup can adapt to this if we end up going this route.

sun’s picture

With this change, it would be "/core is not yours, otherwise it could be yours".

I actually wonder whether all these core vs. mine thoughts and habits just need some super-trivial re-adjustment? I mean, isn't it as simple as this:

D7: > tar zcf mysite.tgz sites/default/

D8: > tar --exclude ./core zcf mysite.tgz

? :)

joshk’s picture

+1 for the concept. Having settings in a more obvious/common place for the common case is a Good Thing(tm).

joshk’s picture

In terms of Pantheon specific stuff, this all looks sane enough to handle.

I will say that from an elegance/taste standpoint, I would like to really lean towards supporting the common case very well. I'm not up to speed on where the re-organization is currently, but IMHO something like:

/core
/modules
/themes
/files
settings.php

As a root makes good sense to me. In the common case where the install is serving a single site, this makes it pretty obvious where everything should go. As I understand it, this is like moving sites/all or sites/default to the root, which again seems like the most obvious thing for the common case.

Multisites could add support by having a /sites directory, which could trigger a code-path to do the traditional matching logic and allow 'sites/sitename/stuff' to work the same way.

The file storage path will always be configurable anyway, but putting it at the root is intuitive to web people in general for the common case. Anywhere else is not.

klonos’s picture

#22 makes sense to me too when it comes to newcomers. If that was the file/folder structure when I first started digging into Drupal, it would have made things a lot easier than having to remember where everything should (and shouldn't) go. So ++ from me.

tstoeckler’s picture

I agree with the direction taken here, but let's leave /files out of this for now, please. Equal to /modules and /themes currently and /settings.php with this patch, /files would contain files *for all sites*, i.e. what was sites/all/files before (except we didn't have that). I think it would be really cool to support that, but that should definitely be a dedicated issue. It seems that will be quite a bit more complicated than /settings.php which is equivalent to previously sites/all/settings.php (which we also didn't have before).

klonos’s picture

I think that with the direction we took it does make sense to have /files (in place of /sites/all/files) but re-purpose this for single-site setups and always keeping in mind the ease of UX. If this ends up to be a multi-site installation, then /files would act as a *for all sites* and "private" per-site files could live under /sites/[sitename]/files. Does that make sense?

PS: I agree that the /files move deserves it own issue. Anybody already filed one?

lpalgarvio’s picture

#22 - #25
this issue is not covering /sites/default/files vs /files.
as per what was discussed before, /files stays inside the default site directory (/sites/default/files) .

feel free to create one issue i guess, but i'll oppose the idea =P

about the shared files, IMO it will just create confusion among users, for something that would have little usage (complex by nature, as each site typically contains different files, images, nodes, etc). advanced users can still create this kind of feature manually.

the current directory layout is this:

/core (drupal core)
/modules (user modules, previously at /sites/all/modules)
/themes (user themes, previously at /sites/all/themes)
/sites (user sites)
   /default (default site)
      /files
      /settings.php (settings.php for default site)
   /example.org (another site)
      /files
      /settings.php (settings.php for another site)
settings.php (top-level settings.php)
webchick’s picture

What joshk says in #22 makes a lot of sense to me (leaving aside the files question, per tstoeckler in #24). I interpret that as "Move settings.php to the root, but make the multisite logic trigger contingent on whether there's a "sites" directory there." That's certainly much easier than fiddling with a 500K PHP file, and it would also be backwards-compatible. When we did this with sites.php a few patches ago, that change screwed up a number of people in IRC.

The tricky part about that is I don't think we could do it unless we also dealt with the files question. ;) Because it would mean that root would have to do everything sites/default is currently doing. (It already basically does want sites/all does.)

Damien Tournoud’s picture

I like the default site at the top-level, but it needs to be completely at the top level (ie. sites/default doesn't exist at all anymore).

Given that a having a default site rarely makes sense in a multi-site deployment, I would argue that we should kill the concept of default site completely and only support:

  • Mono site: in that case it makes sense to have /files as the files directory
  • Multi site: in that case, you don't want a default site at all, and you will have /sites/site1 and /sites/site2, with the common modules at the top-level
catch’s picture

I interpret that as "Move settings.php to the root, but make the multisite logic trigger contingent on whether there's a "sites" directory there."

I'd really like to avoid any file system checks for discovery of multisite, just whether there's a $sites array in settings.php is fine IMO. The sites.php change did catch people out, but so do lots of other things, this just happens to be a change that affects people's local installs of Drupal 8. If we ever invite Rasmus back to DrupalCon it'd be nice if he wasn't bringing up the same issues he pointed out in Szeged in 2008 yet again...

Some multisites are doing files/sitename instead of sites/sitename.domain/files because it's a nicer URL path, not sure whether we want to go into that here at all (and it does mean one site is split all over the place even more).

@klonos brings up an interesting idea with a shared files directory. There are several cases where that would be useful - for example if you install google analytics module it gives you an option to cache the js file locally and serve it from your infra instead of google's. However if you've got a multi-site install then you could end up with 1,000 different copies of that same file in different directories. The same could be done for things like imagecache versions of theme logos or icons and similar. That's sites/all/files rather than sites/default/files though..

mikeytown2’s picture

CSS/JS aggregates could use a shared files directory #1082928: Unified Aggregation Directory(AdvAgg).
Boost could use a shared files directory as well; getting rid of the requirement for the cache folder being created and thus making it easier to use for beginners.

In terms of getting rid of sites/example.com/settings.php I would be all for that. Currently we do not have a 7.x multisite here at work but in 6.x we have all of our settings.php files load up 2 master ones; thus sites/example.com/settings.php on our large multisite is just a couple of lines of code. We have a database settings.php file and a general settings.php file. The general one contains things like memcache (#1324812-7: Unify cache bin prefix: proposal to unified convention), reverse proxy, and the usual things found in settings.php; for us these never change per site. In our database settings file we use db array notation in D6.

IMHO whats needed is a way to override find_conf_path() so if you don't want to use the file system you don't need to. In drupal 6 for us conf_path() could have been setup so it doesn't hit the filesystem; and if you're wondering we have around 1300 domains currently on a single code base with a unified settings.php file.

sun’s picture

I moved forward with the patch already, and after hacking for some hours, it turned out that I might want to withdraw my statement/desire to move the top-level /files directory discussion into a separate issue...

That is, because without the elimination of /sites/default for the mono-site case, the installer essentially has to take two conf_path()s into account; i.e., conf_path() to the top-level /settings.php and the conf_path() to /sites/default.

By eliminating /sites/default for the mono-site case, only one conf_path() remains (in all cases). Thus, if we want to incorporate that directly, I suspect that the total patch/change gets a bit simpler.

Regardless of that, I definitely want to point out the resulting DX/UX of this change with regard to installing Drupal, which didn't really came up here so far, but is pretty important to figure out for this patch.

While hacking on this, I determined that there could be some automations baked into the installer, which would differ from the regular runtime site discovery behavior. To mention a couple of options:

  • No /settings.php, no prepared site directory:

    A mono-site will be installed. There will only be a single /settings.php at the top-level, and no sites directory.

  • No /settings.php, but a /sites/example.com directory, invoking installer on http://example.com:

    /settings.php will be prepared to enable multi-site. Actual site and settings.php lives in /sites/example.com/settings.php.

  • /settings.php exists, but no /sites/example.com directory, invoking installer on http://example.com:

    - same as previous - and /settings.php is adjusted to uncomment the $sites variable if required.

  • /settings.php exists and has $sites aliases defined, invoking installer on http://example.com::

    - same as previous - but installing into the corresponding site directory on positive match.

However, I'd like to keep it simple.

I suggest to do and support the following, essentially keeping the logic in line with the runtime behavior:

  1. If there's no /settings.php, you will install a mono-site. There won't be a /sites/* folder post installation.

  2. If there's a /settings.php but it doesn't define the $sites variable, you will install a mono-site.

  3. If there's a /settings.php and it defines $sites, you will install a multi-site.

Lastly, as @catch already mentioned, we should definitely make a variable in /settings.php the trigger for enabling multi-site functionality. (as opposed to checking whether a /sites directory exists, since that requires a filestat that won't be cached by the filesystem if the directory doesn't exist.)

I also like the idea of a shared public files directory, but that should definitely be a separate issue. :)

I also think we can address @mikeytown2's issue with the top-level /settings.php; e.g., by allowing it to prime the $conf_path variable, which would then be used as an additional condition (next to $sites) for whether to invoke the site directory discovery process of find_conf_path(). However, I'm not sure yet whether I'll bake that into this patch; could also be a simple follow-up issue.

sun’s picture

Status: Needs work » Needs review
FileSize
14.71 KB
20.66 KB

Went ahead with #31.

Fixed bootstrap of installer attempts to require settings.php before it exists.
Moved default.settings.php into ./core directory, so it is updated with Drupal core.
Added install_conf_path() as an attempt to differentiate between mono-site and multi-site in the installer.
Reverted install_conf_path(); require /settings.php + $sites to exist for multi-site installations.
Fixed Drupal installer writes into the filesystem's root directory.

sun’s picture

Status: Needs work » Needs review
FileSize
2.58 KB
21.49 KB

Allow global /settings.php to set a custom $conf_path. (might also fix that single test failure)

mikeytown2’s picture

I haven't tested #34 but after looking at it I think this addresses part of me and catch's concerns about hitting the filesystem because find_conf_path() can be bypassed which will skip the file_exist call in the for loop.

Issue of interest: #752730: Remove file_exists() during bootstrap

+++ b/core/includes/bootstrap.inc
@@ -679,19 +695,35 @@ function unicode_check() {
+  if (isset($sites) && $conf_path !== '.' && file_exists(DRUPAL_ROOT . '/' . $conf_path . '/settings.php')) {

If $conf_path is already set in the global settings.php, can we skip the file_exists check here?

include_once throws a warning if you try to include a file that doesn't exist, thus the need for file_exists in case anyone is wondering. But if we setup $conf_path in the global settings.php we should assume it's good and throw a warning if not.

sun’s picture

I think the file_exists() in that case is not harmful, because the condition is only triggered when multi-site is enabled, so I think there has to be a site-specific settings.php anyway, and thus, the filestat should be cached by the filesystem.

Frankly, I'm more concerned about the relative '.' $conf_path, which is returned and required for the mono-site case. I've documented the reasons in code.

sun’s picture

+++ b/core/includes/bootstrap.inc
@@ -389,20 +386,45 @@ function timer_stop($name) {
+  // If the multi-site functionality is not enabled, $conf_path is empty, which
+  // means that the root/top-level directory is the site directory.
+  else {
+    // Many callers of this function append a slash to the returned path and do
+    // not prefix it with DRUPAL_ROOT, so Drupal writes new files and
+    // directories to the filesystem's root directory (which is possible on
+    // unsecured local Windows development environments). We therefore need to
+    // return a dot instead of an empty string to ensure that the path is
+    // interpreted as a relative path.
+    $conf_path = '.';
   }

As previously mentioned, this is still the most concerning part for me. I'd love to get rid of the relative '.' path, because it looks really wonky in fully qualified file paths (e.g., /var/www/drupal/./files/foo.png).

However, as the comment clarifies, we can only get rid of it if we had a way to enforce and ensure that all callers of conf_path() are prefixing the returned path with DRUPAL_ROOT. Otherwise, Drupal would potentially write all kinds of stuff into the top-level/root directory of the filesystem. Drupal may not only read and write, it may also delete. On unsecured host platforms (such as my local dev environment :P), this can easily mean that Drupal could delete arbitrary stuff on my disk.

One way to possibly achieve that would be to rename conf_path() into site_path(); i.e., an enforced API break. However, even with that, it wouldn't be guaranteed that all callers will properly prefix file paths with DRUPAL_ROOT.

+++ b/core/includes/bootstrap.inc
@@ -2263,6 +2292,23 @@ function _drupal_bootstrap_configuration() {
+  // Redirect the user to the installation script if Drupal has not been
+  // installed yet (i.e., if no $databases array has been defined in the
+  // settings.php file) and we are not already installing.
+  // @todo This is impossible and incompatible with a required /settings.php.
+  if (empty($GLOBALS['databases']) && !drupal_installation_attempted()) {
+    include_once DRUPAL_ROOT . '/core/includes/install.inc';
+    install_goto('core/install.php');
+  }

I've added a @todo here, since it is impossible to reach that code path, because Drupal bails out on a missing /settings.php earlier already. I'd recommend to drop support for this hand-holding entirely, and require people who want to install Drupal to actually visit /install.php.

moshe weitzman’s picture

This is looking good., Anyone want to push it along further?

sun’s picture

Well, I think the essential change of the patch is ready.

We only need to find answers for the two problems outlined in #37. Those are rather discussion topics than code topics - we need a plan for how to address them, or alternatively, an agreement to punt on both.

The second on install.php hand-holding is negligible IMO. The regression partially exists in HEAD already. However, this patch makes it impossible to fix it; i.e., without a top-level /settings.php, all you can reasonably expect is a fatal error. I personally think we should forget about this hand-holding, and instead, rewrite INSTALL.txt into something that people actually read; i.e., a one-pager. Further hand-holding can happen within the installer, but otherwise, checking and handling this stuff on every single request on regular runtime feels insane.

The first one, on /absolute/path/./core/whatever.inc - it's beyond my knowledge whether such paths impose filesystem lookup and/or performance problems? If not, who cares?

An alternative answer could as well be: ./core/whatever.inc is technically "absolute" already, unless I'm mistaken? If that's the case, we don't necessarily have to prefix file paths with DRUPAL_ROOT?

sun’s picture

Status: Needs work » Needs review
FileSize
21.42 KB

Merged HEAD.

sun’s picture

Status: Needs work » Needs review
FileSize
20.61 KB

Merged HEAD.

sun’s picture

The remaining issue is still that conf_path() should be empty. To clarify:

unlink(conf_path() . '/foo/bar.txt');

HEAD deletes sites/default/foo/bar.txt

This deletes /foo/bar.txt

The current patch does not, because it works around the problem and returns '.' instead of an empty path '':

Patch deletes ./foo/bar.txt

The root of the issue is:

  1. Paths do not contain a trailing slash in PHP. To append a file path, one appends /path. With that, an empty relative path becomes an absolute path.
  2. There are two different consumers of conf_path(): Some callers need the relative filesystem path (e.g., to register extension paths, or to show a path in the UI), some others build an absolute path out of it. Some consumers are doing both.

Potential solutions:

  1. Make conf_path() always return a path that ends in a slash and change all callers, so they do:

    unlink(conf_path() . 'foo/bar.txt');
    
  2. Accept the relative path '.' workaround.

    It appears to be correct after all. However, we'd need to double-check and verify that the ./ part does not appear somewhere in the UI and also not in front-end asset paths; i.e., JS/CSS, which are commonly based on drupal_get_path(), which in turn might return a path prefixed with ./...

  3. Invent some fancy new filesystem path resolver service à la:

    $site = new Drupal\Core\System\Site;
    
    $site->getName();                   # 'default'
                                        # 'example.com'
    
    $site->getPath();                   # .
                                        # sites/example.com
    
    $site->getPath('foo/bar.txt');      # foo/bar.txt [OR]
                                        # sites/example.com/foo/bar.txt
    
    $site->getRealPath();               # /var/www/drupal
                                        # /var/www/drupal/sites/example.com
    
    $site->getRealPath('foo/bar.txt');  # /var/www/drupal/foo/bar.txt
                                        # /var/www/drupal/sites/example.com/foo/bar.txt
    ...
    

    In other words: Introduce a service that specifically accounts for this filesystem path problem space.

    (Note that I could see a couple of other methods and use-cases for such a service.)

sun’s picture

Status: Needs work » Needs review
FileSize
20.83 KB

Merged HEAD.

#45 still remains TBD.

plach’s picture

Yep, I do too: before code freeze, devs working with D8 might need to reinstall Drupal in any moment. This would be a minor adjustment compared to that.

derEremit’s picture

also in favour for adding this to drupal 8. As that would help fixing my symlink setup problem:
#1792310: Wrong DRUPAL_ROOT with non-standard code structure

pwolanin’s picture

seems this is duplicate to something that was committed?

webchick’s picture

I also would still really like for this to happen, as a logical follow-on to /modules and /themes. But it'd need to happen before beta, if it's going to happen in D8.

There's some issue around somewhere about moving sites/default/files to /files and same for that one.

tstoeckler’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
71.99 KB

Here's a straight re-roll.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
72.03 KB

This needed a re-roll but git was smart enough to do that.

sun’s picture

Thanks for reviving this issue!

However, what we really need is a discussion and resolution for #45.

#45 outlines the remaining, major problem space very accurately and provides a few possible resolution paths.

Without a solid and safe solution for #45, this change cannot be employed for D8, as it could destroy random data on unsafe file systems (e.g., Windows) and mark a major/critical API change to conf_path() and all dependent functions.

Like @webchick et al, I also believe that we should definitely perform this change for D8; for consistency and simplicity. The question is how we can do it, and whether any of the provided options are feasible and acceptable.

tstoeckler’s picture

So the reason for the test tails is the problem already mentioned in #45: PHP das not play nicely with paths that contain a dot in the middle, e.g. 'this/path/./exists'. With the patch this can happen if you concatenate a string with conf_path() which can return '.' with the patch.

So we need to implement a simple class with methods for pre- and appending strings to the conf path so that people do not need to perform any concatenation anymore.

tstoeckler’s picture

Well, #63 was only partly correct. We don't actually have strings with dots in the middle, but from the review logs it seems the test bot has problems with the strings anyway. E.g.:

'Failed to write configuration file: ./files/simpletest/913101/config_active/system.action.node_save_action.yml'

At first I thought it was because the testbot installs sites in a subdir but I couldn't reproduce those failures even with a site installed in a subdir.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
46.84 KB
110.77 KB

This patch implements such a service. It offloads the actual suffix handling into a \Drupal\Component since that is unrelated to the actual site stuff. I chose \Drupal\Core\Utiltity\Site as the namespace and name, but not 100% satisfied with that, so feel free to suggest something else. #45 suggested \Drupal\Core\System but I don't think that namespace is very meaningful.

Let's see how far we are with this. There's a lot left to clean up but I'd like to see if we can get this to green before further refactoring.

sun’s picture

FileSize
21.61 KB

Very interesting! — I was literally working on almost the exact same thing, just without classes. :)

Instead of a new Site class, I went with a new site_path() function. ;)

For everyone else following at home, to perhaps first clarify why that approach seems most sensible is that there are only 25 calls to conf_path() in D8.

Now, even down to implementation details, we both came up with the exact same helper function:

- conf_path() . '/relative/path'
+ Site::getPath('relative/path')site_path('relative/path')

…which piggy-backs on conf_path() to return the proper + safe path relative to the site path.

I do like the Site class a lot more, but there is one major issue with that:

conf_path() is called and initialized as one of the very first things in Drupal's bootstrap — before the class loader is available. In fact, the class loader implementation is swappable via settings.php, which is discovered + loaded via conf_path(), which presents a race-condition.

The way we're overcoming that situation for Settings is via this hard-coded line in drupal_settings_initialize():

  require_once __DIR__ . '/../lib/Drupal/Component/Utility/Settings.php';

Since the class loader is not available yet, the Settings class must be fully self-contained; i.e., classes in other namespaces cannot be used, because they cannot be located/imported/loaded, unless they would be loaded in a hard-coded way as well. We should hard-code as little as possible in core.

So again, I really like the Site class — but to make that work, we need to keep it self-contained. Additionally:

  1. I'd prefer to move the code of conf_path() + find_conf_path() almost literally for now. Your patch appears to introduce many additional helper methods, but we can re-evaluate and improve the Site class methods later on.
  2. Without having studied all existing uses and details throughout core, the fundamental concept of conf_path() and thus the new Site class is a singleton → the site/conf path can only be instantiated once per lifetime; it is not valid operation to change the site/conf path mid-request. Tests are the only valid exception, and that exception should be explicitly hard-coded and handled; see the approach taken in #1881582: Change configuration overrides to use $config instead of $conf.
  3. Likewise, I'm surprised to see a reset() method on the Site class — I don't think that should be possible, since the site/conf path cannot be changed at (regular) runtime.
  4. The separate Path class appears to be unnecessary.
  5. Let's also remove the usage and coupling of Settings — the only relationship between the two is that the $settings array happens to be contained in a settings.php that happens to be discovered by the Site class, but the Site class should be able to operate independently.
  6. Let's remove the simpletest override stuff from there, too.

Attached (incomplete) interdiff (1) proves that I was literally working in the same thing ;) and (2) also contains some fixes for reroll/merge/rebase conflicts and missing updates to affected files in HEAD, based on #60. We need to incorporate the latter changes into future patches.

sun’s picture

Status: Needs work » Needs review
FileSize
57.95 KB
29.85 KB
  1. Re-incorporated updates and fixes from interdiff.
  2. Changed Site class into a secure singleton.
  3. Removed conf_path(), restored lost docs.

Tested the installer + multi-site discovery + $conf_path override in /settings.php + regular runtime environment.

When testing, don't forget that sites/default is a thing of the past. ;) To test on an existing D8 site, create a /settings.php containing this:

$conf_path = 'sites/default';
sun’s picture

FileSize
55.76 KB
19.08 KB
  1. Removed all remnants of Utility\Path.
  2. Reverted/replaced $require_settings + reset() hackery with a dedicated initInstaller() method.
tstoeckler’s picture

Wow, quite some changes. Here's a consecutive review of your comments:

#68:

In fact, the class loader implementation is swappable via settings.php, which is discovered + loaded via conf_path(), which presents a race-condition.

As far as I'm aware, that's no longer accurate. We now have /core/vendor/autoload.php which we require scripts (like index.php) to include and thus we can rely on the autoloader for /core/vendor and /core/lib. I think the Settings hack is an artifact of when that was not the case. Not 100% certain, though.

Likewise, I'm surprised to see a reset() method on the Site class — I don't think that should be possible, since the site/conf path cannot be changed at (regular) runtime.

I'm fine with removing this if we can, but it's the same for setPath(). If you do that in the middle of a request everything will explode as well. So it's actually much worse than reset() IMO.

The separate Path class appears to be unnecessary.

I'm fine with removing it (although I actually see no reason to since I already wrote and tested it). I initially wanted to use it in drupal_rewrite_settings() because using Site there is not always correct, but then realized that the strings can never be empty, so I used simply concatenation there, or in other words retained that.

Let's also remove the usage and coupling of Settings — the only relationship between the two is that the $settings array happens to be contained in a settings.php that happens to be discovered by the Site class, but the Site class should be able to operate independently.
Let's remove the simpletest override stuff from there, too.

I don't really know what you mean by that since I just copied the existing code over, but you seem to have done the same, so ??!?

#69:

When testing, don't forget that sites/default is a thing of the past. ;) To test on an existing D8 site, create a /settings.php containing this:

$conf_path = 'sites/default';

Good idea! I always just copied the existing settings.php, but that's way cooler.

  1. +++ b/core/default.settings.php
    @@ -83,16 +81,21 @@
    - * # URL: http://dev.drupal.org
    + * URL: http://dev.drupal.org
    ...
      * @code
    

    This is incorrect. Within a @code block "URL:" is not valid.
    (Also elsewhere)

  2. +++ b/core/includes/bootstrap.inc
    @@ -361,8 +333,6 @@ function drupal_override_server_variables($variables = array()) {
    -  // @todo remove once \Drupal\Core\Utility\Site::getMultisitePath() no longer
    -  //   uses $_SERVER.
    

    I still think it makes sense to keep this todo, IMO.

  3. +++ b/core/includes/bootstrap.inc
    @@ -451,27 +421,26 @@ function drupal_settings_initialize($require_settings = TRUE) {
    -  require_once __DIR__ . '../../lib/Drupal/Component/Utility/Settings.php';
    ...
    +  require_once __DIR__ . '/../lib/Drupal/Component/Utility/Settings.php';
    

    Wow, this was massively broken before, or am I missing something?? That seems quite strange.

  4. +++ b/core/includes/bootstrap.inc
    @@ -1867,11 +1836,7 @@ function _drupal_bootstrap_configuration() {
    +  // Set the Drupal custom error handler. (requires \Drupal::config())
    ...
    -  // bootstrap and settings.php cannot be handled by Drupal.
    ...
    -  // requires $conf/settings.php (to be swappable). Therefore, errors in early
    ...
    -  // requires the service container, which requires the class loader, which
    ...
    -  // Circular dependency: The error/exception handler requires config, which
    ...
    -  // Set the Drupal custom error handler.
    

    Why is that no longer true?

  5. +++ b/core/includes/install.core.inc
    @@ -1195,7 +1189,7 @@ function install_settings_form_validate($form, &$form_state) {
    -function install_database_errors($database, $settings_file) {
    +function install_database_errors($database) {
    

    Awesome. ++

  6. +++ b/core/lib/Drupal/Core/Utility/Site.php
    @@ -20,118 +19,141 @@ class Site {
    +  private $root;
    ...
    -  protected static $root;
    

    I see no point in making this private TBH. This class is not swappable anyway, so people cannot replace this with a sub-class. If they do want to subclass this for whatever reason, let them do whatever they want, right?
    (Also elsewhere)

  7. +++ b/core/lib/Drupal/Core/Utility/Site.php
    @@ -20,118 +19,141 @@ class Site {
    +  private static $instance;
    

    Needs docs.

  8. +++ b/core/lib/Drupal/Core/Utility/Site.php
    @@ -20,118 +19,141 @@ class Site {
    +   *   initInstaller() method.
    ...
    +   * @todo Consider to move the $require_settings=FALSE case into a dedicated
    

    Yes, please!!!

  9. +++ b/core/lib/Drupal/Core/Utility/Site.php
    @@ -20,118 +19,141 @@ class Site {
    +  private function __construct($root_directory, array $sites = NULL, $custom_path = NULL, $require_settings = TRUE) {
    

    Awesome, I didn't know you could do private __construct()s. Cool!
    (For private vs. protected see above)

  10. +++ b/core/lib/Drupal/Core/Utility/Site.php
    @@ -154,16 +176,21 @@ protected static function determinePath() {
    +    //   in time.
    ...
    +    //   The Settings singleton is not and cannot be instantiated at this point
    ...
    +    //   settings.php declares the $settings for instantiating Settings.
    ...
    +    // @todo Critical race condition: This class discovers settings.php, and
    

    Actually that's not true, as far as I'm aware. Simpletest initiates the singleton manually earlier in the request.

  11. +++ b/core/lib/Drupal/Core/Utility/Site.php
    @@ -179,65 +206,51 @@ protected static function getSimpletestPath() {
    +    return self::$instance->root . '/' . self::$instance->resolvePath($filepath);
    

    This is incorrect as self::$instance->resolvePath($filepath) may return an empty string.

#71:

  1. +++ b/core/includes/bootstrap.inc
    @@ -333,6 +332,10 @@ function drupal_override_server_variables($variables = array()) {
    +  // @todo Inject a global Request object for all CLI executions + use the
    +  //   manipulated $request->server as basis for any subsequently instantiated
    +  //   Request objects. Alternatively, sub-class Request or fix it upstream to
    +  //   account for CLI environments.
    

    Ahh, so the @todo's back. :-)
    What do you mean by "subsequently instantiated Request objects"? Subrequests?

    Also, there are a bunch of reasons why I think subclassing Request would make sense but apparently the WSCCI folks consider that a no-go. I don't know why that is.

  2. +++ b/core/includes/bootstrap.inc
    @@ -409,38 +412,27 @@ function drupal_valid_http_host($host) {
    -function drupal_settings_initialize($require_settings = TRUE) {
    +function drupal_settings_initialize() {
    

    Yay!

  3. +++ b/core/includes/bootstrap.inc
    @@ -409,38 +412,27 @@ function drupal_valid_http_host($host) {
    -  global $sites, $databases, $cookie_domain, $conf, $db_prefix, $drupal_hash_salt, $base_secure_url, $base_insecure_url, $config_directories;
    +  global $databases, $cookie_domain, $conf, $db_prefix, $drupal_hash_salt, $base_secure_url, $base_insecure_url, $config_directories;
    

    Even more yay!

  4. +++ b/core/includes/bootstrap.inc
    @@ -409,38 +412,27 @@ function drupal_valid_http_host($host) {
    +  if ($conf_path !== '' && is_readable(DRUPAL_ROOT . '/' . $conf_path . '/settings.php')) {
    ...
    +  $conf_path = Site::getSitePath();
    ...
    +    include_once DRUPAL_ROOT . '/' . $conf_path . '/settings.php';
    

    This is exactly the sort of logic we're trying to encapsulate in Site, to prevent people from forgetting that $conf_path may be ''. Let's add a helper to Site that include_once's the settings.php

  5. +++ b/core/lib/Drupal/Core/Utility/Site.php
    @@ -37,29 +30,90 @@ class Site {
    +      if (!self::$instance->isInstaller) {
    ...
    +    if (isset(self::$instance)) {
    

    That can be collapsed into one condition.

  6. +++ b/core/lib/Drupal/Core/Utility/Site.php
    @@ -253,22 +321,16 @@ public static function getAbsolutePath($filepath = '') {
    +   *   remove this.
    ...
    +   * @todo Add special re-initialization method for WebTestBase::setUp() and
    

    Yes, that's what I mentioned above :-)
    Glad we're on the same page.

  7. +++ b/core/lib/Drupal/Core/Utility/Site.php
    @@ -206,6 +256,24 @@ private function getSimpletestPath() {
    +  public static function getSitePath() {
    +    return self::$instance->path;
    +  }
    

    Maybe I'm misunderstanding this method, but If we have this $filepath in getPath() should be required, no?

All in all, looks great. Thanks for all the cleanup.

sun’s picture

FileSize
55.86 KB
2.85 KB

Thanks for the in-depth review, @tstoeckler! And sorry for some of the confusion from my side in my first comments. For clarity, let me address all points from your comment:

re: re: #68:

  1. Hard-coded class file loading: You're right, I wasn't up to speed. All front controllers are including /core/vendor/autoload.php now.
  2. Removal of reset() + setPath(): Yes, with more recent additions to Simpletest to enable testing of $settings overrides as well as the interactive installer, that will be non-trivial to resolve. Removing reset() was simple, but setPath(), simpletest_conf_path & Co are more hairy to deal with.
  3. Removal of Path class: There are two reasons for why I don't think we should introduce that here: (1) To keep this issue/patch focused on its actual goal, a top-level /settings.php and adjustment of conf_path(), and (2) The proposed concept of the Path class was essentially a stream wrapper implementation. I'm not saying that we cannot or should not investigate that idea further, but it would be great to defer that to a separate follow-up issue. Let's get the primary objective done first :)
  4. Removing usage and coupling with Settings: I originally misinterpreted the code in your patch and didn't notice that it was copied from HEAD. As mentioned above, making the singleton work with those existing WebTestBase overrides will be tough.

re: re: #69:

  1. phpDoc @code formatting for $sites in settings.php: The phpDoc changes are merely taken over from the latest code of sites.php in HEAD. I assume someone adjusted that for readability in the past year. Let's simply keep the docs as they are in HEAD and/or discuss them in a separate issue, if necessary.
  2. On drupal_override_server_variables(): I didn't test yet, but I don't think the current function works for its primary purpose right now (adjusting global state $_SERVER variables for CLI scripts/environments). It is supposed to be called very early by CLI scripts, before bootstrapping Drupal, so as to allow CLI scripts to operate with a desired site directory instead of the default site (→ lacking a server/request environment, Drupal would always use the default site). Consequently, none of the things and services that it tries to access should exist yet. I've the impression that it only works currently, because all invocations in core are originating from tests; i.e., Drupal happened to be bootstrapped already. For now, I'd prefer to only adjust the comments, and re-evaluate the situation with drupal_override_server_variables() in a separate issue.
  3. Yes, the require_once path for the Settings class is weirdly broken and I don't know how that can even work in HEAD right now. Anyway, it's no longer needed, hence removed.
  4. Revert of error handler comment in _drupal_bootstrap_configuration(): The error handler registration was adjusted in another issue already; adding a comment to that in this patch here simply felt out of scope.
  5. :)
  6. Private Site class instance properties/methods: They are definitely not public, and there is no way to extend or override the Site class, so the internal methods and properties are in fact private in practice. Exposing them as protected would gain nothing and would be misleading at best.
  7. Docs are resolved in #71.
  8. Docs are resolved in #71.
  9. :) Yup, that's how the singleton pattern works.
  10. Simpletest Settings override: Yes, that needs more investigation.
  11. getAbsolutePath() for empty $filepath: Indeed, fixed.

re: re: #71:

  1. drupal_override_server_variables(): See above; the primary purpose is to allow CLI scripts to operate in a certain site directory as opposed to the default. For that to work with sub-requests & Co, the server parameter adjustments need to stick. But again, I think it would be best to defer that discussion to a separate issue.
  2. :)
  3. :)
  4. The concatenation of $conf_path in drupal_settings_initialize() is safe, because the code already knows that $conf_path is not empty. The only reason for adding getSitePath() was to avoid calling resolvePath(), but that was a silly idea and unnecessary optimization.
  5. init() singleton re-instantiation check: No, combining the conditions would allow to enter the else (__construct()) code path under unwanted circumstances. However, I've changed the control structure to clarify the flow.
  6. :)
  7. Removed.

  1. Fixed getAbsolutePath() for empty filepath.
  2. Removed getSitePath().
  3. Simplify and clarify initialization in init().

Still don't know why testbot isn't able to install — works great for me locally (even when installing in a subdirectory). → I assume it is not able to create/write the /settings.php file in the root directory due to file/folder permissions?

tstoeckler’s picture

Coolio. Only replying to the things I disagree with to keep this short.

there is no way to extend or override the Site class, so the internal methods and properties are in fact private in practice. Exposing them as protected would gain nothing and would be misleading at best.

Exposing them as protected would allow people to subclass them properly when using them for other projects. In general, using private over protected needs justification not the other way around. Since - as you say - there will be no subclassing inside of Drupal anyway, private vs. protected makes no difference. So let's stick with protected.

+++ b/core/includes/bootstrap.inc
@@ -428,7 +428,8 @@ function drupal_settings_initialize() {
+  // Concatenation is safe here, since $conf_path is known to be not empty.
+  $conf_path = Site::getPath();
   if ($conf_path !== '' && is_readable(DRUPAL_ROOT . '/' . $conf_path . '/settings.php')) {
     include_once DRUPAL_ROOT . '/' . $conf_path . '/settings.php';
   }

You're right that it's safe, but I still think we could be nice to people and have a helper method that does that for them.

sun’s picture

sun’s picture

Status: Needs work » Needs review
FileSize
56.13 KB
4.15 KB
  1. Fixed bogus interpretation of isInstaller() and moved the flag into constructor.
  2. Removed bogus _once from settings.php file inclusions.
sun’s picture

FileSize
56.57 KB
1.76 KB

Oh my, finally figured it out:

  1. #2167507: Fix rowCount query usage in pgsql and sqlite drivers fixes the currently existing bug that Drupal doesn't install with SQLite, throwing a Database\RowCountException without exception message, whereas the installer catches the exception and just outputs the exception message (→ empty string). Took me multiple hours to debug that. :(
  2. The testbot fails in PIFR's initial installation of Drupal; i.e., it fails to install in a single-site environment as opposed to a multi-site environment. I was constantly testing the latter, not noticing that the installer fails to create/copy the root /settings.php file. Attached patch fixes that, by adjusting install_check_requirements() to pass absolute filepaths to drupal_verify_install_file(), since the site directory filesystem checks previously resulted in file_exists('') in there. ;)
  3. This adjustment slightly clashes with #1884854: install.core.inc uses the conf dir as $file instead of the $settings_file it should, but it would still be helpful to get that in first.

With that, testbot should finally start to run again. But most likely, all web tests will blow up in mighty ways. ;) Let's see. :)

sun’s picture

FileSize
73.7 KB
32.46 KB

It took me two full days, but I successfully resolved the test environment challenge.

I'm eliminating the entirety of Simpletest hacks and overrides. They are replaced with native multi-site functionality. Each web test gets an actual site in /sites/simpletest/$prefix. For WebTests, we simply let the installer do its job as usual. The test environment is negotiated only once by the Site singleton.

It might be possible to split these changes into a separate issue, but I first want to see how far we get with this solution here.

sun’s picture

FileSize
73.78 KB
763 bytes

Fixed drupal_valid_test_ua() check for interactive installer in install_begin_request().

sun’s picture

I managed to cleanly extract the Simpletest changes — since they are also needed for #1881582: Change configuration overrides to use $config instead of $conf, it made even more sense to separate them into a dedicated issue:

#2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead

pwolanin’s picture

I think the summary needs updating - in fact Drupal 8 already has multisite disabled if you do not create the sites/sites.php file

https://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functi...

tstoeckler’s picture

Issue summary: View changes

Had a stab at updating the issue summary. Please revise if necessary.

I also had a look at the WebTestBase problem. My problem locally was - and I assume the testbots have the same problem - that the webserver user did not have write permissions on the 'sites' directory. Which generally is a sensible thing. I think we should require users to create the sites/simpletest directory manually upon installing Simpletest module and make that writable by the webserver user.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
3.75 KB

Maybe something like this?

tstoeckler’s picture

Also: I wrote #2170023: Use exceptions when something goes wrong in test setup in order to make any sense of the Simpletest output in case of failure during setUp(). That is now green, so I would love reviews over there :-)

sun’s picture

Status: Needs review » Postponed
jibran’s picture

Status: Postponed » Needs review

Back to NR.

sun’s picture

FileSize
52.97 KB
18.31 KB
  1. Merged 8.x.
  2. Fixed merge conflicts + removed unnecessary changes.
  3. Vastly simplified + hardened Site singleton.

Awesome. :)

sun’s picture

Status: Needs work » Needs review
FileSize
53.73 KB
13.34 KB
  1. Only allow Site::tearDownTest() after TestBase::restoreEnvironment().
  2. Fixed update.manager.inc has to use Site::getAbsolutePath() to check fileowner of site directory.
  3. Moved Site singleton into new Site component.
  4. Fixed UnitTestBase (and DrupalUnitTestBase) tests.

This patch could/should come back green. :-)

sun’s picture

Status: Needs work » Needs review
FileSize
56.82 KB
3.95 KB
  1. Fixed DrupalKernelTest.
  2. Converted new instances of conf_path() introduced by #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead
  3. Added special Site::tearDownTest() condition for BrokenSetUpTest.
longwave’s picture

I think #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead broke contrib web tests: #2194271: D8: Make testbot check out projects into top-level /modules directory - is there anything special we have to consider here to ensure contrib tests don't get broken again?

jibran’s picture

  1. +++ b/core/includes/bootstrap.inc
    @@ -495,9 +386,20 @@ function drupal_settings_initialize() {
    +  Site::init(DRUPAL_ROOT, isset($sites) ? $sites : NULL, isset($conf_path) ? $conf_path : NULL);
    

    please move isset checks out of function call.

  2. +++ b/core/lib/Drupal/Core/Site/Site.php
    @@ -0,0 +1,341 @@
    +  private function initializePath(array $sites = NULL, $custom_path = NULL) {
    ...
    +  private function determinePath(array $sites, $require_settings) {
    

    array :S

jibran’s picture

I asked this question in IRC

can someone explain me why we are doing this self::$instance = $this;

and here is the answer by @sun

The Site singleton I'm introducing there is a proper singleton; i.e., it can only be instantiated once and only once. The only exception to that are tests, but even re-instantiation/restore requests for the singleton from there are handled in a 100% controlled fashion; i.e., even a test is only able to instantiate the singleton once.

http://en.wikipedia.org/wiki/Singleton_pattern

Thanks @sun for explaining this, now the patch is making a lot of sense.

donquixote’s picture

The Site singleton I'm introducing there is a proper singleton; i.e., it can only be instantiated once and only once.

I would say this is one of the rare cases where a singleton is ok. The choice of site directory will change static context: E.g. it could cause one or another version of a class to be defined, which inevitably has a global effect. Similar to class loader registration.

(A singleton is ok in cases where we have already given up on avoiding global state. Either because we are already surrounded by global state (Drupal 7), or because what we are doing will have static global effects.)

This being said, I disagree with the idea that any class should be instantiated only once.
The "singleton" idea should not be seen as a constraint on a class, but as a constraint on a variable.
We should not care how many instances of that class are floating around. The thing that matters is that we have a variable is globally available, that is lazy-initialized with an object, and this object won't be replaced after initialization.

Therefore:

  • The constructor should not have to deal with self::$instance. Instead, this should be done e.g. in a static method initInstance().
  • The $instance variable should live somewhere outside of the class that is being instantiated.
  • The result would be two classes: One with the static methods, and one that is going to be instantiated, and can be instaniated as often as anyone likes.

https://github.com/donquixote/drupal/commit/f062d1e7ce68f1712adb72353a14...
This is only a quick attempt. But it shows that the instance management and the actual business are really two separate concerns, that can easily live apart from each other.

I am by far not satisfied with this exact separation. But at least this split makes the separation visible, and inspires to improve it.
E.g. one idea could be to have separate SiteInstance classes: One for single-site and one for multisite.

donquixote’s picture

+
+  // Discover the site directory.
+  Site::init(DRUPAL_ROOT, isset($sites) ? $sites : NULL, isset($conf_path) ? $conf_path : NULL);
+

This won't work :)
The $conf_path is not defined at this point.

sun’s picture

@donquixote: Thanks for your feedback. I strongly disagree with that proposed split, because (1) it only adds complexity but no architectural benefit at all, (2) it makes DX fuzzy, because there is suddenly a class that can be instantiated more than once, whereas that is not possible under any circumstances (except for the controlled edge-cases that are explicitly being accounted for), and (3) it only opens the door for repeating the insanity of the current Settings fake-singleton class.

No, I want (1) simplicity, (2) crystal clear DX and 100% controlled code flows, and (3) no mistakes.

conf_path() (or this Site singleton) sits below and under the hood of the entire rest of the system. You are not able to touch this after it has been instantiated. Introducing a separate "instance" makes little sense, because there can only be one instance. The single instance is accessed from various other code locations. Since there can only be one, there's no point of dependency-injecting it. That's the whole point of the Singleton pattern.

As mentioned in #111, the implementation is very carefully crafted to specifically support exactly two edge-cases we have in core: The installer and tests. And even the possible call chains and logic for those is explicitly force-constrained to a very stringent expected invocation order. Any attempt to futz with the Site singleton beyond that will and is expected to blow up with an exception.

The technical implementation here follows the common singleton pattern in PHP:

http://sourcemaking.com/design_patterns/singleton
http://7php.com/how-to-code-a-singleton-design-pattern-in-php-5/
http://www.phpdesignpatterns.com/design-patterns/pattern-singleton/
http://www.php.net/manual/de/language.oop5.patterns.php#language.oop5.pa... (German only)

In case all of this is not sufficient to convince you, then ping me on IRC, please.


re: #114: $conf_path is expected to be not defined, unless explicitly defined in the root /settings.php file. This works, and is proven by tests.

donquixote’s picture

@sun: I would love to discuss this on irc, but i am out of town over the weekend and only occasionally online, so it won't happen probably.

Singleton is referred to as an anti-pattern more often than otherwise, and it is really hard to find any valid and undisputed use case for "this class must be instantiated only once". In most cases what we actually want is a "statically cached and lazy-instantiated object", and even that is only if we are willing to accept global state (which in this case would be a "yes").

If we are actually going to split this up, I am sure there are better ways than this quickly made-up 1:1 split. E.g.

$site_picker = new SitePicker($sites);
$site_path = $site_picker->determineSitePath($_SERVER);
$site = $site_path ? new MultisiteSite($root, $site_path) : new MainSite($root);
$resolved_path = $site->resolvePath($path);

This would allow big parts of the logic to be nicely unit-tested, without touching any static variables anywhere.
But for the actual usage this can still be wrapped in a static facade thingie (no idea if that's a valid term).

Site::init($root, $sites);
$resolved_path = Site::resolvePath($path);

The static instance within class Site will then really be instantiated only once and won't change later in the request - except for testing.

sun’s picture

Status: Needs work » Needs review
FileSize
57.79 KB
3 KB

@donquixote: I'm not sure what has caused the disconnect for you, but you're definitely trying to solve problems that we don't have. ;) Perhaps the docs are not clear enough yet? To clarify:

  1. Lazy-instantiation is irrelevant. The site directory is negotiated as the 3rd function call in every bootstrap. This is required, because the Site directory presents a dependency for the entire rest of the system.
  2. There is only one Site (directory) and it is irrelevant whether that is a "multi-site" directory or not. Code must not rely on that aspect of the site directory discovery process.
  3. The Site environment info is global to the entire application. Without the Site, you are not able to execute Drupal, because the Site controls from where settings.php is read. settings.php drives configuration directories, databases, the installation profile, hash salt, etc.pp. Like the Site, Settings is read-only. You are not able to change Settings at runtime.

If these clarifications still don't help to clarify the architectural situation, then let's move this discussion into a separate follow-up issue, please. :-)


  1. Fixed conflicts after merging 8.x.
  2. Resolved last remaining @todo in system_requirements().
sun’s picture

Status: Needs work » Needs review
FileSize
57.7 KB
914 bytes

Fixed undefined variable in system_requirements().

sun’s picture

Status: Needs work » Needs review
FileSize
58.46 KB
8.39 KB
  1. Simplified $sites and $conf_path variables in drupal_settings_initialize().
  2. Simplified various code logic aspects of Site singleton + improved docs.
  3. Cleaned up WebTestBase. (to hopefully fix random test failures)
  4. Fixed Site::$isInstaller flag does not default to FALSE.
sun’s picture

This issue is now officially blocked on Drush usage on testbots:

drush si -y --db-url=mysql://my:pass@localhost/drupaltestbotmysql --clean-url=0 --account-name=admin --account-pass=drupal --account-mail=admin@example.com
> You are about to create a sites/default/settings.php file and empty any Config
> directories and DROP all tables in your \'drupaltestbotmysql\' database.
> Do you want to continue? (y/n): y
---
copy(sites/default/default.settings.php): failed to open stream: No such file or directory

:-( Why are we using Drush on the testbots? It only appears to be used for that single site-install command...?

donquixote’s picture

One thing I don't understand:
(independent of the singleton controversy in #113ff)

When Site::initializePath() is called from initInstaller(), it never has a $sites array.
Therefore, the Site::determinePath() will always return '' during installation.
Is this intended?

sun’s picture

Status: Needs work » Needs review
FileSize
83.55 KB
30.55 KB
  1. Restored sites/default/default.settings.php to resolve dependency on Drush on testbots.
  2. Fixed Site::$isInstaller handling, removed unnecessary code and @todos.
  3. Fixed Site::init() is not actually invoked in non-interactive installer in tests.
  4. Ensure Site::init() is called + prevent this error from error happening ever again.

Note: 30KB increase in patch size is caused by copy of default.settings.php.

sun’s picture

Status: Needs work » Needs review
FileSize
83.34 KB
940 bytes

Fixed merge conflicts.

sun’s picture

GREEEEEN! Finally! :-)

I'll try to update the issue summary as soon as possible. But technical code reviews are welcome already.

jibran’s picture

+++ b/core/lib/Drupal/Core/Site/Site.php
@@ -0,0 +1,341 @@
+ * A utility class for easy access to the site path.

An :)

donquixote’s picture

class Site needs some @throws tags.

tstoeckler’s picture

+++ b/core/includes/bootstrap.inc
@@ -489,9 +380,23 @@ function drupal_settings_initialize() {
+  // Read settings.php of the actual site, unless it is the root/default site.
+  // Concatenation is safe here, since $conf_path is known to be not empty.
+  $conf_path = Site::getPath();
+  if ($conf_path !== '' && is_readable(DRUPAL_ROOT . '/' . $conf_path . '/settings.php')) {
     require DRUPAL_ROOT . '/' . $conf_path . '/settings.php';
   }

Should/Could be a follow-up but I just thought we could make this just a bit cooler:

if (Site::isMultisite() && is_readable(Site::getAbsolutePath()) {
 ...
}
tstoeckler’s picture

Wrote a draft change notice. Sadly, I can't RTBC this, though.

donquixote’s picture

I still don't agree with the singleton approach.
It introduces unnecessary obstacles on unit-testing the internal logic.

The entire thing can be split up into unit-testable components.
(shame on me for not actually writing the unit tests yet)
https://github.com/donquixote/drupal/compare/97d689a...D8-sites-1757536-...

EDIT: Bug fixed in
https://github.com/donquixote/drupal/compare/97d689a...D8-sites-1757536-...

tstoeckler’s picture

I'm not a native speaker but I think #132 is incorrect and the current code is correct. I.e. I think "a utility" is correct and "an utility" would be incorrect.

larowlan’s picture

yeah 'a utility' is correct.
English--

sun’s picture

  1. Cleaned up phpDoc in Site class.
  2. Use proper subpathname terminology instead of $filepath to be consistent with PHP's SplFileInfo/RecursiveDirectoryIterator.

@donquixote: I'm not opposed to potential improvements of the implementation, but I'd really prefer to discuss and do that in a follow-up issue, since the changes you're proposing are all-internal to the code and will not affect the public API. As soon as the bootstrap uses a proper kernel, we're going to change this code anyway, in order to inject a proper Request into Site::init() - which isn't possible right now, because drupal_settings_initialize() does not have access to a Request in HEAD. That will be cleaned up in aforementioned issue, or a follow-up issue to it.

The one and only reason for introducing the Site class here is because we have to prevent faulty string concatenations that would result in absolute filesystem paths in case the root directory is the site directory. As long as the public API of the Site class is retained, we can improve the internal implementation at any time, because initialization of the Site singleton is the responsibility of the application framework; user-space/integration code has no business to futz with it after initialization — that is why all of the Site initialization methods ensure that no code is able to re-initialize the site directory, unless it is one of the known edge-cases.

The one and only goal of this issue is to enable a root /settings.php, so as to be consistent with the new top-level extension directories and no longer throw the advanced multi-site concept in everyone's face.

Updated issue summary.

sun’s picture

FileSize
91.18 KB
9.38 KB

Updated core documentation files.

sun’s picture

FileSize
87.42 KB

Merged 8.x. A few diff hunks that touched legacy/deprecated code are gone from core (especially the upgrade docs), so this patch is slightly smaller. :)

Would be great to move forward here.

sun’s picture

#145 still applies cleanly.

sun’s picture

FileSize
87.48 KB

Merged 8.x.

Status: Needs review » Needs work

The last submitted patch, 147: site.147.patch, failed testing.

moshe weitzman’s picture

FYI, Drush has been patched such that it uses the DB API to get the creds instead of a global. So Drush will work before and after this patch lands (without workarounds).

If it greases the wheels, the testbot maintainers are welcome to start running a new SHA1 on Drush's master branch in order to get this patch to green.

sun’s picture

Status: Needs work » Needs review
FileSize
87.88 KB

Merged 8.x. — Samples of the failing tests are passing for me locally, so trying once more against testbot.

Status: Needs review » Needs work

The last submitted patch, 150: site.150.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
Issue tags: +Sunrise Sanity Cruise
FileSize
88.43 KB
1.89 KB
  1. Retain conf_path() for Drush.
  2. Explicitly limit conf_path() to Drush only + assume testbot parent site installation.
  3. Fixed insufficient condition; run-tests.sh bootstraps *exactly* to DRUPAL_BOOTSTRAP_CONFIGURATION now.
sun’s picture

FileSize
88.44 KB

Merged 8.x.

pwolanin’s picture

I'm not sure the issue summary is up to date. At the least you have to create sites.php now for Drupal 8 to turn on multi-site functionality so it's not clear what file checks are you referring to. Or you mean the check for sites.php itself?

From the patch it also looks like you are using the root files/ directory by default for files? I don't see that in the summary.

Status: Needs review » Needs work

The last submitted patch, 153: site.153.patch, failed testing.

sun’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update

Clarified summary on the file_exists()/sites.php aspect (a pet peeve of @catch ;)) + the implicit change to /files becoming the new public files directory (by default; on single-site envs).


Random test failure → #2233929: drupal_set_time_limit should not be able to change the time limit if it's already unlimited

tstoeckler’s picture

153: site.153.patch queued for re-testing.

dawehner’s picture

I really like this patch!

  1. +++ b/core/includes/install.core.inc
    @@ -1937,8 +1945,8 @@ function install_configure_form($form, &$form_state, &$install_state) {
    +  $settings_dir = Site::getPath();
    +  $settings_file = Site::getPath('settings.php');
    

    I wonder whether we should split up the functionality of file vs. directory into two methods.

  2. +++ b/core/includes/install.core.inc
    @@ -2263,20 +2269,11 @@ function install_check_requirements($install_state) {
     
    -    // If default.settings.php does not exist, or is not readable, throw an
    -    // error.
    -    if (!drupal_verify_install_file($default_settings_file, FILE_EXIST|FILE_READABLE)) {
    -      $requirements['default settings file exists'] = array(
    

    Just to understand it: given that DRUPAL_ROOT/default.settings.php never needs to be copied this exception is not helpful anymore.

  3. +++ b/core/lib/Drupal/Core/Site/Site.php
    @@ -0,0 +1,358 @@
    +  private $root;
    ...
    +   */
    +  private $path;
    ...
    +  private $isInstaller = FALSE;
    ...
    +  private static $original;
    ...
    +  private static $instance;
    

    You will never know whether you might want to subclass it.

  4. +++ b/core/lib/Drupal/Core/Site/Site.php
    @@ -0,0 +1,358 @@
    +   * @throws \BadMethodCallException
    ...
    +   * @throws \BadMethodCallException
    ...
    +   * @throws \BadMethodCallException
    +   * @throws \RuntimeException
    

    It would be great to explain under which circumstances we throw these exceptions.

sun’s picture

FileSize
89.05 KB
3.2 KB

Thanks!

  1. Splitting up Site::getPath() for files vs. directories is an interesting thought. — Need to think about that some more. Could also possibly be a follow-up?
  2. Correct. :)
  3. If there will be a use-case in the future, we can change the visibility. For now, the idea is to intentionally keep the class "locked-down".
  4. Added @throws phpDoc descriptions.
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Splitting up Site::getPath() for files vs. directories is an interesting thought. — Need to think about that some more. Could also possibly be a follow-up?

Sure! I don't think that there is a win in getting crazy about API changes, especially at very special APIs.

Correct. :)

If there will be a use-case in the future, we can change the visibility. For now, the idea is to intentionally keep the class "locked-down".

This statement seems to be fine under the assumption that we control all the code, which is potentially not true.

pwolanin’s picture

Status: Reviewed & tested by the community » Needs review

I'm a little concerned about the installer and sites directory permissions checks and changes that we have in core.

With this patch, you'll have Drupal trying to chmod the root install directory?

sun’s picture

FileSize
92.62 KB
34.29 KB
  1. Changed installer + system_requirements() to not change permissions of root directory.
  2. Adjusted suggested inclusion of settings.local.php to use __DIR__.
  3. Empty out obsolete sites/default/default.settings.php (retained for Drush only).
  4. Updated default.settings.php with latest HEAD.
  5. Changed 3rd instance of installer futzing with site directory permissions.
  6. Polished a few comments.
sun’s picture

FileSize
93.19 KB
907 bytes

Resolved default.settings.php merge conflict.

Should still be green.

Status: Needs review » Needs work

The last submitted patch, 163: site.163.patch, failed testing.

Jalandhar’s picture

Status: Needs work » Needs review
FileSize
93.19 KB

Updating patch with re-roll. Please review.

sun’s picture

FileSize
94.07 KB
3.91 KB

Merged 8.x.

sun’s picture

FileSize
94.02 KB

Merged HEAD.

Status: Needs review » Needs work

The last submitted patch, 167: site.167.patch, failed testing.

webchick’s picture

Issue tags: +Favourite of webchick

I still feel like moving away from sites/default is a really important thing to do in Drupal 8, given the changes that already went in for /modules and /themes. I don't however think it should block release nor beta.

Making up a tag to reflect this. :P

chx’s picture

Sigh. I don't like this. Most development workflows include a settings.php + settings.local.php which is now kind of officially sanctioned and with this arrangement now you need to put two (or more) in webroot where they really don't belong. I understand the resentment for sites/default , it was more happenstance than good design that settings.php landed there.

With all that said what about a settings directory?

./modules
./themes
./drivers
./settings

hrm?

tstoeckler’s picture

A settings directory is an interesting suggestion. Is your reservation to settings.php in the root the fact that it's next to index.php (and that it's something vastly different) or is there something else that I'm missing that speaks against the docroot? Just interested on the why of #170.

catch’s picture

Issue tags: +beta target, +D8 upgrade path

If we add a requirement it's going to break existing sites, so needs to happen before we're trying to offer an head to head upgrade path.

Quite like having a settings directory.

chx’s picture

I couldn't quite put a finger on it, it was a feeling, but now I can: so we have the root directory. What's there? Mostly, we can say, the things that must be there. You can't remove from the top directory, perhaps example.gitignore is the only one that could be.

So we put settings.php there.

Then comes custom development and puts settings.local.php there.

Oh, we need overrides. So along comes services.yml.

It just mushrooms and clutters the root dir.

Crell’s picture

Having a directory for site-specific "stuff" makes sense to me, and is consistent with what Symfony does as well. (Symfony fullstack has an "app" directory for such things, which includes both local and git-commitable config.) Whether that directory is top level or under the sites directory is, well, I don't much care. sites/default doesn't really bother me and is long-since committed to muscle memory. :-)

Requiring sites.php in order to trigger multi-site behavior, though, that makes total sense to me; it's unfortunate that those two (separate) issues have been coupled here as JUST making sites.php required to trigger the directory scanning logic should be a trivial change if we're not also moving stuff around.

tstoeckler’s picture

Re @Crell: Requiring sites.php to enable the multi-site discovery is in core already. Check DrupalKernel::findSitePath().

Crell’s picture

Title: Require settings.php to exist in the root folder and to explicitly opt-in for multi-site functionality » Move settings.php to site root, fold sites.php into settings.php

Huh. Then it seems like the title is woefully out of date.

pwolanin’s picture

At this point, I'm -1 to working on this for Drupal 8. I'm pretty much with Crell in the "don't care" camp as far as sites/default, and I agree with chx that it shouldn't be in the root folder. I don't really see this as bringing enough meaningful benefit to be worth the churn.

derheap’s picture

+1 for #170: a /settings folder.

I never liked the sites/default folder. For me it was one useless level of folders.
But cluttering the root folder with settings.* files is not much better.
The root folder should only contain files that can not go anywhere else.
So I would be very glad about changing /sites/default to /settings.

lpalgarvio’s picture

the /settings directory idea looks good.

webchick’s picture

Title: Move settings.php to site root, fold sites.php into settings.php » Move settings.php to /settings directory, fold sites.php into settings.php
chx’s picture

Version: 8.0.x-dev » 8.x-dev
Issue tags: -D8 upgrade path +Needs issue summary update
tim.plunkett’s picture

Version: 8.x-dev » 8.0.x-dev
derheap’s picture

Assigned: sun » derheap

Reassigning to me.
No progress for about a month.
I' ll try to make it work according to #170 and #180.

derheap’s picture

Assigned: derheap » Unassigned
FileSize
68.83 KB

That patch is basically a (broken) reroll of # 166 against HEAD.
My plan was do get a working reroll, and than move the files around.

As I do not enough about the bootstrapping process, I was not able to get it working.
Maybe I messed up things during merge conflict solving.
I can not figure out where do initalize the Site class.

So I give up. The files are not moved.

cilefen’s picture

Issue tags: +Needs reroll
derheap’s picture

Status: Needs work » Needs review
FileSize
70.21 KB

Need help would be more appropriate.

I made a reroll agains HEAD and moved the changes from drupal_initialize_settings to Site/Settings::initialize.
But the install process fails with

( ! ) Fatal error: Uncaught exception 'BadMethodCallException' with message 'Site path is initialized already.' in /Users/tobias/Sites/drupal-dev/d8.local/core/lib/Drupal/Core/Site/Site.php on line 111
( ! ) BadMethodCallException: Site path is initialized already. in /Users/tobias/Sites/drupal-dev/d8.local/core/lib/Drupal/Core/Site/Site.php on line 111

I dont know enought about to bootstrapping process to get it working.

The change according to #170 has not started yet.

Please help.

Status: Needs review » Needs work

The last submitted patch, 186: 1757536.186.diff, failed testing.

derheap’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

The last submitted patch, 184: 1757536.site_.184.patch, failed testing.

cilefen’s picture

Status: Needs review » Needs work

@derheap Good job pushing this issue along. There are a few things I noticed in the recent patch you must correct.

First, when applying the patch, some errors are produced, which is unusual. You must not use tabs to indent and there are whitespace errors. See https://www.drupal.org/coding-standards. You can use php code sniffer to check the code against Drupal's code style https://www.drupal.org/node/1419988. Also, it is possible to configure IDE's and editors to check the code while editing.

(8.0.x=)$ git apply 1757536-186.patch
1757536-186.patch:819: tab in indent.
		// Read the global /settings.php file.
1757536-186.patch:820: tab in indent.
		// Allow it to set/override the following variables:
1757536-186.patch:821: tab in indent.
		$sites = NULL;
1757536-186.patch:822: tab in indent.
		$conf_path = NULL;
1757536-186.patch:823: tab in indent.
		// Exclude it for test requests to prevent settings of the test runner from
warning: squelched 10 whitespace errors
warning: 15 lines add whitespace errors.
+++ b/core/modules/system/src/Tests/System/SettingsRewriteTest.php
@@ -7,8 +7,10 @@
+<<<<<<< HEAD:core/modules/system/src/Tests/System/SettingsRewriteTest.php

There is a merge conflict marker in the patch. I am going to stop here, but if you can get the things above corrected that would be great.

derheap’s picture

FileSize
69.85 KB

Fixed the patch according to #190.

@cilefen
Thanks for the review.
How can I get the whitespace warnings on git apply?

derheap’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 191: 1757536-191.patch, failed testing.

derheap’s picture

Status: Needs work » Needs review

Needs help.

cilefen’s picture

@derheap #191 applies cleanly now.

cilefen’s picture

Status: Needs review » Needs work

The testbot uses drush si and it expects sites/default/default.settings.php to exist but this file is moved to core/default.settings.php by #191.

(8.0.x *%=)$ sudo -u www drush si --db-url=mysql://drupal:drupal@localhost/drupal8x --account-name=admin --account-pass=admin --account-mail=admin@example.com
You are about to create a /Library/WebServer/Documents/drupal8x/sites/default/settings.php file and DROP all tables in your 'drupal8x' database. Do you want to continue? (y/n): y
copy(sites/default/default.settings.php): failed to open stream: No such file or directory drush.inc:689                           [warning]
Failed to copy sites/default/default.settings.php to /Library/WebServer/Documents/drupal8x/sites/default/settings.php 
+++ b/core/modules/update/update.manager.inc
@@ -36,6 +36,8 @@
+

There is an extra line here.

derheap’s picture

FileSize
97.11 KB

I installed 8.0.x and then moved the settings.php and services.yml to settings/.
It works!

The settingsfile is read.
For services.yml I am not quite sure.

But still the installer fails.

I put back the empty settings.php and service.yml to satisfy testbot.

derheap’s picture

derheap’s picture

What about the new services.yml file (#2251113)?
It is one more reason for a /settings directory.

It should also moved to /settings.
Should the work done in a seperate issue or in this?

RainbowArray’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 197: 1757536-197.patch, failed testing.

derheap’s picture

Status: Needs work » Needs review
dawehner’s picture

On top of that I wonder whether we should implement such a specific override capability for the services yml files as well?

  1. +++ b/core/default.settings.php
    @@ -288,6 +325,59 @@
    + * Twig debugging:
    + *
    + * When debugging is enabled:
    + * - The markup of each Twig template is surrounded by HTML comments that
    + *   contain theming information, such as template file name suggestions.
    + * - Note that this debugging markup will cause automated tests that directly
    + *   check rendered HTML to fail. When running automated tests, 'twig_debug'
    + *   should be set to FALSE.
    + * - The dump() function can be used in Twig templates to output information
    + *   about template variables.
    + * - Twig templates are automatically recompiled whenever the source code
    + *   changes (see twig_auto_reload below).
    + *
    + * Note: changes to this setting will only take effect once the cache is
    + * cleared.
    + *
    + * For more information about debugging Twig templates, see
    + * http://drupal.org/node/1906392.
    + *
    + * Not recommended in production environments (Default: FALSE).
    + */
    +# $settings['twig_debug'] = TRUE;
    +
    +/**
    + * Twig auto-reload:
    + *
    + * Automatically recompile Twig templates whenever the source code changes. If
    + * you don't provide a value for twig_auto_reload, it will be determined based
    + * on the value of twig_debug.
    + *
    + * Note: changes to this setting will only take effect once the cache is
    + * cleared.
    + *
    + * Not recommended in production environments (Default: NULL).
    + */
    +# $settings['twig_auto_reload'] = TRUE;
    +
    +/**
    + * Twig cache:
    + *
    + * By default, Twig templates will be compiled and stored in the filesystem to
    + * increase performance. Disabling the Twig cache will recompile the templates
    + * from source each time they are used. In most cases the twig_auto_reload
    + * setting above should be enabled rather than disabling the Twig cache.
    + *
    + * Note: changes to this setting will only take effect once the cache is
    + * cleared.
    + *
    + * Not recommended in production environments (Default: TRUE).
    + */
    +# $settings['twig_cache'] = FALSE;
    +
    
    @@ -450,7 +540,7 @@
    - * any added language. (eg locale_custom_strings_de for german).
    + * any enabled language. (eg locale_custom_strings_de for german).
    
    @@ -497,8 +597,8 @@
      * http://php.net/manual/ini.list.php
    - * See \Drupal\Core\DrupalKernel::bootEnvironment() for required runtime
    - * settings and the .htaccess file for non-runtime settings.
    + * See drupal_environment_initialize() in core/includes/bootstrap.inc for
    + * required runtime settings and the .htaccess file for non-runtime settings.
      * Settings defined there should not be duplicated here so as to avoid conflict
    
    @@ -553,16 +653,14 @@
    - * {config} table. To use a different storage mechanism for the active
    - * configuration, do the following prior to installing:
    - * - Override the 'bootstrap_config_storage' setting here. It must be set to a
    - *   callable that returns an object that implements
    - *   \Drupal\Core\Config\StorageInterface.
    - * - Override the service definition 'config.storage.active'. Put this
    - *   override in a services.yml file in the same directory as settings.php
    - *   (definitions in this file will override service definition defaults).
    + * {config} table. To install Drupal with a different active configuration
    + * storage, you need to override the setting here, in addition to overriding
    + * the config.storage.active service definition in a module or profile.
    + *
    + * The 'bootstrap_config_storage' setting needs to be a callable that returns
    + * core.services.yml.
      */
    -# $settings['bootstrap_config_storage'] = array('Drupal\Core\Config\BootstrapConfigStorageFactory', 'getFileStorage');
    + # $settings['bootstrap_config_storage'] = array('Drupal\Core\Config\BootstrapConfigStorageFactory', 'getFileStorage');
     
     /**
      * Configuration overrides.
    
    @@ -625,11 +723,9 @@
      * development, etc) installations of this site. Typically used to disable
    - * caching, JavaScript/CSS compression, re-routing of outgoing emails, and
    + * caching, JavaScript/CSS compression, re-routing of outgoing e-mails, and
      * other things that should not happen on development and testing sites.
      *
    - * Keep this code block at the end of this file to take full effect.
    + * Keep this include at the end of this file to take full effect.
      */
    -# if (file_exists(__DIR__ . '/settings.local.php')) {
    -#   include __DIR__ . '/settings.local.php';
    -# }
    +# include __DIR__ . '/settings.local.php';
    

    This seemed to be accidentally readded

  2. +++ b/core/lib/Drupal/Core/Site/Site.php
    @@ -0,0 +1,378 @@
    +class Site {
    

    Is there a reason we haven't tried to split up the class into one non-singleton and a singleton helper?

  3. +++ b/core/modules/system/src/Tests/System/SettingsRewriteTest.php
    @@ -7,8 +7,10 @@
     
    +<<<<<<< HEAD:core/modules/system/src/Tests/System/SettingsRewriteTest.php
     use Drupal\Core\Site\Settings;
     use Drupal\simpletest\KernelTestBase;
    +use Drupal\Core\Site\Site;
     
    
    +++ b/core/modules/system/system.install
    @@ -215,14 +216,18 @@ function system_requirements($phase) {
    +<<<<<<< HEAD
    

    ups

  4. +++ b/core/modules/update/update.manager.inc
    @@ -304,7 +305,7 @@ function update_manager_local_transfers_allowed() {
    diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml
    
    diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml
    index 280eb43..792d600 100644
    
    index 280eb43..792d600 100644
    --- a/sites/default/default.services.yml
    
    --- a/sites/default/default.services.yml
    +++ b/sites/default/default.services.yml
    
    +++ b/sites/default/default.services.yml
    +++ b/sites/default/default.services.yml
    @@ -1,53 +1 @@
    
    @@ -1,53 +1 @@
    +#
    diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php
    
    diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php
    index 017fe8a..b3d9bbc 100644
    
    index 017fe8a..b3d9bbc 100644
    --- a/sites/default/default.settings.php
    
    --- a/sites/default/default.settings.php
    +++ b/sites/default/default.settings.php
    
    +++ b/sites/default/default.settings.php
    +++ b/sites/default/default.settings.php
    @@ -1,635 +1 @@
    
    @@ -1,635 +1 @@
     <?php
    -
    

    Why did those files not got removed?

derheap’s picture

@dawehner

On top of that I wonder whether we should implement such a specific override capability for the services yml files as well?

Maybe. But that should be another issue.

1. Hm. I moved the sites/default/default.settings.php to core/default.settings.php. (Thats part of the issue). So that should be correct. The contents seems to be mangled up. I will try again.

2. I do not know. I just try (and fail) to make this issue working again.

3. That should not happen. I will fix it.

4. The files have to stay there until testbot is no longer dependent on drush. Without these files testbot fails immediately.

I'll do another reroll.

My big problem: the install fails with the patch. There has been to much change, that I dont fully understand to get install working again. Please help.

derheap’s picture

FileSize
83.36 KB

Ok, lets try again.
The patch works on an fresh installed site, but a new install will fail.

Status: Needs review » Needs work

The last submitted patch, 205: 1757536-205.patch, failed testing.

derheap’s picture

FileSize
83.89 KB
cilefen’s picture

@derheap Good work on continuing this issue. There's no need to ask for help—it is better to include more information like log output of the errors because some people will know at a glance what to do and make a suggestion. This is a difficult issue.

derheap’s picture

Here is the error from the installer:

( ! ) Fatal error: Uncaught exception 'BadMethodCallException' with message 'Site path is initialized already.' in /Users/tobias/Sites/drupal-dev/d8.local--clean/core/lib/Drupal/Core/Site/Site.php on line 111
( ! ) BadMethodCallException: Site path is initialized already. in /Users/tobias/Sites/drupal-dev/d8.local--clean/core/lib/Drupal/Core/Site/Site.php on line 111
Call Stack
#	Time	Memory	Function	Location
1	0.0005	237376	{main}( )	../install.php:0
2	0.0084	967800	install_drupal( )	../install.php:32
3	0.0085	971792	install_begin_request( )	../install.core.inc:99
4	0.0150	2018208	Drupal\Core\Site\Site::initInstaller( )	../install.core.inc:297

As far as I have tracked it down Settings::initialize() gets called twice.
Once from the "normal" rendering of a page from an installed site and then when the installer kicks in.

@cilefen

There's no need to ask for help

Yes, but I already posted the error some comments (#186) ago. Sometime I just feel lost in the queue …

This is a difficult issue.

You are right, its way more difficult than I expected. It is just a cleanup I would like to see, so I dived in.

cilefen’s picture

@derheap If it is possible, join #drupal and #drupal-contribute in IRC. By doing that you can find out what's going on in real-time and learn who works on what. Also, maybe work on other issues concurrently so you can see the different stages towards completion.

andypost’s picture

I'd beware a 2 singletons at all... the related issue #2199795: Make the Settings class prevent serialization of actual settings points example.
$sites variable is more then needed to negotiate the site folder.
please keep in mind a domain module could extend or reuse this about... files for example, otoh that is a stream wrapper...

err: Issue summary outdated, it's not clear what's left todo and what kind of help needed, is there any architectural reviews?

anyway

+++ b/core/INSTALL.txt
@@ -148,40 +148,39 @@ INSTALLATION
+      yourself. Use the template core/default.settings.php or
+      core/default.services.yml respectively.
...
-        cp sites/default/default.services.yml sites/default/services.yml
...
+        cp core/default.services.yml settings/services.yml
...
-        chmod a+w sites/default/services.yml
...
+        chmod a+w settings/services.yml
...
-        chmod go-w sites/default/services.yml
...
+        chmod go-w settings/services.yml

+++ b/core/UPGRADE.txt
similarity index 100%
copy from sites/default/default.services.yml

copy from sites/default/default.services.yml
copy to core/default.services.yml

Looks a lot of services and settings?
services could provide some negotiation for conf_path() or use some sort of sites.YML instead

derheap’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 207: 1757536-206.patch, failed testing.

derheap’s picture

Status: Needs work » Needs review
FileSize
83.07 KB

Status: Needs review » Needs work

The last submitted patch, 214: 1757536-213.patch, failed testing.

jhedstrom’s picture

Is this still under consideration for 8.0.x?

jhedstrom’s picture

Version: 8.0.x-dev » 8.1.x-dev

Feature request => 8.1.x.

webchick’s picture

Version: 8.1.x-dev » 8.0.x-dev
Category: Feature request » Task

Hm. I would rather recategorize it as a task, and put it back at 8.0.x, with the caveat that it must pass a beta evaluation to still be considered for 8.0.x. It's very confusing as it is now that modules/themes/profiles works one way and files/settings does not, so in that respect this is "finishing existing work" rather than "adding something new."

joelpittet’s picture

@webchick is this still on the plate? It hasn't been picked up (as likely people are working on criticals I'd assume) and this is the first I've seen it. I'll put it on my plate after SafeMarkup criticals are licked if so.

pwolanin’s picture

Issue tags: +Needs beta evaluation

This seems, at least, disruptive for existing sites and there has been no beta evaluation.

This should probably be bumped to 9 now?

webchick’s picture

Issue tags: +rc deadline

A girl can dream.

Manjit.Singh’s picture

Issue tags: +Needs reroll

This would be the huge one.. massive changes :(

Only local images are allowed.

dawehner’s picture

Version: 8.0.x-dev » 9.x-dev

Yeah this is certainly something we should tackle in 9.

plach’s picture

Can't we add support for both locations in a minor release?

catch’s picture

We can't do that without a file_exists() check which would introduce disk i/o we don't want when the file is not there.

If we're somehow able to make that file_exists() neutral on sites that update to the new layout (for example not looking for sites.php if it's found), that might be a worthwhile trade-off to have the forwards-compatibility.

frob’s picture

Version: 9.x-dev » 8.0.x-dev
Issue tags: -Needs reroll

I like this idea, I was always having to explain /sites/default/settings.php vs /sites/[site]/settings.php vs /sites/default/default.settings.php

I have to say this confusion lasts around 2 minutes tops for the least experienced Drupal dev.

So if we are going to be doing this. What needs to be done? I see lots of failing patches.

frob’s picture

Issue tags: +Needs reroll

oops, adding back tag

catch’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Postponed

We'd need to figure out how to support it for new installs and those that want to update, without breaking existing installs, for 8.1.x.

Fabianx’s picture

There is probably a simple solution in 8.1.x, which can be provided with an upgrade path.

We can put a generic sites/settings.php in after installation, then this one has a BC compatible version for upgrade path (with file_exists) and a new version for newer installs.

For the newer version there is no need for file_exists() checks, because either the BC compatible sites/settings.php is in charge or the new settings.php, there is no real BC break for existing sites, but new sites easily get the speed benefit.

That is how I would solve it.

claudiu.cristea’s picture

Status: Postponed » Needs work

I think this should be addressed with BC in 8.1.x.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

This issue was marked as a beta target for the 8.0.x beta, but is not applicable as an 8.1.x beta target, so untagging.

vprocessor’s picture

rerolled, waiting for tests

vprocessor’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 233: 1757536-233.patch, failed testing.

hitesh-jain’s picture

Assigned: Unassigned » hitesh-jain
hitesh-jain’s picture

Assigned: hitesh-jain » Unassigned

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Manuel Garcia’s picture

Patch #233 needs a reroll

eporama’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
0 bytes

Here is a straight reroll of #233 against 8.3.x

daffie’s picture

Status: Needs review » Needs work

@eporama: I am sorry, but your patch is an empty patch.

eporama’s picture

Hmm. not sure what happened. Trying again.

The last submitted patch, 240: move_settings_php_to-1757536-240.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 242: move_settings_php_to-1757536-242.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jibran’s picture

Issue tags: +Needs reroll

Do we still need this? If yes then should we address this in D9 branch before 9.0.0 is released?

frob’s picture

My thoughts is this and more aught to be done for D9. I still think this should be put into D8 with BC and just remove the BC for D9.

dafink’s picture

Status: Needs work » Needs review
FileSize
26.39 KB

https://www.drupal.org/files/issues/2019-02-27/core-n1757536-250.patch

Rerolled patch from comment 242. No other changes made.

Status: Needs review » Needs work

The last submitted patch, 251: core-n1757536-250.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ricardoamaro’s picture

I rebased the patch from #242 and moved the default.services.yml and default.settings.php to core/

On the update.inc "use Drupal\Core\DrupalKernel;" probably still needs to be looked at.

bircher’s picture

Status: Needs work » Needs review

Worked with ricardo on this during Drupalcon
Setting to needs-review so that the drupal CI runs

ricardoamaro’s picture

ricardoamaro’s picture

Fixed 27 coding standards messages.

ricardoamaro’s picture

Fixing the last coding standards issue.

AjitS’s picture

Issue tags: -Needs reroll

@dafink - Thank you for the reroll. It is adviced that once you reroll the patch you remove the "Needs reroll" tag.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Anybody’s picture

Wouldn't it make sense to introduce this change with Drupal 10? I think this approach has several benefits and the current situation is from history.

Comment on backwards compatibility can be seen in #229.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Anybody’s picture

Category: Task » Feature request
Priority: Major » Normal

I guess 10 years later we can clearly say this doesn't seem major ;) Also changing this to a feature request for further discussion if and how to proceed here. Still I think the idea is nice, but no traction anymore.

joachim’s picture

> Introduce a new Site singleton class to replace conf_path().

This falls under #2282779: [meta] DrupalKernel has too many responsibilities and could be done as part of that.

> Move /sites/default/default.settings.php into /core/default.settings.php, so it is updated when Drupal core is updated.

I like the idea is this getting updated, but there is a loss of DX because you have to copy the file between directories instead of within the same one. But again, could be split off to its own issue, as it doesn't depend on this one.

frob’s picture

Why do we need to stick with a settings.php file? I rewrote our settings.php to get everything from environment variables. I would much rather have a settings.yml that can be overridden with environment variables than a php file that needs to be maintained.

ENV_DB__default__default__database=dbname
ENV_DB__default__default__username=dbuser
ENV_DB__default__default__password=dbpass

The above structure maps nicely to the database array structure.

bradjones1’s picture

Yes, in theory the correct answer here is that the Yaml loader supports environment variable interpolation and Drupal configures itself from the DIC parameters.

longwave’s picture

@joachim re #268

> Move /sites/default/default.settings.php into /core/default.settings.php, so it is updated when Drupal core is updated.

This is solved already if you use the scaffolding plugin - this will now update /sites/default/default.settings.php.

Re #269/270 @alexpott and I discussed this briefly at Drupalcon and we are generally +1 for making settings.php smaller (or even trying to remove it entirely), but there are still some tricky problems: if we cache a compiled container in the database, how do we load it again, given that the connection parameters are inside the container? See #3299828-61: Stop storing Settings singleton in object properties for more on this topic.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
FileSize
148 bytes

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.