Staging directory should not have to be writeable.

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.

+1

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.

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.

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.

Here's tests.

Updated IS to reflect solution in #11.

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

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?

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.

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

+  // 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?

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

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

Ahh, that's smart!

@tstoeckler I've created #2696103: \Drupal\Core\Config\FileStorageFactory::getSync() should throw a more specific exception to address this and added a comment.

+++ 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.

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." ?

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.

Giving @pjcdawkins credit on this issue because of the work on #2607352: The installer, updater and status report needlessly require $config_directories to be writable

+++ 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.

@Crell see #23/#2696103: \Drupal\Core\Config\FileStorageFactory::getSync() should throw a more specific exception - I'd agree if the exception caught was more specific.

+++ 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 :)

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)

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?

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?

Let's see if this fixes the problem on platform.

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

Created #2723411: Test the .htaccess added config sync directory to make this even simpler.

@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.

Wow we really want the sync directory to be writable!

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

Linking #2156401: Write install_profile value to configuration and only to settings.php if it is writeable

Looks like this should still be Needs review for #49

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

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.

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

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.

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. :-(

+++ 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.

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.

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.

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.)

@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.

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?

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".

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

RTBC per #61 and #66

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.

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

Committed e5a9806 and pushed to 8.1.x. Thanks!

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?

Spun-off #2782811: [regression] 8.1.8 newly tries to write to settings.php during install because $settings['install_profile'] gets unset.

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. :\

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.

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.

Created #2783749: [regression] Config directories should be created by installer if present in setings.php and if possible

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?

@mikey_p - new issue.

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"