Problem/Motivation

One common way of using config will be:

  • Site Builder A: Export files to a directory
  • Server: Import the exported files from that directory

As we see, the webserver never needs the staging directory to the writeable, but Drupal complains about it,
see core/modules/system/system.install:409

Proposed resolution

  • System module does not require the staging directory to be writeable
  • Config module has a warning if the staging directory is not writeable
  • System module requires that the $config_directories has a 'sync' key because config.storage.staging relies on it
  • System module requires that all directories listed in $config_directories exist

Note this resolution does not do

Require the active directory writeable, in case the site uses file storage

because that should be the responsibility of who ever swaps out the storage (imo).

Remaining tasks

User interface changes

New UI text

API changes

None

Files: 

Comments

dawehner’s picture

Issue summary: View changes
xjm’s picture

Issue tags: +Configuration system

Agreed, there is no longer any functionality in the UI that needs to write to the staging directory, so core does not need to require it to be writeable. A contributed module supporting a UI-to-staging workflow could add this requirement if desired.

alexpott’s picture

+1

alexpott’s picture

Looking at this issue again. I think we should remove all automatic creation of the directories listed in the $config_directories global and all testing of them in system_requirements.

Also as it is a very debatable security practice store active config in sites/default/files it is questionable whether we should create a $config_directories['sync'] setting during install at all.

alexpott’s picture

I guess the question is what can we get away with in Drupal 8. And thinking about this some more - actually we do write to the sync directory - we write there if we import a tarball.

alexpott’s picture

So if this issue is to proceed is going to need to make the ConfigSync UI detect whether or not the sync directory is writable before trying to upload the tarball.

I think the generic config directory checking beyond existence of directory should be removed from system_requirements. The config module should add a check that the sync directory is writeable - since it provides the UI that requires it. And the import tarball form needs to be tested to ensure it handles this type of error properly.

alexpott’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
1.96 KB
alexpott’s picture

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

Status: Needs review » Needs work

The last submitted patch, 7: 2466197-7.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.29 KB
2.58 KB
alexpott’s picture

Here's tests.

alexpott’s picture

Issue summary: View changes

Updated IS to reflect solution in #11.

Status: Needs review » Needs work

The last submitted patch, 11: 2466197-11.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
792 bytes
7.14 KB
dawehner’s picture

  1. +++ b/core/includes/file.inc
    @@ -332,7 +332,17 @@ function file_ensure_htaccess() {
    +  catch (\Exception $e) {
    

    It is weird, that we don't have a more specific exception, but well, this is how it is.

  2. +++ b/core/modules/config/config.install
    @@ -0,0 +1,32 @@
    +  // because only configuration import from a tarball requires the folder to be
    

    Do we check before the import from a tarball whether the folder is writeable already?

  3. +++ b/core/modules/config/config.install
    @@ -0,0 +1,32 @@
    +      'description' => t('The directory %directory is not writable.', ['%directory' => $directory]),
    

    Should we explain why this directory has to be writeable?

alexpott’s picture

Thanks for the review @dawehner.

  1. We could create a new more specific exception in a followup as that should break nothing
  2. Nope - added check and test for it
  3. I think paired with "Configuration directory: sync" this message is fine - also means there are no extra strings for translators to translate
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

We could create a new more specific exception in a followup as that should break nothing

Yeah sure.

Cool, thank you alex

catch’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/modules/config/config.install
    @@ -0,0 +1,32 @@
    +  if ($phase !== 'install' && !is_writable($directory)) {
    +    $requirements['config directory ' . CONFIG_SYNC_DIRECTORY] = [
    +      'title' => t('Configuration directory: %type', ['%type' => CONFIG_SYNC_DIRECTORY]),
    +      'description' => t('The directory %directory is not writable.', ['%directory' => $directory]),
    +      'severity' => REQUIREMENT_WARNING,
    +    ];
    +  }
    

    The issue summary talks about not warning about this - because you can run a site fine without it. But this is still warning about it - should it just be a notice?

  2. +++ b/core/modules/config/src/Form/ConfigImportForm.php
    @@ -72,6 +72,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +      $form_state->setErrorByName('import_tarball', $this->t('The directory %directory is not writable.', ['%directory' => $directory]));
    

    What about doing this check in the form builder itself too, and warning there?

alexpott’s picture

Issue summary: View changes

re #18.1 This warning only occurs if you have the config module installed which I think is the desired behaviour as this is the only place that can write to this directory - updated IS to just describe the current resolution.

alexpott’s picture

Patch addresses #18.2 - we now check and display a message is the folder is not writable before uploading. Seems polite.

tstoeckler’s picture

+  // If a staging directory exists then it should contain a .htaccess file.
+  try {
+    $staging = config_get_config_directory(CONFIG_SYNC_DIRECTORY);
+  }
+  catch (\Exception $e) {
+    $staging = FALSE;
+  }
+  if ($staging) {
+    file_save_htaccess($staging, TRUE);
+  }

Couldn't this just be:

try {
  $staging = config_get_config_directory(CONFIG_SYNC_DIRECTORY);
  file_save_htaccess($staging, TRUE);
}
catch (\Exception $e) {
  // Nothing to do here (but probably with a better comment).
}

?

Edit: Or do we want to support people actively setting the directory to FALSE?

Status: Needs review » Needs work

The last submitted patch, 20: 2466197-20.patch, failed testing.

alexpott’s picture

@tstoeckler because \Exception is such a general exception I didn't want to falsely catch anything coming form that.

alexpott’s picture

Status: Needs work » Needs review

Drupal\system\Tests\Routing\RouterTest::testControllerResolutionPage failed... nothing to do with this.

tstoeckler’s picture

Ahh, that's smart!

alexpott’s picture

@tstoeckler I've created #2696103: config_get_config_directory() should throw a more specific exception to address this and added a comment.

alexpott’s picture

+++ b/core/includes/file.inc
@@ -332,7 +332,19 @@ function file_ensure_htaccess() {
+  if ($staging) {
+    file_save_htaccess($staging, TRUE);
+  }

So if the directory is not writable what should we do here? At the moment this is going to break - I think we should eat this error. `if staging is not writable you're not doing a standard install of drupal with config in the files directory - therefore it is up to you to secure it. You might not even be using apache so this could be pointless.

pjcdawkins’s picture

Status: Needs review » Needs work
  1. +++ b/core/includes/file.inc
    @@ -332,7 +332,19 @@ function file_ensure_htaccess() {
    +  if ($staging) {
    +    file_save_htaccess($staging, TRUE);
    +  }
    
    • I think this could be if ($staging && is_dir($staging) && is_writable($staging)) {
    • The second argument to file_save_htaccess() defaults to TRUE anyway, so that could be removed.
  2. +++ b/core/modules/config/src/Tests/ConfigImportUploadTest.php
    @@ -50,6 +50,16 @@ function testImport() {
    +    // Ensure sumbit button for \Drupal\config\Form\ConfigImportForm is
    

    Little typo here

  3. +++ b/core/modules/system/src/Tests/System/StatusTest.php
    @@ -65,6 +75,9 @@ public function testStatusPage() {
    +    // blah
    

    This comment could be a bit more descriptive!

  4. +++ b/core/modules/system/system.install
    @@ -548,15 +548,22 @@ function system_requirements($phase) {
    +      'description' => t('Your %file file must define the $config_directories variable as an array containing the name of a directories in which configuration files can be written. It must contain a %sync_key key.', array('%file' => $site_path . '/settings.php', '%sync_key' => CONFIG_SYNC_DIRECTORY)),
    

    Can we change "the name of a directories in which configuration files can be written." to "the names of directories in which configuration files can be found." ?

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.18 KB
10.04 KB

Thanks for the review @pjcdawkins. Fixed 2-4.

Point 1 is super tricky. I think we should err on the side of caution and if the staging directory is not writable and does not have an .htaccess file just let it log the security warning. Because if the directory is web accessible then this security warning is a good idea.

I was wrong in #27 - nothing is going to break if the directory does not exist or is not writable we're just going to log a warning.

alexpott’s picture

Crell’s picture

+++ b/core/includes/file.inc
@@ -332,7 +332,19 @@ function file_ensure_htaccess() {
+  try {
+    $staging = config_get_config_directory(CONFIG_SYNC_DIRECTORY);
+  }
+  catch (\Exception $e) {
+    $staging = FALSE;
+  }
+  if ($staging) {
+    file_save_htaccess($staging, TRUE);
+  }

The flag is unnecessary. Put the file_save_htaccess() in the try block, and then an empty catch (with comment saying why it's empty). Same result, but cleaner and shorter.

alexpott’s picture

@Crell see #23/#2696103: config_get_config_directory() should throw a more specific exception - I'd agree if the exception caught was more specific.

alexpott’s picture

+++ b/core/includes/file.inc
@@ -332,7 +332,19 @@ function file_ensure_htaccess() {
+  // If a staging directory exists then it should contain a .htaccess file.
+  // @todo https://www.drupal.org/node/2696103 catch a more specific exception
+  //   and simplify this code.

And there's a @todo about this already :)

heddn’s picture

Status: Needs review » Needs work

The tests must not be testing every scenario. This currently is causing my platformsh build to fail because the filesystem is mounted as read-only and I'm getting following error. I'm running 8.1.1.

Array to string conversion install.core.inc:2156                        [notice]
Drupal\Core\Installer\Exception\InstallerException: File system:         [error]
Writable (<em>public</em> download method)

Array

Settings file: The <em class="placeholder">Settings file</em> is not
writable.

The Drupal installer requires write permissions to <em
class="placeholder">./sites/default/settings.php</em> during the
installation process. The <a
href="https://www.drupal.org/server-permissions">webhosting
issues</a> documentation section offers help on this and other
topics. in /app/web/core/includes/install.core.inc:2160
Stack trace:
#0 /app/web/core/includes/install.core.inc(1018):
install_display_requirements(Array, Array)
#1 /app/web/core/includes/install.core.inc(652):
install_verify_requirements(Array)
#2 /app/web/core/includes/install.core.inc(530):
install_run_task(Array, Array)
#3 /app/web/core/includes/install.core.inc(115):
install_run_tasks(Array)
rachel_norfolk’s picture

Oh? With reference to #34 I have the following

...
    "patches": {
            "drupal/core": {
                "Allow read-only config directories": "https://www.drupal.org/files/issues/2466197-29.patch"
            }
        }
    }
…

In my composer.json and platform.sh seems happy?

heddn’s picture

Re-roll is needed. Next up, is a patch that should get things working again.

re #35, it could be that the difference here is that I'm using config_installer and/or 8.1.1?

heddn’s picture

FileSize
780 bytes
780 bytes
heddn’s picture

heddn’s picture

heddn’s picture

Dropped a test since it doesn't apply cleanly on 8.1.1.

The last submitted patch, 41: staging_directory-2466197-41.patch, failed testing.

heddn’s picture

Status: Needs review » Needs work

The last submitted patch, 43: staging_directory-2466197-42-4.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
FileSize
9.67 KB
alexpott’s picture

alexpott’s picture

@heddn I'm not sure what you been trying to do in the patches from #37 onwards however I'm aware that there still is a problem during installation so what I'm going to do is work from #36 and rebase this on top of #2723411: Test the .htaccess added config sync directory and then add test coverage of having a read-only sync directory during install. I believe doing this will resolve the issues on platform.sh and will the config_installer.

alexpott’s picture

Wow we really want the sync directory to be writable!

alexpott’s picture

So we can actually deprecate install_ensure_config_directory() because its only usage is the deprecated version of KernelTestBase.

Status: Needs review » Needs work

The last submitted patch, 49: 2466197-49.patch, failed testing.

heddn’s picture

pjcdawkins’s picture

Status: Needs work » Needs review

Looks like this should still be Needs review for #49

alexpott’s picture

@pjcdawkins #2723411: Test the .htaccess added config sync directory needs review before this one.

alexpott’s picture

Version: 8.1.x-dev » 8.2.x-dev
Issue tags: +String change in 8.2.0
FileSize
1.61 KB
16.43 KB

I've backtracked on #2723411: Test the .htaccess added config sync directory because @neclimdul had valid security concerns and turned it into a test only patch. Here's an updated patch that removes the requirement to be writable but leaves the htaccess thing alone and documents it. I think it is acceptable to write an error to the log if a .htaccess is missing since this is the safest thing to do. Even if the sync directory is not served by apache or not available to the server it might be moved in the future and future-yous could assume it is safe when it's not because the usual htaccess is missing.

As much as I'd love to see this 8.1.x there are string changes so I think this should only be for 8.2.x.

Status: Needs review » Needs work

The last submitted patch, 55: 2466197-55.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
dawehner’s picture

  1. +++ b/core/includes/install.inc
    @@ -489,8 +489,9 @@ function drupal_install_config_directories() {
    +    $config_directories[CONFIG_SYNC_DIRECTORY] = \Drupal::service('site.path') . '/files/config_' . $config_directories_hash . '/sync';
         $settings['config_directories'][CONFIG_SYNC_DIRECTORY] = (object) [
    -      'value' => \Drupal::service('site.path') . '/files/config_' . $config_directories_hash . '/sync',
    +      'value' => $config_directories[CONFIG_SYNC_DIRECTORY],
           'required' => TRUE,
         ];
    

    Should we stop setting up the the setting, if we could not write in the first place?

  2. +++ b/core/includes/install.inc
    @@ -506,19 +507,21 @@ function drupal_install_config_directories() {
       // Bail out using a similar error message as in system_requirements().
    -  if (!install_ensure_config_directory(CONFIG_SYNC_DIRECTORY)) {
    -    throw new Exception(t('The directory %directory could not be created or could not be made writable. To proceed with the installation, either create the directory and modify its permissions manually or ensure that the installer has the permissions to create it automatically. For more information, see the <a href=":handbook_url">online handbook</a>.', array(
    +  if (!file_prepare_directory($config_directories[CONFIG_SYNC_DIRECTORY], FILE_CREATE_DIRECTORY)
    +    && !file_exists($config_directories[CONFIG_SYNC_DIRECTORY])) {
    +    throw new Exception(t('The directory %directory could not be created. To proceed with the installation, either create the directory or ensure that the installer has the permissions to create it automatically. For more information, see the <a href=":handbook_url">online handbook</a>.', array(
           '%directory' => config_get_config_directory(CONFIG_SYNC_DIRECTORY),
    
    @@ -529,6 +532,9 @@ function drupal_install_config_directories() {
    + * @deprecated in Drupal 8.1.x, will be removed before Drupal 9.0.x. Use
    + *   config_get_config_directory() and file_prepare_directory() instead.
      */
     function install_ensure_config_directory($type) {
    

    Its weird to replace one of the few actual usages of install_ensure_config_directory and replace it with something else. Can't we have an api function which does what you need?

  3. +++ b/core/includes/install.inc
    index 0000000..c971ae6
    --- /dev/null
    
    --- /dev/null
    +++ b/core/modules/config/config.install
    
    +++ b/core/modules/config/config.install
    +++ b/core/modules/config/config.install
    @@ -0,0 +1,32 @@
    
    @@ -0,0 +1,32 @@
    +<?php
    +
    +/**
    + * @file
    + * Install, update and uninstall functions for the config module.
    + */
    +
    

    I'm wondering whether conceptually this totally belongs into config module, given that config is really just config UI. You can sync with just drush easily.

  4. +++ b/core/modules/config/src/Form/ConfigImportForm.php
    @@ -50,6 +50,11 @@ public function getFormId() {
       public function buildForm(array $form, FormStateInterface $form_state) {
    +    $directory = config_get_config_directory(CONFIG_SYNC_DIRECTORY);
    +    $directory_is_writable = is_writable($directory);
    +    if (!$directory_is_writable) {
    +      drupal_set_message($this->t('The directory %directory is not writable.', ['%directory' => $directory]), 'error');
    +    }
    

    Mh, its not entirely obvious why we cannot import a from a tar when the folder is not writeable ... one could extract the file in memory and do everything in memory, or store things temporarily somewhere else. I think this is more of a temporary workaround?

alexpott’s picture

Thanks for the review @dawehner

  1. The settings is only written if we've set the setting to \Drupal::service('site.path') . '/files/config_' . $config_directories_hash . '/sync', - this has to be writable. Slightly altered the code flow to make this more obvious.
  2. I'm not sure an API is that useful for stuff done only during the installer.
  3. I don't think so - drush for example it is going to depend on which user is running drush and which user can write the files. This is Drush's problem and not cores. The only module that needs staging to be writable is Config UI module - thats the whole point of the issue.
  4. We need to put the staged config somewhere so the user can see differences etc before important that's what this directory is for and hence the config.install changes
alexpott’s picture

More on the config_requirements() - here we are testing that the webserver has write access. And as said before, the only place in core this is required is the Config UI module.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

More on the config_requirements() - here we are testing that the webserver has write access. And as said before, the only place in core this is required is the Config UI module.

Gotcha!

Crell’s picture

This does apply to 8.1 and works currently. It's also required for D8 running properly on Platform.sh. Is there any way we could manage an 8.1 backport? (I'm happy to do the reroll for it if so.) We'd really hate to require a patch file for the next 5 months. :-(

alexpott’s picture

+++ b/core/includes/install.inc
@@ -506,19 +502,21 @@ function drupal_install_config_directories() {
-    throw new Exception(t('The directory %directory could not be created or could not be made writable. To proceed with the installation, either create the directory and modify its permissions manually or ensure that the installer has the permissions to create it automatically. For more information, see the <a href=":handbook_url">online handbook</a>.', array(
...
+    throw new Exception(t('The directory %directory could not be created. To proceed with the installation, either create the directory or ensure that the installer has the permissions to create it automatically. For more information, see the <a href=":handbook_url">online handbook</a>.', array(

+++ b/core/modules/config/config.install
@@ -0,0 +1,32 @@
+      'title' => t('Configuration directory: %type', ['%type' => CONFIG_SYNC_DIRECTORY]),
+      'description' => t('The directory %directory is not writable.', ['%directory' => $directory]),

+++ b/core/modules/config/src/Form/ConfigImportForm.php
@@ -50,6 +50,11 @@ public function getFormId() {
+      drupal_set_message($this->t('The directory %directory is not writable.', ['%directory' => $directory]), 'error');

+++ b/core/modules/system/system.install
@@ -549,15 +549,22 @@ function system_requirements($phase) {
+          'description' => t('The directory %directory does not exist.', array('%directory' => $directory)),
...
-      'description' => t('Your %file file must define the $config_directories variable as an array containing the name of a directories in which configuration files can be written.', array('%file' => $site_path . '/settings.php')),
+      'description' => t('Your %file file must define the $config_directories variable as an array containing the names of directories in which configuration files can be found. It must contain a %sync_key key.', array('%file' => $site_path . '/settings.php', '%sync_key' => CONFIG_SYNC_DIRECTORY)),

So there's no API change here really. The changes that block this going in to 8.1.x are all translatable string changes or additions. In mitigation all of these strings are completely admin facing and not just regular admin - they are sitebuilder/siteowner facing whilst doing config operations or viewing the site report. Also existing sites should never see these messages because they all have had to had writable config directories.

I can't make the call on whether to put this in 8.1.x - that needs to be a release manager - either @catch or @xjm.

tstoeckler’s picture

Obviously I can't make a decision either way here, but:

Also existing sites should never see these messages because they all have had to had writable config directories.

This is an important point, I didn't realize that so far. Thanks for pointing that out, just wanted to basically restate that.

  • catch committed 83fce77 on 8.2.x
    Issue #2466197 by alexpott, heddn, dawehner, pjcdawkins: Staging...
catch’s picture

Version: 8.2.x-dev » 8.1.x-dev
Status: Reviewed & tested by the community » Needs review

Committed 83fce77 and pushed to 8.2.x. Thanks!

Had a look at the strings for 8.1.x, I agree it's not too bad, notes below:

  1. +++ b/core/includes/install.inc
    @@ -506,19 +502,21 @@ function drupal_install_config_directories() {
    -  if (!install_ensure_config_directory(CONFIG_SYNC_DIRECTORY)) {
    -    throw new Exception(t('The directory %directory could not be created or could not be made writable. To proceed with the installation, either create the directory and modify its permissions manually or ensure that the installer has the permissions to create it automatically. For more information, see the <a href=":handbook_url">online handbook</a>.', array(
    +  if (!file_prepare_directory($config_directories[CONFIG_SYNC_DIRECTORY], FILE_CREATE_DIRECTORY)
    +    && !file_exists($config_directories[CONFIG_SYNC_DIRECTORY])) {
    +    throw new Exception(t('The directory %directory could not be created. To proceed with the installation, either create the directory or ensure that the installer has the permissions to create it automatically. For more information, see the <a href=":handbook_url">online handbook</a>.', array(
           '%directory' => config_get_config_directory(CONFIG_SYNC_DIRECTORY),
    

    Would it kill us if this error message didn't change in 8.1.x? This seems the most likely message of all of them that new installers will hit, the current information will get you past the error message, and it's only telling you that you might need something that's no longer a requirement.

  2. +++ b/core/modules/config/config.install
    @@ -0,0 +1,32 @@
    +      'title' => t('Configuration directory: %type', ['%type' => CONFIG_SYNC_DIRECTORY]),
    

    This is actually a new string, a quick grep didn't find a good 8.1.x equivalent, it's also short though. But could maybe use the error message as the heading at a stretch to have zero string changes.

  3. +++ b/core/modules/config/src/Form/ConfigImportForm.php
    @@ -50,6 +50,11 @@ public function getFormId() {
    +      drupal_set_message($this->t('The directory %directory is not writable.', ['%directory' => $directory]), 'error');
    

    core/modules/system/system.install: $error = t('The directory %directory is not writable.', array('%directory' => $directory));

    This is in 8.1.x already so not actually a new string.

  4. +++ b/core/modules/system/system.install
    @@ -549,15 +549,22 @@ function system_requirements($phase) {
    +          'description' => t('The directory %directory does not exist.', array('%directory' => $directory)),
    

    ules/system/system.install: $error = t('The directory %directory does not exist.', array('%directory' => $directory));

    Also not a new string.

Crell’s picture

Here's a reroll of #59, applied against 8.1.x, with the two string changes requested in #66. Interdiff is just the string changes, against the reroll.

Something else we noticed in testing is that while this patch lets the site install cleanly (yay!), there's still a warning on the site status page that the sync directory is not writeable. That's a problem on a development instance, but in production is not necessarily a problem and in Platform.sh's case will always be the case, unconditionally, and it's not a problem by design. Should we (in a follow-up, probably) remove or somehow demote that message? (I think it would be fine to do that in 8.2 only, since it doesn't actually prevent functionality it just is annoying and misleading.)

alexpott’s picture

@Crell I think it is perfectly reasonable for the Config module to add this warning to the reports page. This module requires it to be writable in order to work. It only provides a UI for importing and exporting config. If it can't write there it can't do one of it's main tasks.

+++ b/core/modules/config/config.install
@@ -0,0 +1,31 @@
+  // Ensure the configuration sync directory is writable. This is only a warning
+  // because only configuration import from a tarball requires the folder to be
+  // web writable.
+  if ($phase !== 'install' && !is_writable($directory)) {
+    $requirements['config directory ' . CONFIG_SYNC_DIRECTORY] = [
+      'title' => t('The directory %directory is not writable.', ['%directory' => $directory]),
+      'severity' => REQUIREMENT_WARNING,
+    ];
+  }

And it is only a warning - not an error.

Crell’s picture

Mm, fair. It's just that on Platform that message will then always and forever be there, because the whole disk is read-only. (We can't make the sync dir both writeable and git-pushable at the same time, as that defeats the whole purpose.)

How is the reroll otherwise?

rachel_norfolk’s picture

And, even outside of platform.sh, surely "good practice" on a production system should be the only writeable files are things that users need to upload etc, certainly not the intended configuration of the website.
We shouldn't be giving a warning message when people are doings things "right".

Crell’s picture

Someone want to RTBC #67? The discussion after that is essentially off topic, sorry for bringing it up. :-)

jonathanshaw’s picture

Status: Needs review » Reviewed & tested by the community

RTBC per #61 and #66

alexpott’s picture

Mm, fair. It's just that on Platform that message will then always and forever be there, because the whole disk is read-only. (We can't make the sync dir both writeable and git-pushable at the same time, as that defeats the whole purpose.)

Only if they have the config module enabled - which tbh doesn't make a lot of sense if you are using git to do deployments since you probably want to automate config synchronisation with you release process.

xjm’s picture

Assigned: Unassigned » catch

Assigning to catch since he committed the patch to 8.2.x and the 8.1.x version is intended to address his feedback.

catch’s picture

Assigned: catch » Unassigned
Status: Reviewed & tested by the community » Fixed

Committed e5a9806 and pushed to 8.1.x. Thanks!

  • catch committed 8e451a1 on 8.1.x
    Issue #2466197 by alexpott, heddn, Crell, dawehner, pjcdawkins, catch:...

  • catch committed 83fce77 on 8.3.x
    Issue #2466197 by alexpott, heddn, dawehner, pjcdawkins: Staging...

  • catch committed 83fce77 on 8.3.x
    Issue #2466197 by alexpott, heddn, dawehner, pjcdawkins: Staging...
webchick’s picture

When I install 8.1.8, I'm now getting:

Configuration directory: sync
The directory /mnt/www/html/vanillastandard6adrhwm5vn/docroot/sites/default/files/config_a5deca9594f05e7a59a30d108c489de23e0dc665/sync does not exist.

and:

Settings file
The Drupal installer requires write permissions to ./sites/default/settings.php during the installation process. The webhosting issues documentation section offers help on this and other topics.

This wasn't an issue on 8.1.7. Any ideas on how to workaround?

webchick’s picture

Preliminary testing shows that if we adjust our site set-up scripts to pre-create the config sync directory, we can get around this issue, at least on one of our hosting products. So it's looking like it was indeed this patch that caused the breakage compared to 8.1.7. :\

gelierb’s picture

I've been dealing with this for a day or two. Tried the "upgrade" forum for help. Thought it must be something I was doing. Upgrades have seemed so solid up to now. Why is 8.1.8 still available for download if it's causing these problems? Sorry if this is not the correct place to comment.

alexpott’s picture

I've confirmed the first part of #79. When a user has a settings.php with database settings and config directories configured but not writable the installer was able to continue before this patch and create the config directories. Now that does not occur - you get the reported installation error. This is quite different from the reported error in #80 so I'm going to file a separate issue for it.

mikey_p’s picture

Unfortunately there is still a drawback to this approach, and that is while the hook_requirements() requirement has been removed file_ensure_htaccess() still expects it to be writable:

+++ b/core/includes/file.inc
@@ -315,7 +315,22 @@ function file_ensure_htaccess() {
+
+  // If a staging directory exists then it should contain a .htaccess file.
+  // @todo https://www.drupal.org/node/2696103 catch a more specific exception
+  //   and simplify this code.
+  try {
+    $staging = config_get_config_directory(CONFIG_SYNC_DIRECTORY);
+  }
+  catch (\Exception $e) {
+    $staging = FALSE;
+  }
+  if ($staging) {
+    // Note that we log an error here if we can't write the .htaccess file. This
+    // can occur if the staging directory is read-only. If it is then it is the
+    // user's responsibility to create the .htaccess file.
+    file_save_htaccess($staging, TRUE);
+  }

This doesn't check to see if the directory is writable and file_save_htaccess logs an error if it can't write the .htaccess. This is filling up our logs as this happens for ALOT of file operations, sometimes we see dozens of "Security warning: Couldn't write .htaccess file" errors logged per request.

I would suggest either adding a check here to make sure that the config staging directory is writable first, or adding a setting that can be added to settings.php to skip this check. In an unrelated issue, I would suggest that file_ensure_htaccess() include a static check to be sure that it is only run once per request, unless there is a reason to check this on every file operation.

Should I reopen this issue for a followup, or should I create a new issue?

alexpott’s picture

@mikey_p - new issue.

Status: Fixed » Closed (fixed)

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

Dane Powell’s picture

If you are interested in suppressing the "not writable" warning on the status report, and assuming you can't simply disable the config module, I think this might be the best solution: #2877128: [META] Evaluate status report warnings and determine which should be reduced to info, removed, etc.

Also it was surprising to me to learn that the config module is strictly an administrative interface; I assumed it couldn't be disabled if you wanted to do any sort of configuration management. I opened another issue to address this: #2877111: Rename Config module to "Config UI"