Allow selection of which folder a file is to on the file/add form

Please consider the possibility of a default location for each filetype, with a permission to allow some roles to override that.

Maybe an alternative would be to specify a default folder to start and then allow the user to specify a subfolder within that. I think that may take care of some potential security issues.

I still really wouldn't want my users to have to see this at all. A number of UI features have been added to media/file_entity in the past few months, and I seem to be spending a lot of time finding ways to turn many of them off. It would be nice if new features started with an on/off switch for admin ;-)

I don't think that should hold back #1997208 though. That's a very simple addition and would at least cover the use case of "I just don't want them going to the root", which seems to be fairly common.

In terms of allowing choices for folders to upload to, I like the approach of an admin defining specific directories that are available, and make them available in a drop down on the upload / import form. I don't think we need to get fancy. Similar to what IMCE does, we can just have an admin form that asks for the folder names. We could validate that the folder exists.

Does that sound reasonable?

I know in one of the other issues the idea was tossed in of having defaults per file type (image, video, etc). I don't think that's a good idea. It's making an unnecessary assumption that users would want to categorize by mime type and not my some other arbitrary category

Default locations for certain file types, content types, etc. can be dealt with in other issues.

Too allay concerns about less-privileged users having access to this, I'll add a permission for it.

To start, I'll set it up as a simple text field, and then make it auto-complete once I have something working.

I got the basics working, but there is still more to do. Just posting this now so that folks can review the direction.

To do:

1. More testing.
2. Add support for multiple file uploads.
3. Nowhere in the UI does it show that the file is in a subdirectory (other than by hovering over the URL or clicking on it). We should probably show the path somewhere.
4. Having the text field auto-complete would be nice.

The first two items are done. That's all I can get done for now. If this approach makes sense, we can open follow-up issues for the remaining two items.

Added autocomplete. Also, made the permissions more consistent, and fixed a bug where single file uploads to the file system root had an extra slash.

I reviewed this patch and found it works as expected. Nice work @colan !

1. When uploading media files I can choose Upload Path.
2. Typing something new for Upload Path creates a new folder in /sites/default/files.
3. Auto-complete also working fine, showing folders created in previous uploads.

Committed to http://drupalcode.org/project/file_entity.git/commit/2c3bc73.

Boo. I didn't get commit credit even though I provided a "git am"-able patch. :(

@colan is right and I for one really appreciate the work he put into this, hopefully @aaron re-commits with attribution.

The two-step fix is to:

1. git revert COMMIT_ID
2. git am PATCH_FILE

I've done this sort of thing before too, when I've forgotten to attribute authors.

Thanks! :)

I guess you should be glad, that you did not get any attribution for this, because as far as I see it, your changes break all media internet sources like Video-URLs for the media_youtube module.

The specified file youtube://v/fSed_FPXWrI could not be copied, because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions. More information is available in the system log.

EntityMalformedException: Missing bundle property on entity of type file. in entity_extract_ids() (line 7766 of includes/common.inc).

So unfortunately your patch makes modules like media_youtube media_vimeo etc. unusable.

I hope you can take a closer look at this and fix this as soon as possible

I posted a follow up issue, which (hopefully) solves this problem well: #2454823: PHP exception after trying to add a file by URL (e.g. for the media_youtube module)

@hctom: Thanks. Putting that in a separate issue is a better idea as the original intention here was that File Entity would be used independently, especially given that this patch was already committed. Let's handle additional module integration elsewhere.

Having said that, sorry about breaking things!

The definitely isn't critical though, as it doesn't affect all users, only users that are working with the Media module.

I'm reverting this due to the issues mentioned above (not handling non-writeable files or all schemes) and not seeing the security review Dave asked for in #6.

1. +++ b/file_entity.module
   @@ -426,6 +435,9 @@ function file_entity_permission() {
   +    'choose file destination' => array(
   +      'title' => t('Select destination for each uploaded file'),
   +    ),
   

   This isn't used in determining access to the destination fieldset on the file entity edit page but should be. See file_entity_edit().

2. +++ b/file_entity.module
   @@ -2431,6 +2443,91 @@ function file_entity_match_mimetypes($needle, $haystack) {
   +    $directory = variable_get('file_public_path', conf_path() . '/files');
   +  }
   +  else /* assume 'private' */ {
   +    $directory = variable_get('file_private_path');
   +  }
   

   file_default_scheme() can be set to any writeable stream wrapper, such as Amazon S3, which would not be handled here.

   It would be better to do:

   $default_scheme = file_default_scheme();
   
   $wrapper = file_stream_wrapper_get_instance_by_scheme($default_scheme);
   
   $directory = $wrapper->getDirectoryPath();
   

3. +++ b/file_entity.module
   @@ -2431,6 +2443,91 @@ function file_entity_match_mimetypes($needle, $haystack) {
   + *   * View possible upload paths.
   + *   * Select one of them as a target when uploading one or more files.
   

   Unnecessary asterisks.

4. +++ b/file_entity.module
   @@ -2431,6 +2443,91 @@ function file_entity_match_mimetypes($needle, $haystack) {
   + *   TRUE if access is allowed.  FALSE otherwise.
   

   Extra space.

5. +++ b/file_entity.pages.inc
   @@ -112,6 +112,18 @@ function file_entity_add_upload_step_upload($form, &$form_state, array $options
   +    '#description' => t('Enter the path within the upload folder where you would like to place the file (e.g. "documents/finance").  Do not add preceding or trailing slashes.  Leave blank to keep it at the root, with no subfolders.'),
   

   Extra space.

6. +++ b/file_entity.pages.inc
   @@ -112,6 +112,18 @@ function file_entity_add_upload_step_upload($form, &$form_state, array $options
   +    '#required' => FALSE,
   

   Form items are not required by default.

7. +++ b/file_entity.pages.inc
   @@ -112,6 +112,18 @@ function file_entity_add_upload_step_upload($form, &$form_state, array $options
   +    '#access' => file_entity_upload_path_access(),
   

   This form item should not be shown for remote files (files which aren't writeable).

   file_entity_file_is_writeable()

8. +++ b/file_entity.pages.inc
   @@ -439,6 +451,28 @@ function file_entity_add_upload_submit($form, &$form_state) {
   +        // somewhere other than the root of the filesystem.  Otherwise, we'll
   

   Extra space.

9. +++ b/file_entity.pages.inc
   @@ -532,9 +571,24 @@ function file_entity_add_upload_multiple($form, &$form_state, $params = array())
   +    $upload_location = variable_get('file_default_scheme', 'public') . '://';
   

   Might as well switch to file_default_scheme() if we are touching this code anyway.

What is the status of this. I use the latest dev of the media module, media_oembed and the latest dev of this module and its dricing myself and my customers crazy that thigs are not working.

Will there be a way to set the default directory for all uploaded files? I didn't see that in the code from the patch in #9.

I just don't want all my files uploaded to the root of site/default/files. I would rather it be in a sub directory like site/default/files/media

I'm not using this patch, but I ran into an issues with the autocomplete. (On this windows machine at least), searching for "foo/bar", only "foo" is getting into $typed - because Drupal is treating it like part of the $_GET['q'] and it's being split into args by menu system.

I think you need:
$typed = implode('/', func_get_args());

And then on windows the iterator will return back slashes, so you wanna do something like this to normalise the paths coming back from the iterator.
$dir_name = str_replace('\\', '/', $iterator->getSubPathName());

I would also love to see the path matching stuff in it's own function for reusability and testing convenience (and so I can use it in my custom code).

Arguments would be:

@param $search 
  string to search on
@param $directory
  a path like public://some/custom/location/not/everyone/wants/editors/putting/files/in/the/css/aggregation/folder

@return array of matches.

I have updated the last patch to latest dev of file_entity which consequently also applies to latest release 2.0-beta2.

I then went over @DevinCarlson’s review of the code and made the appropriate fixes detailed below, with only #1 not being implemented and #7 needing review. In addition other minor code style changes were made.

1) The destination path information is only provided on the upload form and by the time file_entity_edit gets accessed when browsing a file the file is in full form. Therefore I don’t think we need a permission for an configuration that can’t be specified on the edit file screen?
2) Made the appropriate changes to leverage file_default_scheme.
3) Done
4) Done
5) Done
6) Removed ‘#required’ => false
7) Since the file isn’t loaded to leverage file_entity_file_is_writable(), I just made the form to only show on upload by checking if internet_media is empty which allowed for the form to only show on upload section. This I think isn't the write way to do this.
8) Done
9) Made the appropriate changes to leverage file_default_scheme.

A few other additions were made:

1) Code addition mentioned in https://www.drupal.org/node/2000934#comment-9733891
2) Added fix for windows paths based on last code snippet in https://www.drupal.org/node/2000934#comment-10194425
3) Brought out the path matching into its own function called file_entity_dir_match()

Security Review

I did a quick once over for security and I think the only area of concern is the moving of files and handling form input? Form input should always be sanitized when being rendered and the file moving is pretty close to how image modules handles it. Alas best to have a maintainer take a look at that.

I have also tested with different schemes, as well as media_youtube and remote stream wrappers.

Looks like there is a new test added since the other patches were reviewed: New test is: File entity upload wizard and was brought in 6e9a4067 git sha. I have managed to reproduce the error by setting:

variable_set('file_entity_file_upload_wizard_skip_file_type', TRUE);
variable_set('file_entity_file_upload_wizard_skip_scheme', TRUE);
variable_set('file_entity_file_upload_wizard_skip_fields', TRUE);

and then adding an image. If i remove the added $form['path'] from file_entity_add_upload_step_upload the notice disappears.

Thanks for splitting out that function sylus!

I think the $form['path'] is conflicting with later additions via path / pathauto and causing the issue with simpletest.

So renamed to form['upload_path'] since it is technically tied to that functionality and then on path_file_update() there won't be issues about path being a string instead of either not present or array.

Functionality all still works as expected.

Better commit header.

I can confirm that the patch in #30 provides the very same functionality as #9

From Dave's original post:

But I think it would be much more desired that the user wants to specify where the file should be uploaded when they upload the file.

If this patch is ever committed, it should only be available if enabled through a configuration option. We have several clients that don't even want to think about this... they want the system to be smart enough to put the file in the right location for them.

My preference would be to have a "File directory" field for each File Entity, similar to what exists for Image fields. We would then be able to do creative things with tokens to ensure each file is placed in its proper location. This could also serve as a default location for a file field if no file directory value is set on Image fields, Video fields, etc.

As a follow-up, it seems as though the File Entity Paths module is attempting to accomplish what I mentioned in #32: https://www.drupal.org/project/fe_paths. Might be worthwhile to consider incorporating the capability into File Entity.

@sylus patch 30 is great, thanks! Been using it in production for quite a while and built some custom functionality based on it (depending on file type the default folder gets set using some custom form validation I wrote in a custom module).

Very helpful thanks, appreciated.

RTBC patch 30 , very good, thanks to @colan as well for his work on this.

Small improvement in patch 35. Patch 30 (perhaps when used in conjunction with media_bulk_upload although I do not really care what the situation was) could sometimes throw noticed in watchdog. Not a big deal but its fixed in this patch. See the interdiff_30_to_35.txt and the new patch 35

To re-iterate, elaborate:
This functionality should be added because:

• The functionality works well.
• There's people requesting support for this in the media queue however its not 'media' related, it needs to be done in file_entity
• I'm using it to provide upload functionality where image type selection is coupled with destination folder.

To alleviate any concerns that one might have by adding this functionality, it could be made optional or disabled by default, however I am of the opinion that it should be enabled by default.

Suggest using this patch in conjunction with these patches as well:
#2198973: Update File Entity + Fix Features Override
#2318519: How do I select a file type on /file/add with plupload enabled?
#2846795: fatal error argument 3 must be array - when using custom file/add/path with plupload and media_bulk_upload

soon, going to add a new branch and put this on 7.x-3.x.

I just tested this patch against the current latest release 7.x-2.1 and it's still working great.

I'll try to review this patch again, if it is an optional configuration (disabled by default) then it should be ok for everyone.

TODO: test this with multiform and media_bulk_upload , afaik we may have to add some code to get this functionality to work for multi bulk uploads.

Hi created taxonomy for media folders and included that field in file types .
And i applied this patch working fine.
file_entity_upload_specify_path-2000934-7.patch.

@sri@re
interesting concept but out of scope for this issue, could you please open a new issue and put your patch in there instead and refer to that issue here?
Thanks.

disabled failed patch, setting back to needs review for patch 35

Hi, I am not coding expert, but I found the idea (patch #35) very interesting and have been using it in several projects; somehow after upgrading to 7.x-2.1 I am kind of lost in following instructions till now failed to apply the same patch to the recent release. did I miss something by applying the patch, since the code already was there! Thanks for your advice.

Thank you. ** ReSolved ** by applying new patch.

#35 isn't working with plupload. Directory is never set because
$form_state['values']['path'] needs to be
$form_state['values']['upload_path'] to work as expected.

Patch no longer applies cleanly to dev branch so I had to roll a new one.
My gitfu isn't strong enough to whip up an interdiff with a patch that won't apply.

Here's a version that should apply cleanly to current stable release (7.x-2.12) but won't apply to dev.

@jamesrward nicely done, thanks for the enhancement.

wondering, for those not using plupload , would patch 45 or patch 44 still work?

maybe need to check isset path and isset path_upload

food for thought. would be nice for this to work for everyone, no matter what.

@josepholstad you are correct. #45 doesn't work without plupload. It works well with plupload, however, on both dev and 2.15 branch.
Also, #35 still works without plupload on both dev and 2.15 branch.

Rolling #35 and #45 patches into one, now, like you suggested.

Tested with:
drupal 7.56

with and without:
plupload 7.x-1.7
multiform 7.x-1.4

Re-rolled #49 ("...48.patch") to fix commit author and corresponding comment number.

Rats. I accidentally snuck some irrelevant code into my patch. The following isn't supposed to be there:

@@ -897,6 +955,11 @@ function file_entity_edit_submit($form, &$form_state) {
     }
   }
 
+  if(!empty($file->field_folder) && empty($form_state['values']['replace_upload'])){
+    $tname = taxonomy_term_load($file->field_folder['und'][0]['tid']);
+    $file = file_move($file,$form_state['values']['scheme'] . '://'.$tname->name,FILE_EXISTS_RENAME);
+  }
+

Re-rolling. 3X is a charm, right?

Re-tested dev branch with:
drupal 7.56

with and without:
plupload 7.x-1.7
multiform 7.x-1.4

evaluating patch #52 for possibly the upcomming release. 7.x-2.26

#3019478: Plan for File Entity 7.x-2.26 release

Reroll against 7.x-2.37