Problem/Motivation

There is no reason that I can see why the CONFIG_SYNC_DIRECTORY should need to be writable.

In the case where you want to install and run Drupal on a read-only filesystem, and/or importing configuration from a read-only directory, the current checks prevent installation.

Proposed resolution

Remove the check that requires $config_directories to be writable in the installer and updater. Also remove this check in the system requirements (the status report).

Remaining tasks

Review and patch

User interface changes

Two strings (in the installer) are changed. They are shown when the config directory cannot be created

API changes

None

Data model changes

None

Comments

pjcdawkins created an issue. See original summary.

pjcdawkins’s picture

Status: Active » Needs review
FileSize
3.28 KB
pjcdawkins’s picture

Issue summary: View changes
pjcdawkins’s picture

I missed another check. Due to the previous assumptions, this looks really messy so probably needs reworking

pjcdawkins’s picture

Issue summary: View changes
pjcdawkins’s picture

I had a typo in system_requirements()

The last submitted patch, 4: drupal-2607352-4.patch, failed testing.

Dane Powell’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for this. Having a read-only filesystem is a fairly common security measure (e.g. on Acquia Cloud), and should not prevent installation or throw errors in the status report. This patch fixes both of those.

The only argument I can see for checking for writability is to allow exporting of config to disk. But in this case, the check should be performed in the config export UI, not at install time or in system_requirements.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: drupal-2607352-6.patch, failed testing.

moshe weitzman’s picture

Status: Needs work » Reviewed & tested by the community

That test fail looks unrelated. Hopefully bot will retest now.

In discussions with @alexpott, we actually want core to get out of the business of exporting entirely. That will be left up to Contrib and CLI tools. So a web server writability requirement makes no sense.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: drupal-2607352-6.patch, failed testing.

Dane Powell’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: drupal-2607352-6.patch, failed testing.

Dane Powell’s picture

Status: Needs work » Reviewed & tested by the community
anavarre’s picture

Issue tags: +rc target triage

Not sure if the tag still applies now that we're approaching the GA, but maybe it'll raise awareness?

anavarre’s picture

Issue tags: +CMI
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Ah this is a really good point - there is no requirement that this is writeable by core.

  1. +++ b/core/modules/system/system.install
    @@ -534,16 +534,11 @@ function system_requirements($phase) {
    +  if (empty($GLOBALS['config_directories']) && $phase != 'install') {
    

    I think we should take the opportunity to just test that the staging directory is configured.

  2. +++ b/core/modules/system/system.install
    @@ -534,16 +534,11 @@ 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.', array('%file' => $site_path . '/settings.php')),
    +      '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 read or written.', array('%file' => $site_path . '/settings.php')),
    

    I think this should just be "can be read".

penyaskito’s picture

Status: Needs work » Needs review
FileSize
1.75 KB
5.18 KB

Fixed #17 with @alexpott help on IRC.

alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/system.install
    @@ -534,18 +534,21 @@ function system_requirements($phase) {
    +      // The SYNC directory was not set.
    

    I don't think SYNC is right - we could just say No directory has been configured.

  2. +++ b/core/modules/system/system.install
    @@ -534,18 +534,21 @@ function system_requirements($phase) {
    +        'value' => isset($sync_directory) ? t('Not readable') : t('Not present'),
    

    Nice idea.

penyaskito’s picture

This is not enough, as the htaccess logic is checking for the folder first and if the sync folder value doesn't exists it explodes there.
Should we move the logic in the check for no install block before the check for runtime block?

moshe weitzman’s picture

I didn't look into #20, but perhaps we just omit config dirs from system_requirements() entirely? Drupal core never really needs them. It is just when you get into multiple environments that they become useful.

alexpott’s picture

@moshe weitzman this is tricky - if the sync directory can be accessed via the webserver then we need the .htaccess present to prevent access.

moshe weitzman’s picture

Assigned: pjcdawkins » moshe weitzman
Status: Needs work » Needs review
FileSize
1.93 KB

What about this?

The attached patch removes the special checking for config dir in favor of just an .htaccess check where we already do those. file_ensure_htaccess() already calls config_get_config_directory() which throws an exception if a config dir does not exist.

moshe weitzman’s picture

I looked at the install.inc changes in #18 but I think they are not needed. If Drupal core is creating the config dir, it might as well make it writable. The bug we are specifically fixing here is that the status report makes a wrong assumption.

pjcdawkins’s picture

(edit: ignore this comment, it was a stupid mistake)

pjcdawkins’s picture

Status: Needs review » Needs work
pjcdawkins’s picture

Status: Needs work » Needs review

Hm. I changed install.inc because:

  1. the string is now untrue (on line 509), it doesn't matter if it can be made writable
  2. there's a file_put_contents() for the README which will fail, although I believe it will just fail silently

I think (1) still needs changing.

(2) perhaps doesn't matter, although it needs a comment at least, to state it will fail, and that we don't actually care if it fails (by the by, A README is not actually necessary in order to commit a directory to Git).

moshe weitzman’s picture

@pjcdawkins - did you mean to upload a new patch? your past patch was marked needs work by alexpott so i dont think you mean that the old one is in 'needs review'.

pjcdawkins’s picture

Issue summary: View changes
Status: Needs review » Needs work

I was probably too tired to be commenting, apologies, I meant to leave this 'Needs work'.

#24

The bug we are specifically fixing here is that the status report makes a wrong assumption.

I actually opened the issue because the installer makes the same wrong assumption - both the installer and the status report need to be fixed.

install_ensure_config_directory() currently fails if the config directory cannot be made writable.

alexpott’s picture

And also file_ensure_htaccess() assumes it can write to the config directory.

One of the tensions we have is that the staging directory might or might not be able to served by the webserver. If it can't be served then the .htaccess is pointless. BUT also now the root .htaccess prevents serving of yml files so... but potentially someone would override that in their files directory. Also what about IIS - we have web.config in root but we never bother to create them in file_ensure_htaccess().

pjcdawkins’s picture

That's OK, I think. If you make sure your config directory is not writable, then you can add an .htaccess file to it manually (both a dummy one or the real Drupal one would work). Ideally Drupal would stop checking for .htaccess at least on non-Apache servers, but that's a separate issue.

alexpott’s picture

@pjcdawkins well we still need to change file_ensure_htaccess since it shouldn't be trying to write to the config directory if it can't. TBH I'm not sure that the config directory line should be there.

alexpott’s picture

I think we should only add the .htaccess file to the config directory if Drupal itself creates the directory. And I think we should only report on a missing .htaccess from the sync directory if it is available via the webserver. This would mean if you put config sync outside the web root you don't have to have unnecessary files.

pjcdawkins’s picture

Re. #33

I think for simplicity's sake the .htaccess check should be the same pattern for CONFIG_SYNC_DIRECTORY as for public and private files directories. And I expect that Drupal security people would want to keep those .htaccess checks regardless of how the directory was created.

And I think we should only report on a missing .htaccess from the sync directory if it is available via the webserver

There's no easy way to detect that - perhaps normally the web root would be the Drupal root, but people might have all sorts of configurations, symlinks, etc.

If there were a way to detect if PHP was running behind Apache, then we could only include this check in that case... but it seems that's not really easy or reliable either.

Again, it might be simpler to keep this small obstacle (the .htaccess checks), but to ensure nothing actually breaks if Drupal tries to write to a non-writable directory. And of course we don't want a watchdog message on every single page request.

pjcdawkins’s picture

Assigned: moshe weitzman » Unassigned

Just to clarify my comment in #29:

We need to fix the installer, not just the requirements check, therefore I don't think #24 is the right approach.

I think patch #18 is going in a better direction, but it still needs work based on #19.

pjcdawkins’s picture

Title: The installer needlessly requires $config_directories to be writable » The installer and status report needlessly require $config_directories to be writable
kylebrowning’s picture

Im not seeing the installer fail, only the status report page.

pjcdawkins’s picture

@kylebrowning: The installer fails if you already have $config_directories[CONFIG_SYNC_DIRECTORY] in settings.php set to a read-only directory before you install (which should be a perfectly valid use case).

xjm’s picture

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

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pjcdawkins’s picture

Adding issue #2582475: Installation fails if a valid config sync directory is defined as related (it appears that installation is now completely blocked in 8.1.x if you have a sync directory defined)

pjcdawkins’s picture

xjm’s picture

So my first thought is that this looks like a legacy thing from when the active staging directory was a thing and we read and wrote config on the filesystem all the time.

it looks like we have two approaches, #18 and #23. It might help to update the summary explaining these two different approaches and listing the pros/cons of each (as well as someone confirming that both actually resolve the issue).

Also, depending on the actual approach, I think it should be possible to test some of this, at least codifying the expectations about the directory and confirming that different usecases for it still are possible? (Edit: In other words, make sure we don't break Drush or other workflow tools.)

alexpott’s picture

This issue was giving me deja-vu... we've got essentially a dupe in #2466197: Staging directory should not have to be writeable. ... but that has a fully formed patch with tests and it maintains some of the checks so people will be informed when things might go wrong - for example with the approaches here if a config directory does not exist do you get a warning anymore?

Tempted to close this issue in favour of the other issue but will wait to build consensus.

tedbow’s picture

@alexpott after looking at the patches in both it does seem like this issue is dupe of #2466197: Staging directory should not have to be writeable.

alexpott’s picture

Status: Needs work » Closed (duplicate)
John Pitcairn’s picture

Title: The installer and status report needlessly require $config_directories to be writable » The installer, updater and status report needlessly require $config_directories to be writable
Version: 8.1.x-dev » 8.2.x-dev
Issue summary: View changes
Status: Closed (duplicate) » Active

Reopening. #2466197: Staging directory should not have to be writeable. has been fixed, but the corresponding checks in the status report and updater are still present.

Should probably be a new issue.

John Pitcairn’s picture

Status: Active » Closed (duplicate)