file_munge_filename and use a token and make sure files are uploaded to the temp dir

FWIW, consider using the original input filename, less its file extension for the Node Title. This way the user will get node titles that match what they labeled their image files.

I did a hack to include the '-' in the clean file name, so that multi-part names would be identifiable in the node title. Also, I stripped the file extension from $file_name and created $file_title to provide the $form_state_value 'title' =>. Now I get reasonably sound node titles that I don't have to go back in and edit.

Whoa, never mind, looks like you already addressed the node title problem. Sorry, should have read on.

+ // @todo use a temporary filename.

For this, I could create a variable (unless Drupal has one) for a place to assemble multi-part file uploads.

It probably doesn't have one, so you'd need to define one. Instead of relying on tempnam() which is only going to give you something in /tmp - which is not safe for load balanced setups.

This would be a much more graceful way of allowing site admins to define a safe place (for example, mine is over an NFS filesystem) - so I could use something like /home/myuser/tmp)

(The variable could be defaulted to /tmp, though)

$dir = variable_get('plupload_temp_dir');
if(!is_dir($dir) || !is_writeable($dir)) {
   ...return an error...
}

...keep going...

we could rely on ini_get('upload_tmp_dir') and just rename the file there
- this directive is also one that sysadmins should be aware of for load balanced setups

I'd still like to see a Drupal variable for the temp dir and not rely on PHP config.

This gives a lot more flexibility in general (if you don't have access to modify it or don't want to look at how to alter the ini setting in a module before this has a chance to execute, etc.)

The core already has a function for the temp dir and it already is in this patch.

http://api.drupal.org/api/drupal/includes--file.inc/function/file_direct...

To recap what still needs to be done with this patch, exactly?

This is the last stone hindering #919494: create 6.x-1.0-beta1 release (and mark it supported).

I would like to test out a new patch (cannot code) to help this project moving forward.

I spoke with greggles on IRC a bit. Basically, his goal is that we deal with files uploaded via plupload the same as core. If we do that, the patch is likely to be accepted. I may have some time to look into this later this week, maybe next week. I'm pretty swamped with alpha testing of Node Gallery atm, so if anyone else is available to take a crack at it, I'd appreciate it.

I had a look at this. Creating a completely random name for the temporary file will lead to field_file_save_file() complain about the file extension. I'll have a look if carrying over the original extension is enough.

Here is a proposal.

And by the way, the $user object won't be available sometimes, e.g. if the flash object is not granted access to cookies. Other bulk uploaders circumvent this by using generated keys on the upload form, and checking these in the callback. I didn't see anything like that here. Perhaps that should be added.

There is an error in the above patch. It hardcodes the input filename to the temporary file instead of using $input_filename. I'll reroll shortly.

Fixed the above issue.

The patch above looks like it crosses paths with the commit for #998594: Typo in hook_perm prevents access to admin UI, applying this patch will undo the fix.

The other big issue I found in reviewing, is that before applying the patch, when I uploaded an image that had parens '()' in the filename, the upload stripped them and went on with life. After applying the patch, the uploader reports success, but the nodes that correspond to the image with the parens and all images uploaded after said image in the queue are not created.

Can't reproduce this. Can you give me an exact filename?

btw: The permission change was in the original patches, I just included it here because I thought it was intended.

Filename that was causing it here was IMG_1387(2).JPG

Ah, I see the problems now. I misunderstood some things. I need to take a closer look at what exactly is passed as $_REQUEST["name"].

What a mess, sorry.

Whoops, looks like this got left in 'needs work' when it needed to be in 'needs review'.

I've actually been running the patch from #22 in my development install since the day it was posted, and I haven't found any issues with it. However, I'd like to get a confirmation from @greggles that this was what he was shooting for before I commit, so I'm going to leave this as 'needs review' for him.

In summary, I'm good with it, just want @greggles to OK it before I commit.

Jeez - I don't know what patch I had installed, but every time I apply the patch from #22, it severely breaks things. I did however take the time to make the comment cleanup that @greggles was talking about, and I'm attaching the (known broken) patch here.

@scroogie, I keep on getting messages about "For security reasons, your upload has been renamed to Blob6189935a2e3542a8b857fab801252115.." and the node isn't actually created. I don't know the internals of this patch, so I'll leave it to you to take a crack at first. If you are busy or need help, just let me know and I'll dig into it.

I did confirm with @greggles that once we get this one closed out, we can tag and release a 6.x-1.x version.

Subscribe

+    // This replaces all non-word chars (excluding dots and dashes) in the filename with empty string.
+    $filename = preg_replace('/[^\w\._-]+/', '', $_REQUEST["name"]);

Note that in the D7 version of the module, we're forcing the tempoary filename to always end in ".tmp" and have no other dots in it, and consequently using a more stringent regexp when checking it: preg_match('/^\w+\.tmp$/', $file_name).

If you did that here, you could probably do without the file_munge_filename(), allowed extensions, and related code, since those things would already be prevented. That would allow the code to be a bit simpler.

However, I'm not sure if that's possible with the way the D6 module works or not.

@scroogie, I keep on getting messages about "For security reasons, your upload has been renamed to Blob6189935a2e3542a8b857fab801252115.." and the node isn't actually created.

I'm quite sure that you had the patch from #2 installed then, which uses a random filename. That approach doesn't make sense in my opinion, thats why I dropped it.