Updated: Comment #3

Problem/Motivation

If you installl via Drush, Drupal creates the files and the files/config_hash directories with permissions that doesn't allow the webserver to write them. Fun!

Proposed resolution

Go 0777, when installing via CLI. There are no changes when installing via Web or Simpletest non-interactive installer.

Remaining tasks

User interface changes

None.

API changes

None.

#1908440: Relax MTimeProtectedFileStorage permissions for DX, drush integration and world domination

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman’s picture

We spoke with alexpott. The agreement is to rework the patch so that the mode is passed into install_drupal as a param. Then drush will pass 777 when it does the install. The web installer will not change.

moshe weitzman’s picture

Status: Active » Needs review
FileSize
7.83 KB

Patch, as discussed at Drupalcon Friday sprint. I'm updating issue summary as well.

moshe weitzman’s picture

Sorry, one unintended change snuck in.

moshe weitzman’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

FileSize
7.5 KB

Fixed a missing doxygen.

moshe weitzman’s picture

Title: Drush and Drupal 8: part 2 » Let non-interactive installers determine mode of files directory and its subdirectories
Status: Needs review » Reviewed & tested by the community

Thanks. Retitled for clarity.

moshe weitzman’s picture

Thanks. Better title.

moshe weitzman’s picture

Priority: Normal » Major

Drush and webserver can't both write to config dirs right now so escalating. After this, at least both can write after a drush install.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/file.inc
@@ -481,12 +481,15 @@ function file_create_url($uri) {
+ *   Integer value for the permissions. Consult PHP chmod() documentation for

Can we do @param int $mode then say octal in the docs?

moshe weitzman’s picture

Status: Needs work » Needs review
FileSize
7.88 KB

Rerolled with catch's Doxygen suggestion. No other changes.

catch’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Just a small question...

+++ b/core/modules/image/image.install
@@ -11,7 +11,8 @@
-  file_prepare_directory($directory, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS);
+  $mode = isset($GLOBALS['install_state']['mode']) ? $GLOBALS['install_state']['mode'] : NULL;
+  file_prepare_directory($directory, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS, $mode);

+++ b/core/modules/system/system.install
@@ -371,7 +371,8 @@ function system_requirements($phase) {
-      file_prepare_directory($directory, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS);
+      $mode = isset($GLOBALS['install_state']['mode']) ? $GLOBALS['install_state']['mode'] : NULL;
+      file_prepare_directory($directory, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS, $mode);

in general we are trying to remove global variables, not add them. Does $install_state need to be passed into these function? And/or is there an issue to move this to the Request object and pull it from there?

moshe weitzman’s picture

hook_install(). does not pass $install_state nor does it have the ability to do so today. see https://api.drupal.org/api/drupal/core%21modules%21system%21system.api.p... and its caller https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...

So, addressing that would be a huge change. I don't know of efforts underway to improve this. Perhaps #1530756: [meta] Use a proper kernel for the installer would help. IMO, the installer should be a separate app, outside of Drupal (but bundled with it). Drupal should only focus on serving requests, not self-install.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yeah, agreed with that.

I guess I was hoping we could replace those $GLOBALS calls with something like:

$mode = \Drupal::request()->get('install_state')->mode

...or something. But indeed, it sounds like this is #1530756: [meta] Use a proper kernel for the installer's job to clean up.

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.