Problem
-
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. -
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.
-
In addition, because the multi-site concept is enabled by default, additional calls to
file_exists()
are required on all sites (whethersites.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
-
Require a top-level, global
/settings/settings.php
to exist in any case (set up by the installer). -
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:
- 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. - 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. - 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:
- 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. - The root
/settings/settings.php
is always loaded first. The site-specificsites/foo/settings.php
has access to all of the global variables and is able to override them in a granular way.
- If the root
-
→ (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 -
Introduce a new
Site
singleton class to replaceconf_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');
-
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
Comment | File | Size | Author |
---|---|---|---|
#273 | 1757536-nr-bot.txt | 148 bytes | needs-review-queue-bot |
#258 | interdiff-1757536-242-258.txt | 45.71 KB | ricardoamaro |
#258 | issue1757536_4.patch | 48.59 KB | ricardoamaro |
#257 | interdiff-1757536-242-257.txt | 45.17 KB | ricardoamaro |
#257 | issue1757536_3.patch | 49.13 KB | ricardoamaro |
Comments
Comment #1
webchickI 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.
Comment #2
naxoc CreditAttribution: naxoc commentedYeah, 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?
Comment #3
mariomaric CreditAttribution: mariomaric commentedAnother idea:
sites/default
->site
So we would have in root folder, based on other related issues, this situation:
Comment #4
lpalgarvio CreditAttribution: lpalgarvio commentedsite & 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
Comment #5
mariomaric CreditAttribution: mariomaric commentedOf 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 insite.php
file - and documentation (insite.php
e.g.) could recommend (or dictate?) to place those root directories intosites
folder - and completely ignoresite
directory.Comment #6
naxoc CreditAttribution: naxoc commentedI vote for:
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?
Comment #7
Crell CreditAttribution: Crell commented"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.
Comment #8
webchickThe simplicity is now that /core is the only directory you need to worry about. Everything else is your stuff.
Comment #9
sunIf I understood @catch correctly, then he'd like to:
/settings.php
to the top-level./settings.php
to exist in all cases. (removing one or more filestats)/settings.php
with an explicit flag along the lines of:Or alternatively - just simply move the entirety of
$sites
andsites/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.Comment #10
sunAnd 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 :)
Comment #12
lpalgarvio CreditAttribution: lpalgarvio commentedfrom 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 /):
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):
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 /):
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):
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 /):
everything is mixed and there is no clear distinction of default site vs stuff for all sites vs multisite. this is horrible IMO.
Comment #13
sunWow... ;) Let me try to address some concerns:
Mostly correct.
$sites
) and retain entirely different settings.php files per site. Freedom of choice. (I manually tested both already.)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? :))
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.
Comment #14
lpalgarvio CreditAttribution: lpalgarvio commentedawesome logic there =D
i wasn't aware that the parsing of settings.php was so advanced. that's great!
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!!
Comment #15
sunCoolio, 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.
Comment #16
klonosSeems 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?
Comment #17
Crell CreditAttribution: Crell commentedI'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.
Comment #18
sunThat'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: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.
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.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:
(already in HEAD - pasted from default.settings.php)
Comment #19
Crell CreditAttribution: Crell commentedWow, 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.
Comment #20
sunI 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
? :)
Comment #21
joshk CreditAttribution: joshk commented+1 for the concept. Having settings in a more obvious/common place for the common case is a Good Thing(tm).
Comment #22
joshk CreditAttribution: joshk commentedIn 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:
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.
Comment #23
klonos#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.
Comment #24
tstoecklerI 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).
Comment #25
klonosI 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?
Comment #26
lpalgarvio CreditAttribution: lpalgarvio commented#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:
Comment #27
webchickWhat 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.)
Comment #28
Damien Tournoud CreditAttribution: Damien Tournoud commentedI 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:
/files
as the files directory/sites/site1
and/sites/site2
, with the common modules at the top-levelComment #29
catchI'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..
Comment #30
mikeytown2 CreditAttribution: mikeytown2 commentedCSS/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.
Comment #31
sunI 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:
If there's no /settings.php, you will install a mono-site. There won't be a
/sites/*
folder post installation.If there's a /settings.php but it doesn't define the $sites variable, you will install a mono-site.
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.
Comment #32
sunWent 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.
Comment #34
sunAllow global /settings.php to set a custom $conf_path. (might also fix that single test failure)
Comment #35
mikeytown2 CreditAttribution: mikeytown2 commentedI 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
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.
Comment #36
sunI 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.
Comment #37
sunAs 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.
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.
Comment #38
moshe weitzman CreditAttribution: moshe weitzman commentedThis is looking good., Anyone want to push it along further?
Comment #39
sunWell, 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?Comment #42
sunMerged HEAD.
Comment #44
sunMerged HEAD.
Comment #45
sunThe remaining issue is still that
conf_path()
should be empty. To clarify: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:
/path
. With that, an empty relative path becomes an absolute path.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:
Make
conf_path()
always return a path that ends in a slash and change all callers, so they do: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 ondrupal_get_path()
, which in turn might return a path prefixed with./
...Invent some fancy new filesystem path resolver service à la:
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.)
Comment #49
sunMerged HEAD.
#45 still remains TBD.
Comment #54
plachYep, 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.
Comment #55
derEremit CreditAttribution: derEremit commentedalso 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
Comment #56
pwolanin CreditAttribution: pwolanin commentedseems this is duplicate to something that was committed?
Comment #57
webchickI 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.
Comment #58
tstoecklerHere's a straight re-roll.
Comment #60
tstoecklerThis needed a re-roll but git was smart enough to do that.
Comment #62
sunThanks 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.
Comment #63
tstoecklerSo 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.
Comment #64
tstoecklerWell, #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.:
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.
Comment #65
tstoecklerThis 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.
Comment #68
sunVery interesting! — I was literally working on almost the exact same thing, just without classes. :)
Instead of a new
Site
class, I went with a newsite_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 viasettings.php
, which is discovered + loaded viaconf_path()
, which presents a race-condition.The way we're overcoming that situation for
Settings
is via this hard-coded line indrupal_settings_initialize()
: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:
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 theSite
class methods later on.conf_path()
and thus the newSite
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.reset()
method on theSite
class — I don't think that should be possible, since the site/conf path cannot be changed at (regular) runtime.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.
Comment #69
sunTested 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:Comment #71
sunComment #74
tstoecklerWow, quite some changes. Here's a consecutive review of your comments:
#68:
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.
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.
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.
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:
Good idea! I always just copied the existing settings.php, but that's way cooler.
This is incorrect. Within a @code block "URL:" is not valid.
(Also elsewhere)
I still think it makes sense to keep this todo, IMO.
Wow, this was massively broken before, or am I missing something?? That seems quite strange.
Why is that no longer true?
Awesome. ++
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)
Needs docs.
Yes, please!!!
Awesome, I didn't know you could do private __construct()s. Cool!
(For private vs. protected see above)
Actually that's not true, as far as I'm aware. Simpletest initiates the singleton manually earlier in the request.
This is incorrect as self::$instance->resolvePath($filepath) may return an empty string.
#71:
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.
Yay!
Even more yay!
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
That can be collapsed into one condition.
Yes, that's what I mentioned above :-)
Glad we're on the same page.
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.
Comment #75
sunThanks 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:
re: re: #69:
re: re: #71:
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?
Comment #78
tstoecklerCoolio. Only replying to the things I disagree with to keep this short.
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.
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.
Comment #79
sunThere are two existing patches that fix bugs in the installer, which would be of tremendous help to move forward here:
Comment #80
sunComment #82
sunOh my, finally figured it out:
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. :(/settings.php
file. Attached patch fixes that, by adjustinginstall_check_requirements()
to pass absolute filepaths todrupal_verify_install_file()
, since the site directory filesystem checks previously resulted infile_exists('')
in there. ;)With that, testbot should finally start to run again. But most likely, all web tests will blow up in mighty ways. ;) Let's see. :)
Comment #84
sunIt 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.
Comment #86
sunFixed drupal_valid_test_ua() check for interactive installer in install_begin_request().
Comment #88
sunI 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
Comment #90
pwolanin CreditAttribution: pwolanin commentedI 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...
Comment #91
tstoecklerHad 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.
Comment #92
tstoecklerMaybe something like this?
Comment #93
tstoecklerAlso: 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 :-)
Comment #94
sunTemporarily postponing this issue until the blocking #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead is in.
Comment #95
jibranBack to NR.
Comment #96
sunAwesome. :)
Comment #98
sunThis patch could/should come back green. :-)
Comment #100
sunComment #107
longwaveI 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?
Comment #109
jibranplease move isset checks out of function call.
array :S
Comment #111
jibranI asked this question in IRC
and here is the answer by @sun
http://en.wikipedia.org/wiki/Singleton_pattern
Thanks @sun for explaining this, now the patch is making a lot of sense.
Comment #113
donquixote CreditAttribution: donquixote commentedI 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:
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.
Comment #114
donquixote CreditAttribution: donquixote commentedThis won't work :)
The $conf_path is not defined at this point.
Comment #115
sun@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 thisSite
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.
Comment #116
donquixote CreditAttribution: donquixote commented@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.
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).
The static instance within class Site will then really be instantiated only once and won't change later in the request - except for testing.
Comment #117
sun@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:
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. :-)
Comment #119
sunFixed undefined variable in system_requirements().
Comment #124
sunComment #126
sunThis issue is now officially blocked on Drush usage on testbots:
:-( Why are we using Drush on the testbots? It only appears to be used for that single site-install command...?
Comment #127
donquixote CreditAttribution: donquixote commentedOne 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?
Comment #128
sunNote: 30KB increase in patch size is caused by copy of default.settings.php.
Comment #130
sunFixed merge conflicts.
Comment #131
sunGREEEEEN! Finally! :-)
I'll try to update the issue summary as soon as possible. But technical code reviews are welcome already.
Comment #132
jibranAn :)
Comment #133
donquixote CreditAttribution: donquixote commentedclass Site needs some @throws tags.
Comment #134
tstoecklerShould/Could be a follow-up but I just thought we could make this just a bit cooler:
Comment #135
tstoecklerWrote a draft change notice. Sadly, I can't RTBC this, though.
Comment #136
donquixote CreditAttribution: donquixote commentedI 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-...
Comment #137
tstoecklerI'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.
Comment #138
larowlanyeah 'a utility' is correct.
English--
Comment #139
sunSplFileInfo
/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
intoSite::init()
- which isn't possible right now, becausedrupal_settings_initialize()
does not have access to aRequest
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 theSite
class is retained, we can improve the internal implementation at any time, because initialization of theSite
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 theSite
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.
Comment #140
sunUpdated core documentation files.
Comment #145
sunMerged 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.
Comment #146
sun#145 still applies cleanly.
Comment #147
sunMerged 8.x.
Comment #149
moshe weitzman CreditAttribution: moshe weitzman commentedFYI, 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.
Comment #150
sunMerged 8.x. — Samples of the failing tests are passing for me locally, so trying once more against testbot.
Comment #152
sunComment #153
sunMerged 8.x.
Comment #154
pwolanin CreditAttribution: pwolanin commentedI'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.
Comment #156
sunClarified 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
Comment #157
tstoeckler153: site.153.patch queued for re-testing.
Comment #158
dawehnerI really like this patch!
I wonder whether we should split up the functionality of file vs. directory into two methods.
Just to understand it: given that DRUPAL_ROOT/default.settings.php never needs to be copied this exception is not helpful anymore.
You will never know whether you might want to subclass it.
It would be great to explain under which circumstances we throw these exceptions.
Comment #159
sunThanks!
Site::getPath()
for files vs. directories is an interesting thought. — Need to think about that some more. Could also possibly be a follow-up?Comment #160
dawehnerSure! I don't think that there is a win in getting crazy about API changes, especially at very special APIs.
This statement seems to be fine under the assumption that we control all the code, which is potentially not true.
Comment #161
pwolanin CreditAttribution: pwolanin commentedI'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?
Comment #162
sunComment #163
sunResolved default.settings.php merge conflict.
Should still be green.
Comment #165
Jalandhar CreditAttribution: Jalandhar commentedUpdating patch with re-roll. Please review.
Comment #166
sunMerged 8.x.
Comment #167
sunMerged HEAD.
Comment #169
webchickI 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
Comment #170
chx CreditAttribution: chx commentedSigh. 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?
Comment #171
tstoecklerA
settings
directory is an interesting suggestion. Is your reservation tosettings.php
in the root the fact that it's next toindex.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.Comment #172
catchIf 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.
Comment #173
chx CreditAttribution: chx commentedI 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.
Comment #174
Crell CreditAttribution: Crell commentedHaving 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.
Comment #175
tstoecklerRe @Crell: Requiring sites.php to enable the multi-site discovery is in core already. Check DrupalKernel::findSitePath().
Comment #176
Crell CreditAttribution: Crell commentedHuh. Then it seems like the title is woefully out of date.
Comment #177
pwolanin CreditAttribution: pwolanin commentedAt 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.
Comment #178
derheap CreditAttribution: derheap commented+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.
Comment #179
lpalgarvio CreditAttribution: lpalgarvio commentedthe /settings directory idea looks good.
Comment #180
webchickComment #181
chx CreditAttribution: chx commentedComment #182
tim.plunkettComment #183
derheap CreditAttribution: derheap commentedReassigning to me.
No progress for about a month.
I' ll try to make it work according to #170 and #180.
Comment #184
derheap CreditAttribution: derheap commentedThat 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.
Comment #185
cilefen CreditAttribution: cilefen commentedComment #186
derheap CreditAttribution: derheap commentedNeed 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
I dont know enought about to bootstrapping process to get it working.
The change according to #170 has not started yet.
Please help.
Comment #188
derheap CreditAttribution: derheap commentedComment #190
cilefen CreditAttribution: cilefen commented@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.
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.
Comment #191
derheap CreditAttribution: derheap commentedFixed the patch according to #190.
@cilefen
Thanks for the review.
How can I get the whitespace warnings on git apply?
Comment #192
derheap CreditAttribution: derheap commentedComment #194
derheap CreditAttribution: derheap commentedNeeds help.
Comment #195
cilefen CreditAttribution: cilefen commented@derheap #191 applies cleanly now.
Comment #196
cilefen CreditAttribution: cilefen commentedThe 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.There is an extra line here.
Comment #197
derheap CreditAttribution: derheap commentedI 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.
Comment #198
derheap CreditAttribution: derheap commentedComment #199
derheap CreditAttribution: derheap commentedWhat 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?
Comment #200
RainbowArrayComment #202
derheap CreditAttribution: derheap commentedComment #203
dawehnerOn top of that I wonder whether we should implement such a specific override capability for the services yml files as well?
This seemed to be accidentally readded
Is there a reason we haven't tried to split up the class into one non-singleton and a singleton helper?
ups
Why did those files not got removed?
Comment #204
derheap CreditAttribution: derheap commented@dawehner
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.
Comment #205
derheap CreditAttribution: derheap commentedOk, lets try again.
The patch works on an fresh installed site, but a new install will fail.
Comment #207
derheap CreditAttribution: derheap commentedComment #208
cilefen CreditAttribution: cilefen commented@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.
Comment #209
derheap CreditAttribution: derheap commentedHere is the error from the installer:
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
Yes, but I already posted the error some comments (#186) ago. Sometime I just feel lost in the queue …
You are right, its way more difficult than I expected. It is just a cleanup I would like to see, so I dived in.
Comment #210
cilefen CreditAttribution: cilefen commented@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.
Comment #211
andypostI'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
Looks a lot of services and settings?
services could provide some negotiation for conf_path() or use some sort of sites.YML instead
Comment #212
derheap CreditAttribution: derheap commentedComment #214
derheap CreditAttribution: derheap commentedComment #216
jhedstromIs this still under consideration for 8.0.x?
Comment #217
jhedstromFeature request => 8.1.x.
Comment #218
webchickHm. 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."
Comment #219
joelpittet@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.
Comment #220
pwolanin CreditAttribution: pwolanin as a volunteer commentedThis seems, at least, disruptive for existing sites and there has been no beta evaluation.
This should probably be bumped to 9 now?
Comment #221
webchickA girl can dream.
Comment #222
Manjit.SinghThis would be the huge one.. massive changes :(
Comment #223
dawehnerYeah this is certainly something we should tackle in 9.
Comment #224
plachCan't we add support for both locations in a minor release?
Comment #225
catchWe 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.
Comment #226
frobI 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.
Comment #227
froboops, adding back tag
Comment #228
catchWe'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.
Comment #229
Fabianx CreditAttribution: Fabianx at Tag1 Consulting commentedThere 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.
Comment #230
claudiu.cristeaI think this should be addressed with BC in 8.1.x.
Comment #232
xjmThis 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.
Comment #233
vprocessor CreditAttribution: vprocessor at Skilld commentedrerolled, waiting for tests
Comment #234
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #236
hitesh-jain CreditAttribution: hitesh-jain at Acquia commentedComment #237
hitesh-jain CreditAttribution: hitesh-jain at Acquia commentedComment #239
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedPatch #233 needs a reroll
Comment #240
eporama CreditAttribution: eporama as a volunteer and at Acquia commentedHere is a straight reroll of #233 against 8.3.x
Comment #241
daffie CreditAttribution: daffie commented@eporama: I am sorry, but your patch is an empty patch.
Comment #242
eporama CreditAttribution: eporama as a volunteer and at Acquia commentedHmm. not sure what happened. Trying again.
Comment #249
jibranDo we still need this? If yes then should we address this in D9 branch before 9.0.0 is released?
Comment #250
frobMy 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.
Comment #251
dafink CreditAttribution: dafink as a volunteer commentedhttps://www.drupal.org/files/issues/2019-02-27/core-n1757536-250.patch
Rerolled patch from comment 242. No other changes made.
Comment #254
ricardoamaro CreditAttribution: ricardoamaro as a volunteer and at Acquia commentedI 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.
Comment #255
bircherWorked with ricardo on this during Drupalcon
Setting to needs-review so that the drupal CI runs
Comment #256
ricardoamaro CreditAttribution: ricardoamaro as a volunteer and at Acquia commentedFixed the PHPLint
Comment #257
ricardoamaro CreditAttribution: ricardoamaro as a volunteer and at Acquia commentedFixed 27 coding standards messages.
Comment #258
ricardoamaro CreditAttribution: ricardoamaro as a volunteer and at Acquia commentedFixing the last coding standards issue.
Comment #259
AjitS@dafink - Thank you for the reroll. It is adviced that once you reroll the patch you remove the "Needs reroll" tag.
Comment #263
AnybodyWouldn'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.
Comment #267
AnybodyI 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.
Comment #268
joachim CreditAttribution: joachim at Factorial GmbH commented> 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.
Comment #269
frobWhy 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.
The above structure maps nicely to the database array structure.
Comment #270
bradjones1Yes, in theory the correct answer here is that the Yaml loader supports environment variable interpolation and Drupal configures itself from the DIC parameters.
Comment #271
longwave@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.
Comment #273
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.