Problem/Motivation

in #2466197: Staging directory should not have to be writeable. it was decided that setting the warning on non-writable config directories was just fine. See https://www.drupal.org/node/2466197#comment-11271845 for more on the discussion.

However the config not being writable really shouldn't be a warning level notice, because a) its not actionable on production platforms and b) only configuration import from a tarball requires the folder to be web writable.

Proposed resolution

Remaining tasks

Review the patch.

User interface changes

Non-writable config directories will be reported under the information section instead of a site-wide warning.

API changes

None

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

japerry created an issue. See original summary.

japerry’s picture

Patch inc..

geerlingguy’s picture

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

Dinesh18’s picture

Agree with #3

geerlingguy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +d8ux

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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

alexpott’s picture

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

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

gagarine’s picture

Issue tags: -d8ux +Usability

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

jonasdk’s picture

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

alexpott’s picture

@jonasdk because #6 was not answered and #7

gargsuchi’s picture

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

alexpott’s picture

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

q0rban’s picture

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?

q0rban’s picture

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?

q0rban’s picture

Status: Needs work » Needs review
FileSize
1.03 KB

@gargsuchi, looks like something from #944582: ./sites/default/files directory permission check is incorrect during install AND status report might have snuck in your patch—file_directory_is_writable() doesn't appear to be in 8.5.x yet. Re-roll attached.

gargsuchi’s picture

Thats correct @q0rban.
Thanks for this patch.

gargsuchi’s picture

alexpott’s picture

Status: Needs review » Needs work

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.

pjcdawkins’s picture

Status: Needs work » Needs review
FileSize
1.25 KB

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.

alexpott’s picture

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

Status: Needs review » Needs work

The last submitted patch, 20: 2880445-remove-config-write-warning-20.patch, failed testing. View results

pjcdawkins’s picture

Removing the test that was checking for this message

Status: Needs review » Needs work

The last submitted patch, 23: 2880445-remove-config-write-warning-23.patch, failed testing. View results

pjcdawkins’s picture

Status: Needs work » Needs review

The test succeeded.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Less code. Nice.

  • larowlan committed 8fb5919 on 8.5.x
    Issue #2880445 by pjcdawkins, japerry, gargsuchi, q0rban: Config sync...

  • larowlan committed 7c6bf57 on 8.4.x
    Issue #2880445 by pjcdawkins, japerry, gargsuchi, q0rban: Config sync...
larowlan’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

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.

larowlan’s picture

Status: Fixed » Closed (fixed)

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