Problem/Motivation

When attempting to create a private file folder, I put in an invalid path and get one of two errors:

When making a directory that cannot be created by the webuser on OSX in the root dir
IE "/home/a/dev/files-private" (OSX has a home directory not writeable by webuser)
"Operation not supported in _drupal_mkdir_call()"

When making a directory that cannot be created due to permissions (OSX, Linux, Windows)
"Permission Denied in _drupal_mkdir_call()"

Followed by the correct error:
"The directory /home/i/dev/files-private does not exist and could not be created."

Screenshot_10_6_13_12_01_PM.png

Proposed resolution

To use @mkdir() instead of mkdir().

Remaining tasks

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug
Unfrozen changes Unfrozen because it only changes what error messages are revealed to the end user
Prioritized changes The main goal of this issue is usability
Files: 
CommentFileSizeAuthor
#15 2105949-15-silence_mkdir_errors.patch1.94 KBfenstrat
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,002 pass(es).
[ View ]
#10 2105949-7-silence_mkdir_errors-test-only.patch1.11 KBfenstrat
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,076 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
#7 2105949-7-silence_mkdir_errors.patch1.67 KBfenstrat
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,080 pass(es).
[ View ]
Screenshot_10_6_13_12_01_PM.png238.48 KBjaperry

Comments

fenstrat’s picture

Status:Active» Needs review
StatusFileSize
new576 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,818 pass(es).
[ View ]

This patch gets rid of the icky error, but keeps the friendly "The directory %directory does not exist and could not be created." error.

Sitting here at the table with @chx at PNWDS who okayed this approach. He also waxed quite lyrically about how PHP's handling of errors like this is ... far from ideal.

fenstrat’s picture

The fact that this passes suggests we need a test for this.

Also related #1068266: drupal_mkdir does not set permissions to directories it created recursively .

jhedstrom’s picture

Issue summary:View changes
Status:Needs review» Needs work
Issue tags:+Needs tests

Needs tests as per #2. Patch still applies, and the approach makes sense to me as well.

fenstrat’s picture

@jhedstrom Any pointers on how'd you go about writing a test for this? Happy to give it a shot, but not sure of the best approach.

jhedstrom’s picture

@fenstrat, take a look at \Drupal\system\Tests\File\DirectoryTest -- that is testing drupal_mkdir(). I'd guess you could create a directory, remove access to that directory, then call drupal_mkdir() and ensure the icky errors don't appear?

areke’s picture

Issue summary:View changes
fenstrat’s picture

Status:Needs work» Needs review
StatusFileSize
new1.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,080 pass(es).
[ View ]
new1.11 KB

Thanks @jhedstrom, here's a shot at that. The test-only patch should fail, but with an exception as mkdir() isn't silenced with @mkdir.

As DirectoryTest is a unit test (it extends from DrupalUnitTestBase (soon to be KernelTestBase I believe) and not WebTestBase) I'm not quite sure how to test the output of php warnings. So while the test will cause an exception if mkdir isn't silenced I'm not sure how to actually test there is no icky PHP warning?

fenstrat’s picture

Hmm, hidden files aren't tested?

fenstrat’s picture

StatusFileSize
new1.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,076 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

Trying again.

Status:Needs review» Needs work

The last submitted patch, 10: 2105949-7-silence_mkdir_errors-test-only.patch, failed testing.

jhedstrom’s picture

Issue summary:View changes
Status:Needs work» Reviewed & tested by the community

I think failure by exception is a fine way to test. #7 is RTBC I think (#11 is just the test, which is expected to fail without the fix).

I've added the beta phase evaluation to the summary.

fenstrat’s picture

Issue tags:-Needs tests

Great, thanks @jhedstrom.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

I think we should be adding the @ to the call to drupal_mkdir() in system_check_directory()

We rely on the errors in LocalStream::mkdir() - which is obviously missing test coverage.

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

With this change this logic is broken.

And we do this elsewhere.

  if (@drupal_mkdir($file, $mod)) {
    return TRUE;
  }
  else {
    return FALSE;
  }

From drupal_install_mkdir()

    // Let mkdir() recursively create directories and use the default directory
    // permissions.
    if ($options & FILE_CREATE_DIRECTORY) {
      return @drupal_mkdir($directory, NULL, TRUE);
    }

From file_prepare_directory().

fenstrat’s picture

Status:Needs work» Needs review
StatusFileSize
new1.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,002 pass(es).
[ View ]

@alexpott in #14 is correct, the errors are relied upon in other locations. I've moved the @ into system_check_directory().

I've kept the test, and silence the call there, i.e. @drupal_mkdir().

Also, while #2050759: Move drupal_chmod and other code in file.inc to a Drupal\Core\File\FileSystem class deprecated drupal_mkdir() there are still calls all over core to it.