drupal_mkdir() can't create an absolute path recursively

#1: 2043771.drupal.file-drupal_mkdir-recursive.patch queued for re-testing.

Thanks for finding this @joachim!

DIRECTORY_SEPARATOR is appended at the end of the loop:

$recursive_path .= DIRECTORY_SEPARATOR;

How about this approach?

Shouldn't this be comparing DIRECTORY_SEPARATOR rather than assuming it's a / ?

I actually want to get rid of DIRECTORY_SEPARATOR in this function: https://drupal.org/node/1068266#comment-7118718 but I put it back into the patch for consistency.

I'm confused by this... is this because with an absolute path, the first item of the array is a ''?

Yeah - if an absolute path is exploded then first string in the array empty and we need to ignore it.

Looks good to me. Can we call this RTBC?

+++ b/core/modules/system/lib/Drupal/system/Tests/File/DirectoryTest.phpundefined
@@ -50,6 +50,13 @@ function testFileCheckLocalDirectoryHandling() {
     $this->assertDirectoryPermissions($directory, $old_mode);
+
+    // Check creating a directory using an absolute path.
+    $absolute = $this->randomName();
+    // Create it within the local files directory anyway, as we know we can
+    // write there.
+    $absolute_path = realpath($directory) . DIRECTORY_SEPARATOR . $absolute;

This only creates one level directory. Shouldn't we also test recursive stuff (multi-level directories)?

Added an extra level to the directory created in the test.
Also, test descriptions are not suposed to be translated. The patch linked to in #7 fixes this.

Looks good to me. Let's wait for testbot.

It looks good, it is green and it is fairly simple patch. I'll set this to RTBC. Anybody feel free to change that if any other objections appear.

Nice find...

+++ b/core/includes/file.incundefined
@@ -1483,8 +1483,18 @@ function drupal_mkdir($uri, $mode = NULL, $recursive = FALSE, $context = NULL) {
+    // Is the filepath relative or absolute?

I think this comment should be a little more verbose and explain why $components[0] is == '' for absolute paths.

+++ b/core/modules/system/lib/Drupal/system/Tests/File/DirectoryTest.phpundefined
@@ -50,6 +50,10 @@ function testFileCheckLocalDirectoryHandling() {
+    // Check creating a directory using an absolute path.
+    $absolute_path = drupal_realpath($directory) . DIRECTORY_SEPARATOR . $this->randomName() . DIRECTORY_SEPARATOR . $this->randomName();
+    $this->assertTrue(drupal_mkdir($absolute_path, 0775, TRUE), 'No error reported when creating new absolute directories.', 'File');

I think we should also test that the directory actually exists... not just that no errors where reported.

+++ b/core/modules/system/lib/Drupal/system/Tests/File/DirectoryTest.phpundefined
@@ -50,6 +50,11 @@ function testFileCheckLocalDirectoryHandling() {
+    $this->assertDirectoryPermissions($absolute_path, 0775);
   }

Could we use assertFileExists() here?

https://api.drupal.org/api/drupal/core%21vendor%21phpunit%21phpunit%21PH...

assertDirectoryPermissions() checks that it exists and also checks the perms. Surely that is better?

Hm... you're right. Patch looks fine to me then.

RTBC?

RTBC.

Committed 420f38c and pushed to 8.x. Thanks!

Drupal 7 is also affected.
Backport of the fix would be nice.

I created a backport for this based on d8 function.

1. +++ b/includes/file.inc
   @@ -2409,7 +2411,66 @@ function drupal_mkdir($uri, $mode = NULL, $recursive = FALSE, $context = NULL) {
   +  }  ¶
   +  ¶
   +  // If recursive, create each missing component of the parent directory
   +  // individually and set the mode explicitly to override the umask.
   

   Trailing whitespace.

2. +++ b/includes/file.inc
   @@ -2409,7 +2411,66 @@ function drupal_mkdir($uri, $mode = NULL, $recursive = FALSE, $context = NULL) {
   +        // Not necessary to use drupal_chmod() as there is no scheme.
   +        if (!chmod($recursive_path, $mode)) {
   +          return FALSE;
   +        }
   

   Any downside if using drupal_chmod()?

3. Let's add test coverage.

I am pretty sure that D7 is not affected. Uploading a test.

Fixing test.

And works as expected.

The behavior is obvious if you look at the actual code, and the check in the test is very simple, no need to even bother committing it to core. Changing to fixed.

No, I mean exactly that, fixed.

This issue was originally for D8. The patch for D8 was committed and marked as "needs backport" for D7. But D7 is not affected, it can create directories recursively using absolute paths just fine. So there is nothing that needs to be done neither for D8, nor D7; hence, fixed.

@20th well if there's no test coverage then adding some always seems like a good idea. That said...

+++ b/modules/simpletest/tests/file.test
@@ -1208,6 +1208,22 @@ class FileUnmanagedDeleteTest extends FileTestCase {
+    $unique = tempnam(sys_get_temp_dir(), 'drupal_directory_test_');

This shouldn't be using the temporary directory for testing. It should be using the site's files directory because using the temp directory means that it won't be cleaned up if this test somehow fails.

@alexpott: got it. How about like this then.

Strange, that passes locally.

I see you have a tesbot CI error which is the same as mine in #2837008: Move scheduler.test into /tests and move three test module into /tests/modules. I started getting this problem yesterday 17 Dec 23:00 GMT.

I've raised #2837015: CI problem with permission when testing 7.x patches but there has been no response yet.

Thanks for looking in this problem, @jonathan1055!

Actually, I've had some failures because of "Permission denied" error even before Dec 16, but that might be completely unrelated. For example, check failure logs in #39. That patch passes all tests locally without problems.

The test failures in #39 are not related. Those are due to your assertions which fail due to lack of permission for the user running the tests. The 'CI error' failures we are now getting are before the tests even start to run, it is a failure of permission when trying to apply the patch (I think). You get the grey 'CI error' against the patch, instead of the pink 'pass / fail' indicator.

Hopefully someone on the testbot team will see that issue and respond. Are you on IRC or do you have any other way to alert the team? It is a fairly important fault affecting all 7.x project patches (and even core patches)

#2837015: CI problem with permission when testing 7.x patches is fixed I so have requeued the patch.

Thanks a lot, @jonathan1055!

Okay, I've fixed a couple things on the testbots permissions wise (they were kinda messy) and now realpath should not be returning FALSE.

OTOH, if you use realpath on the testbots, your going to get unexpected behavior due to the fact that they run out of a *symlink* directory called /checkout - realpath is going to get rid of that symlink and the directories are not going to match.

We run them via a symlink to simulate running drupal out of a subdirectory, However, this is less than ideal and we should probably build them in a subdirectory to begin with.

I've opened an issue here: #2838194: Run out of real subdirectory, not symlink

Looks like the behavior of realpath() is still the same and the test is still failing, so we are not quitre there yet. :)

@Mixologic, thank you very much for your help! But this issue is so minor that it is probably not worth investing any more time into it. I will keep an eye on #2838194: Run out of real subdirectory, not symlink and report there if realpath() starts behaving normally, or I find another way to test this.

For context:
To test recursive directory creation, I try to get the absolute path of public:// directory using realpath() but it returns FALSE instead. My theory is, this happens because the testbot user does not have enough permissions on some of the directories. This is a documented behavior of the realpath() function:

The running script must have executable permissions on all directories in the hierarchy, otherwise realpath() will return FALSE.