Icky PHP warnings occur when private filesystem path cannot be created

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.

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 .

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

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

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

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?

Hmm, hidden files aren't tested?

Trying again.

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.

Great, thanks @jhedstrom.

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

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

Patch in #15 still applies.

DirectoryTest passes locally.

Retesting, and also here's a review:

+++ b/core/modules/system/src/Tests/File/DirectoryTest.php
@@ -51,6 +51,13 @@ function testFileCheckLocalDirectoryHandling() {
+    @drupal_chmod($parent_path, 0444);

...

+    $this->assertFalse(@drupal_mkdir($second_child_path, 0775, TRUE), t('Creating child directory under a non writable parent fails.'), 'File');


+++ b/core/modules/system/system.module
@@ -801,7 +801,7 @@ function system_check_directory($form_element, FormStateInterface $form_state) {
+  if (!is_dir($directory) && !@drupal_mkdir($directory, NULL, TRUE)) {

Should use the file_system service.

Here's the changes as requested by @Mile23 in #16.

I would be nice to be able to pass a silent as parameter to the method ... Is there a way to check beforehand whether a mkdir would fail?

@dawehner It doesn't look like there is a way to check before hand if php's mkdir() will fail. As @alexpott noted in #14 LocalStream::mkdir() does call drupal_mkdir() if STREAM_REPORT_ERRORS is set (and calls @drupal_mkdir() if it is not). However I think properly implementing a silent param in FileSystem::mkdir() would be best as a follow up.

#18 fixes this bug and adds a test to prove it.

Shouldn't the patch be doing the silencing inside the service on the actual PHP call to mkdir?

This is not reproducible in Drupal 8 because The private files directory is no longer configurable in the UI or available in config. However, it is reproducible in Drupal 7, so changing version.