[drupal-devel] [task] Enhanced upload administration
Junyor
drupal-devel at drupal.org
Tue Sep 6 21:34:16 UTC 2005
Issue status update for
http://drupal.org/node/25756
Post a follow up:
http://drupal.org/project/comments/add/25756
Project: Drupal
Version: cvs
Component: upload.module
Category: tasks
Priority: normal
Assigned to: Junyor
Reported by: Junyor
Updated by: Junyor
-Status: patch (ready to be committed)
+Status: patch (code needs work)
Changes are needed after the revision patch. I'm going to have to
rethink upload_delete() a bit.
Junyor
Previous comments:
------------------------------------------------------------------------
Sat, 25 Jun 2005 22:43:40 +0000 : Junyor
Attachment: http://drupal.org/files/issues/drupal-4-6.upload.junyor.patch (4.84 KB)
I've found the need to have some administration for file attachments.
Here's a demo patch of some of the proposed added functionality. This
functionality should easily allow users to get a list of the files
they've uploaded, so they can get rid of old files if they reach their
size limit.
Changes:
- Added an 'administer uploaded files' permission
- Added admin/uploads and 'my uploads', which list uploaded files
Planned changes:
- statistics for the 'my uploads' and admin/uploads pages with total
size of uploaded files and size limit
- functionality to delete/rename files (using checkboxes to select
multiple files)
Please comment on code quality, current functionality, and planned
functionality. Thanks.
Patch applies to 4.6 and HEAD.
------------------------------------------------------------------------
Sat, 25 Jun 2005 23:57:07 +0000 : clydefrog
Sounds like a good idea from your description. I haven't tested, but a
quick glance at the patch shows an odd comment:
+/*
+ * Menu callback:
+ * Pages where users add and delete their subscriptions to nodes
+ */
------------------------------------------------------------------------
Sun, 26 Jun 2005 08:52:50 +0000 : Junyor
Oops. That's what I get for copying code. I'll fix that in the next
patch.
------------------------------------------------------------------------
Tue, 19 Jul 2005 04:12:14 +0000 : moshe weitzman
Looks useful to me. If I had more uploads on my dev site I would try it
out.
Some code comments:
- don't reuse upload_page() for both user and admin page. split off
admin stuff into a new menu callback which points to a new
upload_admin_page() function.
- in the query for admin page, don't directly use the UID when building
the $sql. instead, pass that as an argument at end of pager_query().
this protects against SQL injection attack
- do we really want to show file locations to regular users? i think
not.
- perhaps show node title in the file listing tables
your proposed enhancements look swell to me.
related question: do attachments on unpublished nodes acount against
user quota? if so, user is powerless to delete them? this module could
fix that I think. when unpublished, i suggest showing node title
without hyperlink.
------------------------------------------------------------------------
Tue, 19 Jul 2005 05:21:09 +0000 : Junyor
Thanks for the feedback, Moshe! I haven't worked on this in a couple of
weeks, but I'll try to get back to it soon and I'll add in your
feedback. Moving back to active so it doesn't clog up the patch queue
any longer.
------------------------------------------------------------------------
Thu, 21 Jul 2005 11:43:16 +0000 : Junyor
Moshe: When you said the following:
- do we really want to show file locations to regular users? i think
not.
- perhaps show node title in the file listing tables
what did you mean? The "Location" column is the node title where the
file is attached. What file location is being shown to users?
------------------------------------------------------------------------
Thu, 21 Jul 2005 13:32:06 +0000 : moshe weitzman
yeah, i think we are ok on file location .... you might consider
releasing this as a contrib module if core is uninterested.
------------------------------------------------------------------------
Sun, 24 Jul 2005 21:26:39 +0000 : Junyor
Attachment: http://drupal.org/files/issues/drupal-4-6.junyor.upload2.patch (11.09 KB)
Here's a new update. Still not ready for commit, but much further
along. Developed against 4.6, but patch applies to CVS with some fuzz.
Uploading files didn't seem to work for me in CVS, so I couldn't test
whether the patch worked.
When the time comes, I hope this will be considered for core, as it
seems like missing functionality to upload.module now.
Changes since last patch:
* Split upload_page into upload_page and upload_admin
* Added 'delete all selected files' operation on admin/upload and
upload pages
* Fixed upload_admin query to avoid SQL code injection (thanks, Moshe)
* Renamed "Location" column to "Title"
* Don't show unpublished nodes as links in the "Title" column
* Fixed bugs so that you can actually delete files now ;)
To Do:
* statistics for the 'my uploads' and admin/uploads pages with total
size of uploaded files and size limit
* figure out why status messages remain across page loads
* get Operations column working
Questions:
* What operations should go in the Operations column, anyway?
* What other operations should be added to the mass-operations
drop-down?
------------------------------------------------------------------------
Mon, 25 Jul 2005 14:39:22 +0000 : Junyor
Attachment: http://drupal.org/files/issues/drupal-4-6.junyor.upload3.patch (15.62 KB)
New patch.
Changes since last patch:
* Added basic statistical information to /admin/upload and /upload
* Fixed problem getting correct username on /admin/upload
------------------------------------------------------------------------
Mon, 25 Jul 2005 15:01:09 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/screenshot.gif (16.29 KB)
Hey Junyor, awesome patch!
I've recently wrote my own PHP based file management script that does
very similar (but not in a Drupal enivorment). I've attached a screen
shot of the interface.
As for operations, I'd love to see the basics like what I have. Copy
(file or folder), Move (file or folder), Zip (file or folder) for
download, and obviously delete, upload, and create folder :) Also, I
have a function that cleans file names too, replacing spaces and weird
punctucation with a certain character (right now just a _) to avoid
conflicts on Windows based machines. This feature could obviously be
enabled or turned off.
I researched this quite a bit and I've found some top notch code for
all of these operations, all open source as well. Additionally, all of
the icons are from this set that was released as open source as well so
we could include those too.
I'd love to contribute and work with you to further extend this module.
Having solid file management features would be great in Drupal.
Junyor, just drop me a line through the Drupal contact and I'd be glad
to discuss things and aid in the development of this!!
------------------------------------------------------------------------
Mon, 25 Jul 2005 15:12:13 +0000 : Junyor
I think the changes you want are outside the scope of upload.module, let
alone this patch. It's probably better to work on a new contrib module
with the functionality you're referring to. FWIW, file.inc already has
some functions you could hook into. Good luck!
------------------------------------------------------------------------
Fri, 19 Aug 2005 12:44:04 +0000 : Junyor
Attachment: http://drupal.org/files/issues/drupal-4-6.junyor.upload4.patch (16.22 KB)
Updated patch for CVS. Removed operation column, as I couldn't find a
way to get the delete link to work and the checkbox will work just as
well anyway. Please review. I'd like to get this into 4.7.
------------------------------------------------------------------------
Mon, 29 Aug 2005 20:15:00 +0000 : m3avrck
Looking good but needs some work.
The link to each file that has been upload is malformed:
http://localhost/drupal/?q=http://localhost/drupal/files/upload.module
Some reason that is absolute and it shouldn't be, looks like line 280
is the problem there. 'Page not found' with dirty URLs. However, with
clean URLS, I get 403 forbidden "access denied" to this link:
http://localhost/drupal/files/upload.module
Is there a way to set a limit on total file size per user? When I goto
"my uploads" it says limit per file and per directory is 1mb yet I
don't see any way to set these in Settings > Uploads. Also, shouldn't
you be able to set different limits for each user as well?
Additionally, under "my uploads" it says allowable extensions but I
don't see anywhere to define those as well?
Under latest HEAD with Drupal running on PHP 5.0.3 I get the following
warnings:
warning: array_merge() [function.array-merge]: Argument #1 is not an
array in \modules\upload.module on line 124.
warning: array_flip() [function.array-flip]: The argument should be an
array in \modules\upload.module on line 133.
warning: implode() [function.implode]: Bad arguments. in
\modules\upload.module on line 139.
Seems all of these warnings are being caused by the extensions variable
I pointed out... not defined correctly, and this *should* be
configurable option in Settings > Uploads ... not hardcoded like it is
in the module.
Please let me know if I can help, I think we can get this into core, it
is definetly needed!
------------------------------------------------------------------------
Mon, 29 Aug 2005 20:33:10 +0000 : m3avrck
Ok turns out if file dowloads are set to "Private" the links work fine.
If they are "Public" over HTTP, they break, as noted above.
Now going along with this, it would be great if an Admin could set
permissions for each of these files for viewing... e.g., if files are
set up to be "Private" to assign which roles can actually view or
download these files or to keep them anonymous. I think this would be
bring the file management features full circle and finally be up to par
with this :)
------------------------------------------------------------------------
Mon, 29 Aug 2005 21:31:39 +0000 : Junyor
Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload5.patch (21.12 KB)
OK, I fixed the problem with the download URLs. It seems like
upload.module doesn't use l() elsewhere, so I used the same syntax. I
think I fixed the warnings you got from PHP5, but I don't have it
around to test with. Also, I fixed some identation and tab problems.
I also updated for current HEAD (there was on reject and lots of fuzz).
Give a specific role the "upload files" permission and you'll be able
to configure max file size and extensions. Setting permissions per
file or for private vs. public is outside the scope of this patch. I
believe there is already an issue for an overhaul of the file system,
though.
------------------------------------------------------------------------
Mon, 29 Aug 2005 22:06:00 +0000 : m3avrck
Ok looks like the links for public files are formed correctly now.
However, I am getting a "Forbidden" access when trying to view the file
like: http://localhost/drupal/files/somefile.txt ... is that a problem
with Drupal or the module? I checked .htaccess and don't see it
restricting files, hmmm.
All of the warnings have been fixed and are working correctly now with
PHP 5.
Now you say "Give a specific role the 'upload files' permission and
you'll be able to configure max file size and extensions." What if I
don't assign this permission to anyone? In that case, UID=1 logged in
can only upload files up to 1MB in size and has a total of 1MB of
space. Shouldn't Upload > Settings have a default setting then that
takes precedence? For instance, go in there and set defaults to 25MB
for each. Then logged in as UID=1, I can upload files within those
limits. If I then assign another role this permission, they should
inherit "this default" instead of having it hard coded as 1MB and
having to change again. Does this make sense? From a usability
perspective this was how I *expected* it to work but didn't see what
was going on till you clarified :)
For the extensions list, I would make the code more robust. I can throw
in as many spaces as I want and it is saved with them intact. I would
think taking a more standard route of defining extensions like:
html,php,txt,doc with a common character seperating extensions, easier
to read too especially on a long list :). Then you can just explode(',')
and trim() each term, preventing managled extensions from being entered
in the database.
Any link or updates with that overhaul of the filesystem? I'd be
interested in helping out, who is spearheading that?
------------------------------------------------------------------------
Mon, 29 Aug 2005 22:14:25 +0000 : m3avrck
Oh had one more idea... shouldn't there be a global total size limit?
As logged in as UID=1 and I'm setting up my website, I should be able
to specify a total size for all files with this Drupal install...
either 0 for unlimited (which I believe it is now) or enter some
number.
Then when I assign different roles the ability to adminster uploads,
they can set file size limits too but all of these limits come under
one giant umbrella limit that that main admin sets up. Otherwise I
could see some problems with authenticated users assigning limits *too*
big. Need some sort of check there :)
Sorry for the mass amount of critiques, just want to get this patch
squared away as perfect as possible. It's really important to get into
core so gotta make sure we cover all of our bases!
------------------------------------------------------------------------
Tue, 30 Aug 2005 05:55:48 +0000 : Junyor
Does the forbidden file problem go away if you create a role that has
the 'upload files' permission?
The new file handling was discussed (briefly) on drupal-devel:
http://lists.drupal.org/archives/drupal-devel/2005-08/msg00473.html.
Improving settings for the upload.module is a fine idea, but one
outside the scope of this patch. Feel free to create new issues and
submit patches for them.
Thank you again for your help!
------------------------------------------------------------------------
Tue, 30 Aug 2005 14:09:12 +0000 : m3avrck
Well I shouldn't get 'file forbidden' logged in as UID=1 but I do.
However, I have 'authenticated user' set with all of the upload
permissions and I still get that error when I try to download a file
with Drupal set to 'public' files as a setting.
As for cleaning up the settings, I do believe that is part of this
patch and definetly within the scope :) In terms of usability, these
settings should be moved around a bit. However, in terms of the code
freeze, I believe this patch is ready but it does need a little bit of
tweaking before release.
------------------------------------------------------------------------
Tue, 30 Aug 2005 14:32:20 +0000 : m3avrck
Patch ready to be committed! Tested and works great. Forbidden file
problem and settings problem are do to inadequacies wit the current
Drupal file system, not this patch! More patches needed :)
More information about the drupal-devel
mailing list