drupal_mkdir does not set permissions to directories it created recursively

Thanks for the patch. I will test it later.
grossmann-mcs

Deleted for double post. Sorry.
grossmann-mcs

Confirm that the #4 patch works fine under 7.12

I can also confirm that patch #4 works on 7.12. I would really like to see this commited, thanks :)

I'm wondering why this is behaving the way it is, the last directory having the correct permissions but not the parent directories. Is mkdir applying the umask only on the parent directories?

The PHP docs for umask warn:

Avoid using this function in multithreaded webservers. It is better to change the file permissions with chmod() after creating the file. Using umask() can lead to unexpected behavior of concurrently running scripts and the webserver itself because they all use the same umask.

I think this is something we have to worry about.

Also, needs tests.

Tagging.

Has been committed to D7 ?

@paolomainardi no, hence the "needs backport to D7". It will be committed to D8 first, and then moved for backport, and marked "fixed" when it has been committed.

Hi all,

I changed title to be more suitable.

I maded a patch with comment #10 in mind.

Also, I provided for following situations:

1. If some part of path is already exists, function will not chmod it too - to avoid this, I calculate already existing part of $uri
2. The creation order not changed: if we create one folder step by step, there can be situation when someone specifies a mode which does not allow owner to create new entries in directory. So, drupal_mkdir use $recursive flag as usual, and then apply drupal_chmod to all created folders.

Maybe it would be better to begin chmod from the end of directories: if we create $f = 'public://test1/test2/test3/'; file_prepare_directory($f, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS);, then we should chmod to 'public://test1/test2/test3/', then 'public://test1/test2/' and at last to 'public://test1/' - because of forgoing reasons. Currently we made it from the begining.

I think there is can be a problems on Windows systems - because of using another directory delimiter.

Function was tested with $uri with schema ('public://test1/test2/') and absolute uri ('/var/www/drupal8-core/sites/default/files/test_a1/test_a2/test_a3'). Maybe there is problems with relative uri.

Test assertion plan:

1. Make sure drupal_mkdir() not process malformed uri
2. Make sure drupal_mkdir() process recursively uri with schema ('public://test1/test2/')
3. Make sure drupal_mkdir() process recursively absolute uri ('/var/www/drupal8-core/sites/default/files/test_a1/test_a2/test_a3')
4. Make sure drupal_mkdir() process recursively relative uri ('../sites/default/files/test_b1/test_b2/test_b3')
5. Make sure all of this for non-recursive case.

Where shoud I place a file with test of that core functions? I looked for tests of drupal file subsystem in D7 and D8 without results! Where can I see an example of core api tests? Please point me right direction if you can.

Just warning in non-recursive case in drupal_chmod(). Fixed.

Probably I found out where to put tests for drupal_chmod:

core/modules/system/lib/Drupal/system/Tests/System

or

core/modules/system/tests/file.test

#16: drupal-mkdir_recursive-1068266-16.patch queued for re-testing.

#16: drupal-mkdir_recursive-1068266-16.patch queued for re-testing.

Writing tests

+++ b/core/includes/file.incundefined
@@ -2322,12 +2322,49 @@ function drupal_mkdir($uri, $mode = NULL, $recursive = FALSE, $context = NULL) {
+      drupal_chmod($uri_existing, $mode);

This will give a false positive if it fails.

Also added tests.

Including new patch plus interdiff.

d'oh. No longer needs tests. But they do need to be reviewed.

#22: mkdir_recursive-1068266-21.patch queued for re-testing.

Thank you, scottrigby!

Made little fixes for #21.

Lost the tag in there somewhere

@andyceo looks like my patch failed because i renamed the method, and also didn't break. I added your fix, and also removed the extra 'check' from `testFileCheckDirectoryHandling` and `checkFileCheckDirectoryHandling` because it seems redundant now.

I'm adding an interdiff against my patch in #22 only because #28 contains multiple commits.

edit: also cleaned up 1 line documentation to match standards.

You can not do this. You should patch the local stream only because you have no idea that if foo/bar/baz exists then foo/bar does as well. That's a presumption made on local filesystem grounds.

Sorry for not addign this is in #34 : thanks to everyone working on this patch.

Here's the IRC discussion. Let's presume you have an URI like s3://styles/large/s3/foo.jpg (coming from image_style_path). This does not mean that s3://styles/large has any meaning at all. Core does not use it, so why should the stream wrapper handle it? Accessing it might throw an exception for all you know. So what we need to do is

1. Introduce drupal_mkdir_local().This handles file paths like sites/default/files/styles/large/public/foo.jpg and NOT schemed URIs.
2. drupal_mkdir checks for a scheme, if it's not there, it's a local path so it calls drupal_mkdir_local().
3. The local stream wrapper already has a local path so it calls drupal_mkdir_local().

Updated issue summary.

New patch for 1-3 in new issue summary. New tests need to be written.

Here's a new patch based on IRC with chx & zendoodles.
I left an {@inheritdoc} in there but I'm tired and need to upload this before I forget (doc can be updated if this is looking like the right direction) =)

This fails because $directory_path .= DIRECTORY_SEPARATOR . $directory; makes it start with the root. I have fixed that and tightened the code just a tiny little bit. Tiny bit: you can move the whole thing into one while condition which we do not want to do because the readability of that is horrid. I also added / cleaned up the comments a tiny bit. I know it installs at least. I also renamed the new helpers to private.

Removed one outdated comment.

Directories must be executable and writable for a directory to be created within them.
Not necessary to set $mode to file_chmod_directory variable if default.
Simplified _drupal_mkdir_local() a bit.
fileperms() returns directory bit so can't be used in the test.

Making some improvements.

This issue is not related to umask at all. The problem is simply that PHP's mkdir() only sets the mode on the top-level directory being created.

Overriding umask when creating directories would be a separate issue.

I was also able to simplify the code and only introduce one function instead of two.

Interdiff coming up.

interdiff #45 => #49

@jbrown Thank you for your effort on this, but I think maybe you misunderstand the issue. We've already looked at this approach (see the progression of patches starting from the one in comment #38) with a variety of roadblocks.

Also, the test changes no longer actually test the issue here. If you make changes to tests, please upload a tests only patch in addition to your complete patch to demonstrate failing tests without the fixes in the full patch.

(See also this duplicate issue #1196382: styles subfolders get wrong folder permissions (0755) which is broken with 0755 for directory permissions, the current behavior.)

+++ b/core/modules/system/lib/Drupal/system/Tests/File/DirectoryTest.php
@@ -32,24 +32,19 @@ class DirectoryTest extends FileTestBase {
     // Check that new directory permissions were set properly.
-    $this->assertDirectoryPermissions($parent_path, 0444);
-    $this->assertDirectoryPermissions($child_path, 0444);
-
-    // Check that existing directory permissions were not modified.
-    $this->assertDirectoryPermissions($directory, $old_mode);
+    $this->assertDirectoryPermissions($parent_path, 0755);
+    $this->assertDirectoryPermissions($child_path, 0755);

(Adding the needs tests tag back based on scottrigby's comment in #38 and so we don't loose the ones we have based on the most recent patch.)

This is the test only from #47 and #49. We will see -- if either passes, the test is not good.

I am reworking this patch.

@ZenDoodles you do not understand my patch - please read my patches more carefully before commenting.

My previous patch includes chx's request that we should not override the recursive functionality for uris with schemes - these should be handled by the scheme implementation:

+  // If recursive and there is no URI scheme, create each missing component of
+  // the parent directory individually so they are created with the correct
+  // mode. Schemes can handle this issue in their own implementation.
+  if ($recursive && !file_uri_scheme($uri)) {

and

     if ($options & STREAM_REPORT_ERRORS) {
-      return mkdir($localpath, $mode, $recursive);
+      return drupal_mkdir($localpath, $mode, $recursive);
     }
     else {
-      return @mkdir($localpath, $mode, $recursive);
+      return @drupal_mkdir($localpath, $mode, $recursive);
     }

The tests from #45 were totally broken for these reasons:

• Passing a value from fileperms() to assertDirectoryPermissions() does not work as fileperms() returns the full mode including the directory bit.
• It is not possible to create directories recursively unless the mode provides both writing and execution.
• drupal_mkdir() was not being called with $recursive set to TRUE (which is the very thing it is supposed to test)

I have realised that I misunderstood the issue summary:

PHP mkdir() does not set file permissions as expected when we create directories recursively

This implies that mkdir() does set the file permissions as expected when we do not create directories recursively. However, this is not correct. mkdir() is always influenced by the umask - it is just that in file_prepare_directory() we override this with chmod - but only for the top-level directory.

The solution is to define that drupal_mkdir() overrides umask for both the top-level directory created and any components of the directory path that get created due to $recursive being specified.

#54: 1068266.patch queued for re-testing.

Wow - it still applies!!

Would this address #1791280: folders (auto) created with wrong permissions too? If so, then I can mark it as duplicate of this one. If not, would it be possible to take that issue on board? It is very similar, isn't it?

I back-ported patch #54 to Drupal 7 and can confirm that it fixes #1791280: folders (auto) created with wrong permissions. I think it is safe to mark it as duplicate of this issue.

The changes in includes/file.inc are all the same. Changes of lib/Drupal/Core/StreamWrapper/LocalStream.php in D7 will need to be made in includes/stream_wrappers.inc instead.

Would be nice when the patch is submitted and back-ported soon, because it is really an annoying bug.

@chx is all over this issue, so would be good if he'd review and eventually sign off #54

Attached the back-port to D7

Test previous patch for 7.x.

#60: drupal-mkdir_recursive-1068266-60.patch queued for re-testing.

I don't think this was committed to D8 yet, please don't move it to D7 until it is.

Just moved it to test the D7 patch.

The patch #60 works with my Drupal 7.18 version on my local machine. The problem occurs when I create or edit image styles. The permission of the directory it creates or re-creates becomes 700 or drwx------. I dont know if this is related to this problem or it's a settings problem on my local machine.
I'm using CentOS 5.8 64bit, PHP 5.3.3, Apache 2.2.3

#54: 1068266-tests-only.patch queued for re-testing.

#54: 1068266.patch queued for re-testing.

#54: 1068266-tests-only.patch queued for re-testing.

#54: 1068266.patch queued for re-testing.

Yes #54 is good. I clicked an admin retest for it (boo abusing test admin powers), just to see it still applies and apparently it does, I have no doubts it'll pass again.

#54: 1068266.patch queued for re-testing.

Committed and pushed to 8.x. Thanks!

Marking back to 7.x, but AFAIK those URI -> uri changes in the docs look incorrect, so we should re-roll without those.

D7 reroll.

I found a regression issue of this ticket which blocks drupal 8 installation on windows OS.

Step to reproduce the issue.

1. try to install drupal-8 and on step 3, you can see this screen:

2. var_dump($uri); var_dump(DIRECTORY_SEPARATOR); in drupal_mkdir() shows exactly why it doesn't work:

The main issue is explode() at https://gist.github.com/vijaycs85/4756532#file-file-php-L11

This should fix it on Windows - can someone with a Windows setup test it?

#76 fixes the issue on Windows XP, thanks!

#76 works for both install and simpletest. Thanks @jbrown.

removed tag.

Thanks guys.
I guess this now may fix the scary problem (pictured in #75) that killed all our Windows-based core contribution sprint beginners at the Sydney Drupalcon sprint.
It kept saying the problem was permissions, but nothing sane we could try would get past it.

Can someone RTBC?

Committed/pushed the followup, thanks!

Thanks catch!

Here's the D7 reroll.

#84: 1068266.patch queued for re-testing.

Good to go, if test is green.

+  function testFileCheckLocalDirectoryHandling() {
+    $directory = conf_path() . '/files';

Er, this will pollute the actual site's files directory with something that remains after the test run and never gets cleaned up. It should be using the simpletest version of the public files directory instead.

Otherwise, this looked good to me (nice work!), but how much has the Drupal 7 patch been manually tested? A patch like this definitely requires manual testing in several different scenarios (especially if an earlier version already broke things on Windows).... I did some light testing on my own (including uploading/installing modules via the user interface which seemed like a code path that used drupal_mkdir() in interesting ways) and so far so good, but more people really need to try it out.

***

A couple other comments (less likely to be commit blockers though):

1. +  if ($recursive) {
   +    // Ensure the path is using DIRECTORY_SEPARATOR.
   +    $uri = str_replace('/', DIRECTORY_SEPARATOR, $uri);
   .....
   +  }
   +
   +  // Do not check if the top-level directory already exists, as this condition
   +  // must cause this function to fail.
   +  if (!_drupal_mkdir_call($uri, $mode, FALSE, $context)) {
   

   Seems odd to me that $uri is modified only in the if() statement, but then used either way later on. Either it needs to be modified or it doesn't, right? (However, I'm not sure if this matters in practice.)

2. +  // Not necessary to use drupal_chmod() as there is no scheme.
   +  return chmod($uri, $mode);
   

   Well, I guess... though drupal_chmod() also does a "@" to suppress errors on the chmod() call (not sure why), and other nice things like a watchdog() message on failure. Should we at least use the "@" here? I think I agree with not calling drupal_chmod() itself though, since it does some high-level stuff with stream wrappers that might actually fail in certain situations where drupal_mkdir() would have worked before.

3. Would be nice to remove t() from the assertion messages in the tests.

Fixed the testing directory.

1. The only reason we need be using forward slashes is so the explode() on the next line works. It doesn't actually make sense to use DIRECTORY_SEPARATOR: http://alanhogan.com/tips/php/directory-separator-not-necessary - I've removed it. PHP handles it behind the scenes.

2. I think it is better to let chmod() just output it's own errors. This will automatically go into watchdog.

3. Fixed.

#88: 1068266-followup.patch queued for re-testing.

#88: 1068266-followup.patch queued for re-testing.

#88: 1068266-followup.patch queued for re-testing.

#88: 1068266-followup.patch queued for re-testing.

#88: 1068266-followup.patch queued for re-testing.

#88: 1068266-followup.patch queued for re-testing.

+++ b/core/includes/file.incundefined
@@ -1746,10 +1746,11 @@ function drupal_mkdir($uri, $mode = NULL, $recursive = FALSE, $context = NULL) {
-    // Ensure the path is using DIRECTORY_SEPARATOR.
-    $uri = str_replace('/', DIRECTORY_SEPARATOR, $uri);
+    // Ensure the path is using all forward slashes so it can be exploded.
+    $uri = str_replace('\\', '/', $uri);
     // Determine the components of the path.
-    $components = explode(DIRECTORY_SEPARATOR, $uri);
+    $components = explode('/', $uri);
+    // Don't handle the top-level directory in this loop.
     array_pop($components);
     $recursive_path = '';
     foreach ($components as $component) {
@@ -1765,7 +1766,7 @@ function drupal_mkdir($uri, $mode = NULL, $recursive = FALSE, $context = NULL) {

@@ -1765,7 +1766,7 @@ function drupal_mkdir($uri, $mode = NULL, $recursive = FALSE, $context = NULL) {
         }
       }
 
-      $recursive_path .= DIRECTORY_SEPARATOR;

I'd leave DIRECTORY_SEPARATOR anyway. It does no harm at all and it looks much nicer to use a constant instead of a hard-coded value.

Updated issue summary.

Re-rolled #88 against head, not sure if we need to implement #96

Note that we recently implemented a smarter algorithm for this in PhpStorage:

#2110863: Support open_basedir

Already in that issue, we discussed that we should probably change drupal_mkdir() to use the same algorithm.

I'm not sure whether this is the right issue for doing that though, since this appears to be a bug in D7, too...

I'm confused —

+++ b/core/includes/file.inc
@@ -1516,9 +1516,7 @@ function drupal_mkdir($uri, $mode = NULL, $recursive = FALSE, $context = NULL) {
-    $uri = str_replace('/', DIRECTORY_SEPARATOR, $uri);
...
     $components = explode(DIRECTORY_SEPARATOR, $uri);

The subsequently following code relies on DIRECTORY_SEPARATOR, but you're removing it?

$uri = str_replace('/', DIRECTORY_SEPARATOR, $uri); ensures that the only separator in the string is DIRECTORY_SEPARATOR otherwise you can have both / and \ in the same path.

I realize not everyone has access to this on their server, thus the necessity for this patch but this issue was resolved for me by changing a line in the suphp.conf file. It sets umask to 0077 by default, changing it to 0022 made new child directories create with the correct permissions.

Here's a straight reroll of #88.

What that means is: NO modification to drupal_mkdir() (which is just a wrapper for a service now), and only changes to DirectoryTest.

The changes to the test only do two things:

1. Access public:// instead of assembling a path from site.path . '/files'.
2. Remove uses of t().

DirectoryTest passes locally after modification, so either the test is no good or subsequent work on Drupal addressed the issue of recursive permissions.

If this is still an issue it could be addressed in Drupal\Core\File\FileSystem::mkdir() and then tested against.

Really though it's probably still a D7 issue exclusively. Change the version once this test passes ::fingers crossed::

The D8 patch was committed in #83. Yay!

There was a backport patch in #84. Yay!

That patch was reviewed in #87. Yay!

The patch in #88 switched versions to D8, but incorporated some changes from the review in #87. Erm...

So future work on this issue should re-roll against Drupal 7.x on #84 and move forward with the review in #87.

Here's the reroll of #84.

Applied on a drupal 7.x, worked well, many thanks !

A bit confused about this, but let's get the ball rolling again, started from #104 and added the feedback from #87, let's see what the testbot thinks.

It finally passed, had some odd errors on the testbot.

Do we need to add a separate issue for the remaining 7.x patch?

@pcambra yes, if you want (see https://www.drupal.org/core/backport-policy)

In some ways drupal's policies regarding fixing and back porting is becoming unnecessarily complicated.

The policy itself says,

Alternatively the needs backport to D7 issue tag can be added to the parent issue to indicate that a child issue still needs to be created (but actually creating the child issue is preferred).

Personally, I would take issue with the merits of what is in brackets.

Why should an issue that began 5 years ago as a Drupal 7 issue, which has subsequently made way for Drupal 8 to take precedence, now need a separate issue?

Using the "needs backport to D7" tag makes far more sense, especially since the patch already exists in the issue thread.

@stefan.r, just created #2789723: [D7 backport] drupal_mkdir does not set permissions to directories it created recursively to follow up this.

RTBC for #103 for 8.x.

Hi, I went ahead and rerolled #103's patch. Here is the only conflict from the reroll.

<<<<<<< 27ab301a6d198ce3c9e072f53cc42544ecc3851f:core/tests/Drupal/KernelTests/Core/File/DirectoryTest.php
  public function testFileCheckLocalDirectoryHandling() {
    $site_path = $this->container->get('site.path');
    $directory = $site_path . '/files';
=======
  function testFileCheckLocalDirectoryHandling() {
    $directory = 'public://';
>>>>>>> Applying patch from issue 1068266 comment 10407477:core/modules/system/src/Tests/File/DirectoryTest.php

I resolved it by adding public to the function from the patch. Patch now applies cleanly to 8.4.x-dev.

This was fixed in 8.x by webchick back in 2013, please do not re-open this issue. Also, see duplicate 8.x issue which needs to be closed as a duplicate (duplicate issue below)
#2211657: file_unmanaged_copy not working properly when the destination directory does not exist.

For proof of fix;
see commit:
1068266

commit 5b6c981231eaae67608a37e90113d862631bb72b
Author: webchick <webchick@24967.no-reply.drupal.org>
Date:   Fri Feb 8 16:25:48 2013 -0800

    Issue #1068266 by linclark, andyceo, scottrigby, chx, jbrown, ñull, weboide: Fixed drupal_mkdir()

Therefore marking this as fixed in order to allow 7.x backport to get committed.
see RTBC backport for 7.x

#2789723: [D7 backport] drupal_mkdir does not set permissions to directories it created recursively