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.

Files: 
CommentFileSizeAuthor
#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 ]

Comments

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
StatusFileSize
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

StatusFileSize
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

StatusFileSize
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:

    <?php
    $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.

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