We have issues that want a 'default' location for upload:
#1563886: Ability to set default file destination folder when using "Add file"
#1997208: Specify a fixed custom file directory for use with the /file/add form

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. Something like a tree-widget of writeable folders, or a simple textfield that autocompletes with the folder of the selected file system.

#30 allow_selection_of-2000934-30.patch9.09 KBsylus
PASSED: [[SimpleTest]]: [MySQL] 860 pass(es).
[ View ]
#29 allow_selection_of-2000934-29.patch9.04 KBsylus
PASSED: [[SimpleTest]]: [MySQL] 860 pass(es).
[ View ]
#25 allow_selection_of-2000934-25.patch9.07 KBsylus
FAILED: [[SimpleTest]]: [MySQL] 860 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#9 file_entity_upload_specify_path-2000934-9.patch8.6 KBcolan
PASSED: [[SimpleTest]]: [MySQL] 791 pass(es).
[ View ]
#8 file_entity_upload_specify_path-2000934-8.patch5.22 KBcolan
PASSED: [[SimpleTest]]: [MySQL] 882 pass(es).
[ View ]
#7 file_entity_upload_specify_path-2000934-7.patch3.16 KBcolan
PASSED: [[SimpleTest]]: [MySQL] 882 pass(es).
[ View ]


John Pitcairn’s picture

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

aaron’s picture

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.

John Pitcairn’s picture

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 ;-)

bkosborne’s picture

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

colan’s picture

Assigned:Unassigned» colan
Issue summary:View changes

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.

Dave Reid’s picture

Issue tags:+needs security review

Not to fellow maintainers: this needs a full security review before this can be committed.

colan’s picture

Status:Active» Needs review
new3.16 KB
PASSED: [[SimpleTest]]: [MySQL] 882 pass(es).
[ View ]

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.
colan’s picture

new5.22 KB
PASSED: [[SimpleTest]]: [MySQL] 882 pass(es).
[ View ]

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.

colan’s picture

new8.6 KB
PASSED: [[SimpleTest]]: [MySQL] 791 pass(es).
[ View ]

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.

gdaw’s picture

Status:Needs review» Reviewed & tested by the community

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.

  • aaron committed 2c3bc73 on 7.x-2.x
    Issue #2000934 by colan: fixed Allow selection of which folder a file is...
aaron’s picture

Status:Reviewed & tested by the community» Fixed
colan’s picture

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

gdaw’s picture

Status:Fixed» Needs work

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

colan’s picture

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! :)

hctom’s picture

Priority:Normal» Critical

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

hctom’s picture

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)

colan’s picture

Priority:Critical» Major

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

Devin Carlson’s picture

Priority:Major» Normal

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


  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.

  • Devin Carlson committed 54a45a3 on 7.x-2.x
    Revert "Issue #2000934 by colan: fixed Allow selection of which folder a...
cmriley’s picture

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.

gmclelland’s picture

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

sime’s picture

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

sime’s picture

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.
sylus’s picture

Status:Needs work» Needs review
new9.07 KB
FAILED: [[SimpleTest]]: [MySQL] 860 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 25: allow_selection_of-2000934-25.patch, failed testing.

sylus’s picture

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.

sime’s picture

Thanks for splitting out that function sylus!

sylus’s picture

Status:Needs work» Needs review
new9.04 KB
PASSED: [[SimpleTest]]: [MySQL] 860 pass(es).
[ View ]

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.

sylus’s picture

new9.09 KB
PASSED: [[SimpleTest]]: [MySQL] 860 pass(es).
[ View ]

Better commit header.