Updated: Comment #N

Problem/Motivation

As change record https://drupal.org/node/1792924 Multi-site functionality is now opt-in the install instructions should change accordingly.

Proposed resolution

Rewrite and drop portions of the INSTALL.txt instructions

Remaining tasks

Needs work on the paragraph order.

User interface changes

API changes

CommentFileSizeAuthor
#20 core-multi-site-install-2168617-20-do-not-test.patch4.58 KBclemens.tolboom
#18 core-multi-site-install-2168617-18.patch4.58 KBclemens.tolboom
#16 core-multi-site-install-2167347-14.patch0 bytesclemens.tolboom
#16 interdiff-2167347-12-14.txt1.6 KBclemens.tolboom
#14 interdiff-2168617-12-14.txt1.25 KBclemens.tolboom
#14 core-multi-site-install-2168617-14-do-not-test.patch3.49 KBclemens.tolboom
#12 interdiff-2168617-6-12.txt1.2 KBclemens.tolboom
#12 core-multi-site-install-2168617-12-do-not-test.patch3.34 KBclemens.tolboom
#6 core-multi-site-install-2168617-6-do-not-test.patch3.21 KBclemens.tolboom
core-multi-site-install-do-not-test.patch2.33 KBclemens.tolboom
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented
Related issues: +#1757536: Move settings.php to /settings directory, fold sites.php into settings.php
clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented
Title: INSTALL.txt for multisite is not conform changenotice » INSTALL.txt for multisite is not conform change record
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Title: INSTALL.txt for multisite is not conform change record » INSTALL.txt has not been updated to match multisite functionality change
Status: Needs review » Needs work

According to my read of
https://drupal.org/node/1792924
the scanning stuff still works as it used to, but is only invoked if the sites.php file exists. The documentation in sites.php agrees with that interpretation.

So this patch isn't correct. All it needs is an addition to say that the multi-site scanning and the possibility of using multiple different settings.php files doesn't take place unless there is a sites.php file present.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented

Thanks for correcting me. I'll need to test this again.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

If the interpretation in #3 is incorrect, then we need to update:
- that change notice
- the INSTALL.txt file
- the documentation in sites.php

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented
Status: Needs work » Needs review
FileSize
core-multi-site-install-2168617-6-do-not-test.patch3.21 KB

My assumption require every dir as an entry in sites/sites.php was wrong.

Attached patch fixes for that.

sites/example.sites.php needed a change as it _is_ required to kick-in the multisite feature.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

I do not think we want to remove these lines from INSTALL.php:

 Additional site configurations are created in subdirectories within the 'sites'
 directory. Each subdirectory must have a 'settings.php' file, which specifies
-the configuration settings. The easiest way to create additional sites is to
-copy the 'default' directory and modify the 'settings.php' file as appropriate.
-The new directory name is constructed from the site's URL. The configuration for
-www.example.com could be in 'sites/example.com/settings.php' (note that 'www.'
-should be omitted if users can access your site at http://example.com/).
+the configuration settings. Make a copy of sites/default/default.settings.php
+to your new site directory.

The changes to sites.php look good!

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented
Status: Needs work » Needs review

the configuration settings. The easiest way to create additional sites is to
-copy the 'default' directory and modify the 'settings.php' file as appropriate.
-The new directory name is constructed from the site's URL. The configuration for
-www.example.com could be in 'sites/example.com/settings.php' (note that 'www.'

This is now very bad practice as the config files are named in settings.php so people get either

  1. WSOD as the kept settings for config_* still points to sites/default
  2. path does not exists error when they changed the sites/defaults/files/config_* to map their new site/my-sites/files/config_*
  3. two sites having the same config_* value which is an attack vector
  4. using the same drupal_hash_salt

This suggestion should really go :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

True, we need to say to start with default.settings.php and not settings.php. But we still need the information about the name of the directory mapping to the site URL, which was removed.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented
Assigned: Unassigned » clemens.tolboom

Issue disappeared from my radar :-/

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented

Patch must be reapplied due to #2083733: Add instructions for reinstall to INSTALL.txt

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented
Status: Needs work » Needs review
FileSize
core-multi-site-install-2168617-12-do-not-test.patch3.34 KB
interdiff-2168617-6-12.txt1.2 KB

Rerolled.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

Thanks for the reroll!

I gave this patch a review, and found a typo:

+copy file 'default.settings.php' from the 'sites/default' directory into the new
+site directory into the a 'settings.php' file and modify as appropriate.

Other than that, I think it is clear. However, I think we also need a patch for the top portion of default.settings.php, which doesn't make it clear that you need sites.php in order fo this to work.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented
Status: Needs work » Needs review
FileSize
core-multi-site-install-2168617-14-do-not-test.patch3.49 KB
interdiff-2168617-12-14.txt1.25 KB

I rephrased around the typo.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

That looks fine, thanks!

Another typo I missed in the last review, in INSTALL.txt:

+For this to work you need the file sites/sites.php to exists. Make a copy of

exists -> exist

And we still need some changes to be made to default.settings.php.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented
Status: Needs work » Needs review
FileSize
interdiff-2167347-12-14.txt1.6 KB
core-multi-site-install-2167347-14.patch0 bytes

Fixed type and adjusted default.settings.php

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Can you upload the patch again? It seems to be empty.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented
FileSize
core-multi-site-install-2168617-18.patch4.58 KB
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

Thanks!

One error:

+ * sites/sites.php must be present. It's optional settings will be loaded, and

It's should be Its.

That small fix and I think we'll be done!

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented
FileSize
core-multi-site-install-2168617-20-do-not-test.patch4.58 KB
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Reviewed & tested by the community

I think this latest patch is good to go. Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: core-multi-site-install-2168617-18.patch, failed testing.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented
Status: Needs work » Reviewed & tested by the community

The failure is unrelated to this issue so back to RTBC

The test did not complete due to a fatal error.
Completion check
FilterAdminTest.php 297
Drupal\filter\Tests\FilterAdminTest->testUrlFilterAdmin()

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Reviewed & tested by the community » Fixed

Thanks again! Committed to 8.x.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.