Config sync should not throw a warning when not being writable

+1 — it's not only on hosting providers; the default configuration I have for my own production sites doesn't allow writes to the config sync directory, and especially with the redesigned status report page, it always irks me to know I have to scroll down once I see the huge "1 WARNING" label at the top, and then I realize that yes, it's still just the config sync directory.

Agree with #3

May I be so bold... tested on the site in question where I was irked for the last time. The patch applies and makes it so the site has no more warnings.

So what happens when you try to upload a tarball to a site where the folder is not writeable?

I.e. I think we should prevent upload on admin/config/development/configuration/full/import with a message when the folder is not writeable.

Usability is preferred over UX, D7UX, D8UX etc. See https://www.drupal.org/issue-tags/topic

Any news why this didn't make it into Drupal 8.4.0?

@jonasdk because #6 was not answered and #7

The attached patch fails against 8.4.x branch. Attaching a new one.

@gargsuchi did you see #7. The suggestion is to remove the warning from the system report altogether and just do error handling on the config tarball upload screen.

I.e. I think we should prevent upload on admin/config/development/configuration/full/import with a message when the folder is not writeable.

@alexpott, are you thinking that an error would display after attempting to upload the tarball? Or that the form field would be disabled and the message would be visible on the form prior to submission?

After further investigation, it appears that the form is already disabled along with an error message if the directory isn't writable:

  /**
   * {@inheritdoc}
   */
  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');
    }
    $form['import_tarball'] = [
      '#type' => 'file',
      '#title' => $this->t('Configuration archive'),
      '#description' => $this->t('Allowed types: @extensions.', ['@extensions' => 'tar.gz tgz tar.bz2']),
    ];

    $form['submit'] = [
      '#type' => 'submit',
      '#value' => $this->t('Upload'),
      '#disabled' => !$directory_is_writable,
    ];
    return $form;
  }

So it seems like the patch in #12 is really all that is remaining—changing the requirement from warning to info. Thoughts @alexpott?

@gargsuchi, looks like something from #944582: Check for execute permissions on directories that require file write permissions might have snuck in your patch—file_directory_is_writable() doesn't appear to be in 8.5.x yet. Re-roll attached.

Thats correct @q0rban.
Thanks for this patch.

Ah nice that we already check. Therefore I vote that we just remove this from the status report altogether. It's not that useful and the only time in the UI you need it you are going to be informed right then and there.

Does one remove config_requirements() (and config.install altogether), or keep it in case anyone is calling it directly and return []? This patch assumes the former.

@pjcdawkins your patch is correct. We've had some fun with removing class files before but this is not a class file so removing is definitely correct. Yay one less file!

Removing the test that was checking for this message

The test succeeded.

Less code. Nice.

Committed as 8fb5919 and pushed to 8.5.x.
Cherry picked as 7c6bf57 and pushed to 8.4.x.

Thanks, this one has annoyed me in the past too.

I see that this has already been committed, however there is a use case where you may want to write to the config directory on production. Modules like Webform have the webform itself as configuration. This many times needs to be changed by the website content maintainers.

Using the Config Split module this configuration can be excluded so that it does not get overwritten during deployments. At the beginning of the deployment the excluded split is exported so that after the code is updated and the new configuration is imported the excluded configuration is imported last so that changes made on the site are not lost.

However deployment scripts should be configured in such a way that they ensure that this folder is writable.

@frederickjh agreed that's a good use case, but there's still no need for a core warning for everyone.

And you could still have a read-only directory on production, and sync the config/DB to an external environment, then merge the config however you like, test, then deploy to production. Production doesn't need to have writable files (except for tmp, cache, and uploads).