Support from Acquia helps fund testing for Drupal Acquia logo

Comments

breathingrock’s picture

Patch with fix.

slashrsm’s picture

Version: 7.x-1.7 » 7.x-1.x-dev
Status: Active » Needs work

Hm... is it even meant to set plupload_temporary_uri with trailing slash? Aren't URI's in Drupal usually w/o it ($base_url, module_get_path(), ...)?

jamesrward’s picture

Actually it appears that since there is never a slash at the end of $temp_directory files are always uploaded to the wrong directory when plupload_temporary_uri is set. When the slash is there the process fails but when the slash isn't there it completes fine after using the wrong directory.

For example if we set plupload_temporary_uri to /myfiles/tmp the temporary files will all be created in /myfiles with tmp at the start of the filename.

There are two issues at work here. The first is that temp files aren't being written where we expect due to this line appending the filename right on the end of the directory name:

$out = fopen($temp_directory . $file_name, $chunk == 0 ? "wb" : "ab");

The second issue and the reason it fails when plupload_temporary_uri has a trailing slash is this line:
$files[$i]['tmppath'] = variable_get('plupload_temporary_uri', 'temporary://') . $value;

This version of plupload_temporary_uri isn't run through file_prepare_directory first so it will retain it's trailing slash and no longer match where the temp file was written.

Assuming a filename of myfile.jpg we would end up with a temporary file with the path /myfiles/tmpRandomString.tmp and the copy command would be looking for it at /myfiles/tmp/RandomString.tmp. Without the trailing slash it will work but the temp files are being written in /myfiles instead of /myfiles/tmp

To see this just set plupload_temporary_uri and look at the folder above the uri you provide after clicking start upload and before clicking the submit/next button.

In my testing adding the slash into the fopen calls works with and without trailing slashes and fixes the incorrect directory issue as well as working fine when plupload_temporary_uri is not set.

jamesrward’s picture

Status: Needs work » Needs review
FileSize
1.01 KB

Rolled against 7.x-2.x on the first one. This one is rolled against 7.x-1.x.

sam.spinoy@gmail.com’s picture

I've tested the latest patch against the 7-1.7 release. I have the plupload_temporary_uri variable set. While the patch does fix the "temp files being uploaded to the wrong directory" problem, it now actually fails when trying to save the images. Gives me an error message like "cannot copy temp file your/temp/dirp1dshdskjhdskd.tmp to its final destination". Adding a trailing slash to the plupload_temporary_uri variable solved the problem for me.

jamesrward’s picture

Thanks @samspinoy I fixed that locally and forgot to include in my patch. With these changes spanning multiple locations I'm starting to think we should DRY this up and drop the ternary in favour of a consistent function for preparing the directory. I'll re-roll the patch with the quick fix first then take a crack at a new function. I want to look around at how some other modules handle this issue first.

jamesrward’s picture

Patch and interdiff. Tested this with the no plupload_temporary_uri variable set and a plupload_temporary_uri with and without a trailing slash. It feels reckless to always add the slash but it doesn't fail under any circumstances on my machine (macOS). Would love some feedback from a windows user on this.

generalredneck’s picture

Status: Needs review » Reviewed & tested by the community

I can vouche this has been working for us for a while.