file_unmanaged_copy not working properly when the destination directory does not exist.

I had a similar issue:

• File field using the media widget
• File field has a File directory configured below the default drupal files directory
• File field supports both public (sites/sitename/files) and private (sites/sitename/private) stream wrappers

When uploading a file to this field and selecting the private stream wrapper the file will never be copied to sites/sitename/private/sub because file_unmanaged_copy doesn't support creating directories.

The issue above is also caused by the same behavior because file_unmanaged_move relies on file_unmanaged_copy.

The attached patch tells file_prepare_directory to create the directory if the destination doesn't exist.

Hi,
I tested the last patch (in comment #7) and it works for me (I was having issues when changing file destination from private to public).
Does it have a chance to be in the next release ?

I have just applied the patch in #7 to a project I am working on.

When attaching a file to a privatemsg, the file would get saved to the private directory but not the public one, as desired. The public directory did not exist, and would not get created.

The patch fixed that for me, so that the functionality does what it's expected to, and there are no errors in watchdog.

Is there some reason that code change should not be included in core?

We were having problems uploading because the files subdirectories weren't being created (media/users/[uid] in our case) because the second parameter to file_prepare_directory function was never getting passed in from file_unmanaged_copy, preventing new directories from being created. Applied the patch #7 and it corrects the issue.

This would need to go in Drupal 8 first.

Probably could use a test also?

I've attached a patch against the latest dev of Drupal 8.
Now I'll take a look at if a test is needed and what it would take to create it.

Just ran into this.

Just creating a file in public:// thrn moving it to public://foo and then a couple of asserts wpuld be enough test coverage.

Tagging RC target since this is a straightforward, backportable fix.

I'm going to do my best to follow @catch's test recommendation as soon as I can, but I haven't done anything with this since my last comment, and I'm not doing anything with it at the moment, so I'm un-assigning myself for now.

This caught my eye because I've been working on a migration file issue. The patch didn't apply, so did the reroll. And then I just keep going and added some tests.

I just added an extra test using php7 and myqsl 5.5 because in my local file_unmanaged_move is throwing errors.

Thanks for your patch quiteone.

Today I was able to give it a proper review to this patch.

In general, looks great, just a few nits:

1. +++ b/core/tests/Drupal/KernelTests/Core/File/UnmanagedCopyTest.php
   @@ -84,6 +84,14 @@ function testOverwriteSelf() {
   +    $new_filepath = file_unmanaged_copy($uri, drupal_dirname($uri) . '/sub/' . drupal_basename($uri), FILE_EXISTS_ERROR);
   

   drupal_dirname and drupal_basename are deprecated functions, can we use
   FileSystem::dirname() and FileSystem::basename() instead?

2. +++ b/core/tests/Drupal/KernelTests/Core/File/UnmanagedMoveTest.php
   @@ -38,6 +38,15 @@ function testNormal() {
   +    $newest_filepath= file_unmanaged_move($newer_filepath, $desired_filepath, FILE_EXISTS_ERROR);
   

   A missing space before the "=".

Thanks for your patch quiteone.

@gnuget, thx. Both fixed. However, note that I also changed two of the assertions to not use deprecated functions.

Looks great. this is RTBC to me.

Thanks!

We have tests nice - but I think we need a change record to inform people about the behaviour change.

1. +++ b/core/tests/Drupal/KernelTests/Core/File/UnmanagedCopyTest.php
   @@ -84,6 +84,14 @@ function testOverwriteSelf() {
   +    $this->assertTrue($new_filepath, 'Copying into same directory works.');
   
   +++ b/core/tests/Drupal/KernelTests/Core/File/UnmanagedMoveTest.php
   @@ -38,6 +38,15 @@ function testNormal() {
   +    $this->assertTrue($newest_filepath, 'Move was successful.');
   

   Seems unnecessary given file_exists(FALSE) is false...

2. +++ b/core/tests/Drupal/KernelTests/Core/File/UnmanagedCopyTest.php
   @@ -84,6 +84,14 @@ function testOverwriteSelf() {
   +    $this->assertTrue(file_exists($uri), 'Original file exists after copying onto itself.');
   +    $this->assertTrue(file_exists($new_filepath), 'Copied file exists after copying onto itself.');
   
   +++ b/core/tests/Drupal/KernelTests/Core/File/UnmanagedMoveTest.php
   @@ -38,6 +38,15 @@ function testNormal() {
   +    $this->assertTrue(file_exists($newest_filepath), 'File exists at the new location.');
   +    $this->assertFalse(file_exists($newer_filepath), 'No file remains at the old location.');
   

   PHPunit has $this->assertFileExists() and assertFileNotExists() we should use them.

Point 1 and 2 fixed in this new patch.

I will write the change record now.

Here the changed record:

https://www.drupal.org/node/2786505

(It's my first time doing change records, sorry for any mistake :D)

@gnuget, thx. I;m new to change records as well. But When I read the description in the change record the term 'missing directories are needed' is unusual, at least to me.. How about just referring to them as the parent directories (like 'man mkdir'). file_unmanaged_copy and file_unmanaged_move now make parent directories as needed. And shouldn't the 'before' sections mention that an error message was generated?

I think this is ready, the change record was already edited by AlexPott, so this just needs another review before to be RTBC. :-)

Hi @SchWalter.

Thanks for your work on this issue.

Can you please provide an interdiff? so we can see more easily what you did?

Thanks!

Here is an interdiff between the patches in #24 AND #33.

rerolled , trigger tests

This issue is a duplicate issue of these two issues:

D8:
#1068266: drupal_mkdir does not set permissions to directories it created recursively

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

The above D7 issue is marked as RTBC.

Previously I was using the patch for D7 from this issue and it was working but now I am aware of the RTBC issue, so I'm going to swap out the patch from this issue with the RTBC patch from 2789723 that already has tests and is already fully backported and ready for commit