Problem/Motivation

When a user has a settings.php with database settings and config directories configured but not writable the installer was able to continue before #2466197: Staging directory should not have to be writeable. and create the config directories. Now that does not occur - you get the reported installation error.

This regression appears to have broken Acquia's cloud hosting.

Proposed resolution

When settings.php contains config directory info try to create the directories. If they are not there error - if they are don't worry about if they are not writable.

Steps to test

  1. The starting permissions need to be something like this:
    $ ll sites/default
    total 224
    drwxr-xr-x  4 alex  staff    306 14 Aug 13:20 .
    drwxr-xr-x  6 alex  staff    374 14 Aug 13:20 ..
    -rw-r--r--  1 alex  staff   6762 12 Aug 14:33 default.services.yml
    -rw-r--r--  1 alex  staff  30223 12 Aug 14:35 default.settings.php
    drwxrwxrwx  3 _www  staff    136 14 Aug 13:20 files
    -rw-r--r--  1 alex  staff   6762 14 Aug 13:20 services.yml
    -rw-r--r--  1 alex  staff  31825 14 Aug 13:20 settings.php
    
    • Ensure that settings.php has 644 permissions
    • Ensure sites/default/files is writable by the webserver and empty
    • Ensure the sites/default has 755 permissions and is not writable by the webserver
    • Ensure that settings.php is not owned by the webserver
  2. settings.php has to have hash_salt, database and config directory configuration already
  3. The config directory location should point to somewhere web writable - somewhere in sites/default/files is the default
  4. go to yoursite/core/install.php and install drupal
  5. Afterwards the config directory should be created

Remaining tasks

User interface changes

API changes

Data model changes

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Title: Staging directory should be created by installer if possible » [regression] Staging directory should be created by installer if possible
Issue summary: View changes
Status: Active » Needs review
FileSize
1.39 KB
2.23 KB
alexpott’s picture

Note we have test coverage that a read-only config sync directory is possible - see \Drupal\system\Tests\Installer\InstallerExistingConfigDirectoryTest

The last submitted patch, 2: 2783749-2.test-only.patch, failed testing.

bircher’s picture

So the patch in #2 works, but you are told that the settings.php is not writeable on the "verify requirements" install step. You can then click on "try again" and without changing anything in the files the installation succeeds.

(This of course given a non-writable settings.php containing the database credentials and the path to a non-existing config/sync folder that can be created later.)

The patch attached here makes so that the check that settings.php is writeable can be skipped even if the config/sync folder does not exist. (the "try again" works because this issue is about trying to create that folder if it doesn't exist and the check that leads to settings.php having to be writeable happens before.

Status: Needs review » Needs work

The last submitted patch, 5: 2783749-5.patch, failed testing.

alexpott’s picture

I tested #2 and it works fine. This one is quite tricky to manually test. @bircher what did you do exactly think the change in #5 is necessary? If what you are saying is true that then test this patch adds would fail.

alexpott’s picture

Berdir’s picture

We don't have a staging directory anymore, shouldn't this say Sync directory? :) The other issue also had that wrong.

alexpott’s picture

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

Added test to test to the issue summary and re-uploading #2 afrer manually testing again and proving to myself it works.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Title: [regression] Staging directory should be created by installer if possible » [regression] Config directories should be created by installer if present in setings.php and if possible
alexpott’s picture

Better and more tests and better help text too.

alexpott’s picture

Status: Needs review » Needs work

@bircher mea culpa... you are right! And the evidence is in the steps to reproduce in the issue summary :)

-rw-r--r-- 1 _www staff 6762 14 Aug 13:20 services.yml
-rw-r--r-- 1 _www staff 31825 14 Aug 13:20 settings.php

So if my settings.php is not writeable by the webserver I see what you are seeing - the need to press try again and then it magically works. This bit of this feels like #2782811: [regression] 8.1.8 newly tries to write to settings.php during install because $settings['install_profile'] gets unset

And is also impossible for the automated test to test I think since even 444 doesn't trigger it - it occurs when it is a different user.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Status: Needs work » Needs review
FileSize
949 bytes
6.69 KB

This patch makes a similar fix to #5 but also checks that the required hash_salt is set. Obviously our test infra was relying on the settings_verified being false due to config directories not existing however the hash salt definitely is required and if not set we're going to need to write it.

bircher’s picture

Status: Needs review » Reviewed & tested by the community

Right! the hash salt needs to be there too of course!

On that note, it may be worth adding a $install_state['parameters']['profile'] == Settings::get('install_profile', FALSE) check to either the the place where $install_state['settings_verified'] is set or where it is used to skip the check for write access to settings.php.
But that could also be a follow-up because if you try to install a profile and have a different one set in a read-only settings.php you get an error. There are different ways we can treat this case and to fix the regression the patch in #16 works. So RTBC..

alexpott’s picture

@bircher going to create a followup to deal with the case where the install profile is set in settings.php. I tested 8.1.7 and the current behaviour is not a regression - it behaves exactly the same and makes you choose the profile.

  • catch committed 9e2f220 on 8.3.x
    Issue #2783749 by alexpott, bircher: [regression] Config directories...

  • catch committed c187f1d on 8.2.x
    Issue #2783749 by alexpott, bircher: [regression] Config directories...

  • catch committed 540a5f4 on 8.1.x
    Issue #2783749 by alexpott, bircher: [regression] Config directories...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to all three 8.x branches.

If this bug hadn't been in a patch release I'd have just reverted from 8.1.x, but don't see another option here.

xjm’s picture

Issue tags: +8.1.8 release notes

Status: Fixed » Closed (fixed)

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