Patch confusion

This issue has patches for both 8 and 7. The 7 issue is created, and the goal remaining on this issue is to test the rerolled patch in #123 for 8.4.x.

Problem/Motivation

PHP mkdir() does not set file permissions as expected when we create directories recursively (See http://us.php.net/manual/en/function.mkdir.php#100106 for more information.) As a result:

Proposed resolution

The original patches addressed the issue at the drupal_mkdir() level, but it must also be addressed at the LocalStreamWrapper level. Also we do not want to repeat the code for the recursive bits in both locations, so the best way to address it is with a new function drupal_mkdir_local() to also recursively set the permissions.

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. (see also http://www.advomatic.com/blogs/aaron-winborn/stream-wrappers-and-you ). So what we need to do is:

Remaining tasks

  1. Introduce drupal_mkdir_local().This handles file paths like sites/default/files/styles/large/public/foo.jpg and NOT schemed URIs. Can be refactored from http://drupalbin.com/21585
  2. drupal_mkdir checks for a scheme, if it's not there, it's a local path so it calls drupal_mkdir_local(). http://privatepaste.com/fda114cd01
  3. The local stream wrapper already has a local path so it calls drupal_mkdir_local(). http://privatepaste.com/7a5832a7fd
  4. Write a simpletest to check a new recursively created local directory for correct file system permissions.
  5. Backport the above to D7 s#DrupalLocalStreamWrapper::mkdir#LocalStreamWrapper::mkdir#

Original report by [weboide]

// Text of original report here.
file_prepare_directory does not seem to set the permissions to folders it has created recursively.

For example:

In settings.php:

$conf['file_chmod_directory'] = 02775; // even tried with 0775

And then run:

$f = 'public://test1/test2/test3/'; file_prepare_directory(\$f, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS);

would create

`-- [drwxr-sr-x]  test1
    `-- [drwxr-sr-x]  test2
        `-- [drwxrwxr-x]  test3

I was expecting to see test1 and test2 with group write permission.

CommentFileSizeAuthor
#123 1068266_rerolled_103-123.patch2.04 KBgeorob
#107 interdiff-1068266-107.txt1.9 KBpcambra
#107 1068266-107.patch6.7 KBpcambra
#104 1068266_reroll_84.patch6.7 KBMile23
PASSED: [[SimpleTest]]: [MySQL] 41,614 pass(es). View
#103 1068266_103.patch2.01 KBMile23
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,550 pass(es). View
#97 1068266-mkdir_recursive-96.patch3.42 KBwesleydv
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1068266-mkdir_recursive-96.patch. Unable to apply patch. See the log in the details link for more information. View
#88 1068266-followup.patch3.34 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 57,522 pass(es). View
#84 1068266.patch6.94 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 40,080 pass(es). View
#76 1068266-windows.patch754 bytesjbrown
PASSED: [[SimpleTest]]: [MySQL] 50,079 pass(es). View
#75 requirement-problem.png40.35 KBvijaycs85
#75 error.png22.77 KBvijaycs85
#74 1068266.patch6.79 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 39,747 pass(es). View
#60 drupal-mkdir_recursive-1068266-60.patch3.64 KBñull
PASSED: [[SimpleTest]]: [MySQL] 39,595 pass(es). View
#54 1068266-tests-only.patch2.71 KBjbrown
FAILED: [[SimpleTest]]: [MySQL] 49,058 pass(es), 4 fail(s), and 0 exception(s). View
#54 1068266.patch7.03 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 49,526 pass(es). View
#52 1068266.patch1.64 KBchx
FAILED: [[SimpleTest]]: [MySQL] 41,261 pass(es), 5 fail(s), and 0 exception(s). View
#52 1068266_0.patch1.64 KBchx
PASSED: [[SimpleTest]]: [MySQL] 41,265 pass(es). View
#50 interdiff.txt7.46 KBjbrown
#49 1068266.patch5.21 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 41,238 pass(es). View
#47 1068266.patch5.51 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 41,123 pass(es). View
#45 1068266_45.patch5.71 KBchx
FAILED: [[SimpleTest]]: [MySQL] 36,970 pass(es), 12 fail(s), and 6 exception(s). View
#43 1068266_43.patch5.71 KBchx
FAILED: [[SimpleTest]]: [MySQL] 36,914 pass(es), 78 fail(s), and 48 exception(s). View
#42 1068266_42.patch5.9 KBchx
FAILED: [[SimpleTest]]: [MySQL] 36,900 pass(es), 78 fail(s), and 48 exception(s). View
#40 mkdir_recursive-1068266-40.patch5.75 KBscottrigby
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#38 mkdir_recursive-1068266-38.patch4.34 KBscottrigby
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#32 mkdir_recursive-1068266-32.patch4.18 KBscottrigby
FAILED: [[SimpleTest]]: [MySQL] 37,155 pass(es), 34 fail(s), and 64 exception(s). View
#32 interdiff.txt2.24 KBscottrigby
#28 drupal_mkdir_recursive_1068266_28.patch5.34 KBandyceo
FAILED: [[SimpleTest]]: [MySQL] 37,158 pass(es), 18 fail(s), and 60 exception(s). View
#22 mkdir_recursive-1068266-21.patch4.08 KBscottrigby
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#22 interdiff-21.txt2.54 KBscottrigby
#16 drupal-mkdir_recursive-1068266-16.patch2.39 KBandyceo
PASSED: [[SimpleTest]]: [MySQL] 36,750 pass(es). View
#14 drupal-mkdir_recursive-1068266-14.patch2.36 KBandyceo
FAILED: [[SimpleTest]]: [MySQL] 36,858 pass(es), 0 fail(s), and 12 exception(s). View
#4 1068266-04-drupal-mkdir-perms.patch907 byteslinclark
PASSED: [[SimpleTest]]: [MySQL] 33,258 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

weboide’s picture

Status: Active » Closed (won't fix)
weboide’s picture

Status: Closed (won't fix) » Active

Couldn't we use something like that in drupal_mkdir?:

From the link above:

The mode on your directory is affected by your current umask. It will end
up having ([mkdir-mode] and (not [umask])

$oldumask = umask(0);
mkdir('mydir', 0777); // or even 01777 so you get the sticky bit set
umask($oldumask);
linclark’s picture

Version: 7.0 » 7.x-dev
Priority: Minor » Normal

Marked #1196382: styles subfolders get wrong folder permissions (0755) as a duplicate. I'm upping the priority because this affects users on shared hosting, who cannot delete the directories created by image module because of this.

linclark’s picture

Version: 7.x-dev » 8.x-dev
Status: Active » Needs review
FileSize
907 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,258 pass(es). View

This patch applies the solution proposed in #2. It gets the current umask, sets umask to 0, creates the directories, and then resets the umask to whatever it was.

You can test this by creating an Image Style. Before applying the patch, if you ls -al sites/default/files/styles/, you will see that the file perms on the new image style file are 755 (if they aren't, that means you have a different existing umask). After the patch, the file perms will be 775.

grossmann’s picture

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

grossmann’s picture

Deleted for double post. Sorry.
grossmann-mcs

guile2912’s picture

Confirm that the #4 patch works fine under 7.12

linclark’s picture

Priority: Normal » Major

This bug affects users on shared hosting, who cannot delete the directories created by image module.

Because it's likely that these people won't know what the issue is or have the permissions to fix it even if they do, I'm going to bump this to major.

Volx’s picture

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?

tim.plunkett’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

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.

catch’s picture

Issue tags: +needs backport to D7

Tagging.

paolomainardi’s picture

Has been committed to D7 ?

tim.plunkett’s picture

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

andyceo’s picture

Title: file_prepare_directory does not set permissions to directories it created recursively » drupal_mkdir does not set permissions to directories it created recursively
Status: Needs work » Needs review
FileSize
2.36 KB
FAILED: [[SimpleTest]]: [MySQL] 36,858 pass(es), 0 fail(s), and 12 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, drupal-mkdir_recursive-1068266-14.patch, failed testing.

andyceo’s picture

FileSize
2.39 KB
PASSED: [[SimpleTest]]: [MySQL] 36,750 pass(es). View

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

andyceo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Needs tests, -needs backport to D7

The last submitted patch, drupal-mkdir_recursive-1068266-16.patch, failed testing.

andyceo’s picture

Status: Needs work » Needs review
andyceo’s picture

scottrigby’s picture

Issue tags: +drupaldelphia2012

Writing tests

scottrigby’s picture

Status: Needs review » Needs work
Issue tags: -drupaldelphia2012
FileSize
2.54 KB
4.08 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
+++ 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.

scottrigby’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, mkdir_recursive-1068266-21.patch, failed testing.

scottrigby’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

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

andyceo’s picture

Issue tags: -needs backport to D7

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

Status: Needs review » Needs work
Issue tags: +needs backport to D7

The last submitted patch, mkdir_recursive-1068266-21.patch, failed testing.

andyceo’s picture

FileSize
5.34 KB
FAILED: [[SimpleTest]]: [MySQL] 37,158 pass(es), 18 fail(s), and 60 exception(s). View

Thank you, scottrigby!

Made little fixes for #21.

andyceo’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

Issue tags: +drupaldelphia2012

Lost the tag in there somewhere

Status: Needs review » Needs work

The last submitted patch, drupal_mkdir_recursive_1068266_28.patch, failed testing.

scottrigby’s picture

Status: Needs work » Needs review
FileSize
2.24 KB
4.18 KB
FAILED: [[SimpleTest]]: [MySQL] 37,155 pass(es), 34 fail(s), and 64 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, mkdir_recursive-1068266-32.patch, failed testing.

chx’s picture

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.

chx’s picture

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

linclark’s picture

As I mentioned to chx in IRC, it is important to ensure whatever gets committed here fixes #1196382: styles subfolders get wrong folder permissions (0755) as well, or we need to reopen that issue.

I marked that issue as a duplicate of this one in October. The only reason that I moved this issue to major is because the other issue likely affects users who are unable to diagnose the problem for themselves.

chx’s picture

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().
ZenDoodles’s picture

Issue summary: View changes

Updated issue summary.

scottrigby’s picture

Status: Needs work » Needs review
FileSize
4.34 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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

Status: Needs review » Needs work

The last submitted patch, mkdir_recursive-1068266-38.patch, failed testing.

scottrigby’s picture

Status: Needs work » Needs review
FileSize
5.75 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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) =)

Status: Needs review » Needs work

The last submitted patch, mkdir_recursive-1068266-40.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
5.9 KB
FAILED: [[SimpleTest]]: [MySQL] 36,900 pass(es), 78 fail(s), and 48 exception(s). View

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.

chx’s picture

FileSize
5.71 KB
FAILED: [[SimpleTest]]: [MySQL] 36,914 pass(es), 78 fail(s), and 48 exception(s). View

Removed one outdated comment.

Status: Needs review » Needs work

The last submitted patch, 1068266_43.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
5.71 KB
FAILED: [[SimpleTest]]: [MySQL] 36,970 pass(es), 12 fail(s), and 6 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 1068266_45.patch, failed testing.

jbrown’s picture

Status: Needs work » Needs review
FileSize
5.51 KB
PASSED: [[SimpleTest]]: [MySQL] 41,123 pass(es). View

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.

jbrown’s picture

Assigned: Unassigned » jbrown
Status: Needs review » Needs work

Making some improvements.

jbrown’s picture

Assigned: jbrown » Unassigned
Status: Needs work » Needs review
FileSize
5.21 KB
PASSED: [[SimpleTest]]: [MySQL] 41,238 pass(es). View

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.

jbrown’s picture

FileSize
7.46 KB

interdiff #45 => #49

ZenDoodles’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

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

chx’s picture

Status: Needs work » Needs review
FileSize
1.64 KB
PASSED: [[SimpleTest]]: [MySQL] 41,265 pass(es). View
1.64 KB
FAILED: [[SimpleTest]]: [MySQL] 41,261 pass(es), 5 fail(s), and 0 exception(s). View

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

jbrown’s picture

Assigned: Unassigned » jbrown

I am reworking this patch.

jbrown’s picture

Assigned: jbrown » Unassigned
Issue tags: -Needs tests
FileSize
7.03 KB
PASSED: [[SimpleTest]]: [MySQL] 49,526 pass(es). View
2.71 KB
FAILED: [[SimpleTest]]: [MySQL] 49,058 pass(es), 4 fail(s), and 0 exception(s). View

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

andyceo’s picture

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

jbrown’s picture

Wow - it still applies!!

ñull’s picture

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?

ñull’s picture

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.

sun’s picture

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

ñull’s picture

FileSize
3.64 KB
PASSED: [[SimpleTest]]: [MySQL] 39,595 pass(es). View

Attached the back-port to D7

Status: Needs review » Needs work

The last submitted patch, drupal-mkdir_recursive-1068266-60.patch, failed testing.

Volx’s picture

Version: 8.x-dev » 7.x-dev

Test previous patch for 7.x.

Volx’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

Version: 7.x-dev » 8.x-dev

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

Volx’s picture

Just moved it to test the D7 patch.

webgeecko’s picture

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

jbrown’s picture

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

jbrown’s picture

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

jbrown’s picture

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

jbrown’s picture

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

chx’s picture

Status: Needs review » Reviewed & tested by the community

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.

jbrown’s picture

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

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs work

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.

jbrown’s picture

Status: Needs work » Needs review
FileSize
6.79 KB
PASSED: [[SimpleTest]]: [MySQL] 39,747 pass(es). View

D7 reroll.

vijaycs85’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work
FileSize
22.77 KB
40.35 KB

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:

requirement-problem.png

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

error.png

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

jbrown’s picture

Status: Needs work » Needs review
FileSize
754 bytes
PASSED: [[SimpleTest]]: [MySQL] 50,079 pass(es). View

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

plach’s picture

#76 fixes the issue on Windows XP, thanks!

vijaycs85’s picture

Issue tags: -needs backport to D7

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

vijaycs85’s picture

Issue tags: +needs backport to D7

removed tag.

dman’s picture

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.

jbrown’s picture

Can someone RTBC?

vijaycs85’s picture

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

Status: Reviewed & tested by the community » Fixed

Committed/pushed the followup, thanks!

jbrown’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Needs review
FileSize
6.94 KB
PASSED: [[SimpleTest]]: [MySQL] 40,080 pass(es). View

Thanks catch!

Here's the D7 reroll.

vijaycs85’s picture

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

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

Good to go, if test is green.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work
+  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.
jbrown’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
FileSize
3.34 KB
PASSED: [[SimpleTest]]: [MySQL] 57,522 pass(es). View

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.

jbrown’s picture

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

jbrown’s picture

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

vijaycs85’s picture

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

jbrown’s picture

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

jbrown’s picture

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

Status: Needs review » Needs work

The last submitted patch, 1068266-followup.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
Issue tags: +needs backport to D7, +drupaldelphia2012

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

slashrsm’s picture

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

slashrsm’s picture

Issue summary: View changes

Updated issue summary.

wesleydv’s picture

FileSize
3.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1068266-mkdir_recursive-96.patch. Unable to apply patch. See the log in the details link for more information. View

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

sun’s picture

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?

Status: Needs review » Needs work

The last submitted patch, 97: 1068266-mkdir_recursive-96.patch, failed testing.

jbrown’s picture

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

jphelan’s picture

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.

Mile23’s picture

Status: Needs work » Needs review
FileSize
2.01 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,550 pass(es). View

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::

Mile23’s picture

Version: 8.0.x-dev » 7.x-dev
FileSize
6.7 KB
PASSED: [[SimpleTest]]: [MySQL] 41,614 pass(es). View

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.

Status: Needs review » Needs work

The last submitted patch, 104: 1068266_reroll_84.patch, failed testing.

cutesquirrel’s picture

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

pcambra’s picture

Status: Needs work » Needs review
FileSize
6.7 KB
1.9 KB

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.

Status: Needs review » Needs work

The last submitted patch, 107: 1068266-107.patch, failed testing.

The last submitted patch, 107: 1068266-107.patch, failed testing.

pcambra’s picture

Status: Needs work » Needs review

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

  • webchick committed 5b6c981 on 8.3.x
    Issue #1068266 by linclark, andyceo, scottrigby, chx, jbrown, ñull,...
  • catch committed 727b07e on 8.3.x
    Issue #1068266 by jbrown: Followup to drupal_mkdir() does not set...

  • webchick committed 5b6c981 on 8.3.x
    Issue #1068266 by linclark, andyceo, scottrigby, chx, jbrown, ñull,...
  • catch committed 727b07e on 8.3.x
    Issue #1068266 by jbrown: Followup to drupal_mkdir() does not set...
pcambra’s picture

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

stefan.r’s picture

Issue tags: -needs backport to D7

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

izmeez’s picture

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.

pcambra’s picture

Fabianx’s picture

Version: 7.x-dev » 8.1.x-dev
Status: Needs review » Reviewed & tested by the community

RTBC for #103 for 8.x.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 107: 1068266-107.patch, failed testing.

  • webchick committed 5b6c981 on 8.4.x
    Issue #1068266 by linclark, andyceo, scottrigby, chx, jbrown, ñull,...
  • catch committed 727b07e on 8.4.x
    Issue #1068266 by jbrown: Followup to drupal_mkdir() does not set...

  • webchick committed 5b6c981 on 8.4.x
    Issue #1068266 by linclark, andyceo, scottrigby, chx, jbrown, ñull,...
  • catch committed 727b07e on 8.4.x
    Issue #1068266 by jbrown: Followup to drupal_mkdir() does not set...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

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

georob’s picture

Version: 8.3.x-dev » 8.4.x-dev
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +TCDrupal 2017
FileSize
2.04 KB

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.

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.