Issue status update for http://drupal.org/node/25756 Project: Drupal Version: cvs Component: upload.module Category: tasks Priority: normal Assigned to: Junyor Reported by: Junyor Updated by: Junyor Status: patch Oops. That's what I get for copying code. I'll fix that in the next patch. Junyor Previous comments: ------------------------------------------------------------------------ June 25, 2005 - 23:43 : 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. ------------------------------------------------------------------------ June 26, 2005 - 00:57 : 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 + */

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: active +Status: patch 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? 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.

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: m3avrck Status: patch 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!! m3avrck 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

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 (code needs review) 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. 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!

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: m3avrck Status: patch (code needs review) 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 :) m3avrck 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!

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: m3avrck Status: patch (code needs review) 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? m3avrck 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.

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 (code needs review) 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! 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!

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: m3avrck -Status: patch (code needs review) +Status: patch (ready to be committed) 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 :) m3avrck 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.

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 (code needs work) +Status: patch (code needs review) Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload6.patch (23.58 KB) Updated patch to HEAD. I've rolled in the changes from http://drupal.org/node/30025, as they intermingle. I also removed some extraneous whitespace at the end of lines. I'm still hoping to get this into 4.7. 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 :) ------------------------------------------------------------------------ Tue, 06 Sep 2005 21:34:11 +0000 : Junyor Changes are needed after the revision patch. I'm going to have to rethink upload_delete() a bit.

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 (code needs review) Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload7.patch (23.92 KB) Indeed. Fixed. 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 :) ------------------------------------------------------------------------ Tue, 06 Sep 2005 21:34:11 +0000 : Junyor Changes are needed after the revision patch. I'm going to have to rethink upload_delete() a bit. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:50:19 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload6.patch (23.58 KB) Updated patch to HEAD. I've rolled in the changes from http://drupal.org/node/30025, as they intermingle. I also removed some extraneous whitespace at the end of lines. I'm still hoping to get this into 4.7. ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:21:17 +0000 : m3avrck Settings > Uploads shows the same thing as Uploads ... looks like the settings from the other patch weren't moved over correctly?

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: Souvent22 -Status: patch (code needs review) +Status: patch (ready to be committed) +1. Yes yes. patch worked well. Really like the statistics feature, and the planned extensibility with the drop-down action. hope this makes it in. Souvent22 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 :) ------------------------------------------------------------------------ Tue, 06 Sep 2005 21:34:11 +0000 : Junyor Changes are needed after the revision patch. I'm going to have to rethink upload_delete() a bit. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:50:19 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload6.patch (23.58 KB) Updated patch to HEAD. I've rolled in the changes from http://drupal.org/node/30025, as they intermingle. I also removed some extraneous whitespace at the end of lines. I'm still hoping to get this into 4.7. ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:21:17 +0000 : m3avrck Settings > Uploads shows the same thing as Uploads ... looks like the settings from the other patch weren't moved over correctly? ------------------------------------------------------------------------ Tue, 13 Sep 2005 22:19:46 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload7.patch (23.92 KB) Indeed. Fixed. ------------------------------------------------------------------------ Wed, 14 Sep 2005 02:10:13 +0000 : m3avrck Attachment: http://drupal.org/files/issues/upload.module_5.patch (24.39 KB) Patch works great! Tested out all sorts of circumstances, patch works flawlessly. This is *much* needed functionality in core. This patch doesn't actually add anything that isn't in core, rather it just makes this information readily available which is *very* helpful. Much needed and I think because of that this should go in 4.7. Rerolled patch to fix a usability issue on Settings > Upload ... using form_group_collapasible() cleans up page when you have more than roll with file permissions.

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: m3avrck Status: patch (code needs work) Dries didn't really look through the code too much, my fault! Just tested to make sure it *actually* works. Will do that tomorrow morning, unless Junyor can patch it before me :) m3avrck 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 :) ------------------------------------------------------------------------ Tue, 06 Sep 2005 21:34:11 +0000 : Junyor Changes are needed after the revision patch. I'm going to have to rethink upload_delete() a bit. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:50:19 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload6.patch (23.58 KB) Updated patch to HEAD. I've rolled in the changes from http://drupal.org/node/30025, as they intermingle. I also removed some extraneous whitespace at the end of lines. I'm still hoping to get this into 4.7. ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:21:17 +0000 : m3avrck Settings > Uploads shows the same thing as Uploads ... looks like the settings from the other patch weren't moved over correctly? ------------------------------------------------------------------------ Tue, 13 Sep 2005 22:19:46 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload7.patch (23.92 KB) Indeed. Fixed. ------------------------------------------------------------------------ Wed, 14 Sep 2005 02:10:13 +0000 : m3avrck Attachment: http://drupal.org/files/issues/upload.module_5.patch (24.39 KB) Patch works great! Tested out all sorts of circumstances, patch works flawlessly. This is *much* needed functionality in core. This patch doesn't actually add anything that isn't in core, rather it just makes this information readily available which is *very* helpful. Much needed and I think because of that this should go in 4.7. Rerolled patch to fix a usability issue on Settings > Upload ... using form_group_collapasible() cleans up page when you have more than roll with file permissions. ------------------------------------------------------------------------ Wed, 14 Sep 2005 17:39:02 +0000 : Souvent22 +1. Yes yes. patch worked well. Really like the statistics feature, and the planned extensibility with the drop-down action. hope this makes it in. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:31:58 +0000 : Dries Souvent22, m3avrck: please review patches more carefully! This patch needs a lot of work: 1. Many strings can't be translated. 2. Why do we sort the data in PHP and not in MySQL. 3. Why do we retrieve all records in MySQL but not show them all. Why do we show 10 users only? Problem (2) and (3) should be fixed by creating a table that can be (i) sorted and (ii) paged.

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: moshe weitzman Status: patch (code needs work) In upload_total_space_used(), it looks like your query will give right results if you JOIN to the node table instead of node_revisions. same for your "too many items in list" problem. I didn't test these theories, just perused the patch. moshe weitzman 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 :) ------------------------------------------------------------------------ Tue, 06 Sep 2005 21:34:11 +0000 : Junyor Changes are needed after the revision patch. I'm going to have to rethink upload_delete() a bit. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:50:19 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload6.patch (23.58 KB) Updated patch to HEAD. I've rolled in the changes from http://drupal.org/node/30025, as they intermingle. I also removed some extraneous whitespace at the end of lines. I'm still hoping to get this into 4.7. ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:21:17 +0000 : m3avrck Settings > Uploads shows the same thing as Uploads ... looks like the settings from the other patch weren't moved over correctly? ------------------------------------------------------------------------ Tue, 13 Sep 2005 22:19:46 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload7.patch (23.92 KB) Indeed. Fixed. ------------------------------------------------------------------------ Wed, 14 Sep 2005 02:10:13 +0000 : m3avrck Attachment: http://drupal.org/files/issues/upload.module_5.patch (24.39 KB) Patch works great! Tested out all sorts of circumstances, patch works flawlessly. This is *much* needed functionality in core. This patch doesn't actually add anything that isn't in core, rather it just makes this information readily available which is *very* helpful. Much needed and I think because of that this should go in 4.7. Rerolled patch to fix a usability issue on Settings > Upload ... using form_group_collapasible() cleans up page when you have more than roll with file permissions. ------------------------------------------------------------------------ Wed, 14 Sep 2005 17:39:02 +0000 : Souvent22 +1. Yes yes. patch worked well. Really like the statistics feature, and the planned extensibility with the drop-down action. hope this makes it in. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:31:58 +0000 : Dries Souvent22, m3avrck: please review patches more carefully! This patch needs a lot of work: 1. Many strings can't be translated. 2. Why do we sort the data in PHP and not in MySQL. 3. Why do we retrieve all records in MySQL but not show them all. Why do we show 10 users only? Problem (2) and (3) should be fixed by creating a table that can be (i) sorted and (ii) paged. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:34:24 +0000 : m3avrck Dries didn't really look through the code too much, my fault! Just tested to make sure it *actually* works. Will do that tomorrow morning, unless Junyor can patch it before me :) ------------------------------------------------------------------------ Wed, 14 Sep 2005 23:59:54 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload8.patch (25.37 KB) This is as far as I'm getting tonight. Not everything is fully tested, but I believe I've addressed Dries' concerns. However, I've come up with a couple additional problems during testing and in code review. I'd appreciate help on my to do items, especially the last one. To do: - simplify user portion of upload_statistics - add revision deleting to upload_delete - figure out how to take revisions into account in queries (NOTE: the query in upload_total_space_used() is apparently wrong); right now, each revision just adds to totals or shows as an additional item in the 'my uploads' list Done: - changed a lot of double quotes to single quotes - added lots of t()s - greatly simplified admin portion of upload_statistics - moved disk space usage list to a separate tab - made disk space usage list show all users in a sortable table - fixed some comment typos - just return from menu callbacks instead of using theme('page')

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 (code needs work) +Status: patch (code needs review) Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload10.patch (26.01 KB) OK, all done. upload_statistics() has been cleansed. 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 :) ------------------------------------------------------------------------ Tue, 06 Sep 2005 21:34:11 +0000 : Junyor Changes are needed after the revision patch. I'm going to have to rethink upload_delete() a bit. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:50:19 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload6.patch (23.58 KB) Updated patch to HEAD. I've rolled in the changes from http://drupal.org/node/30025, as they intermingle. I also removed some extraneous whitespace at the end of lines. I'm still hoping to get this into 4.7. ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:21:17 +0000 : m3avrck Settings > Uploads shows the same thing as Uploads ... looks like the settings from the other patch weren't moved over correctly? ------------------------------------------------------------------------ Tue, 13 Sep 2005 22:19:46 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload7.patch (23.92 KB) Indeed. Fixed. ------------------------------------------------------------------------ Wed, 14 Sep 2005 02:10:13 +0000 : m3avrck Attachment: http://drupal.org/files/issues/upload.module_5.patch (24.39 KB) Patch works great! Tested out all sorts of circumstances, patch works flawlessly. This is *much* needed functionality in core. This patch doesn't actually add anything that isn't in core, rather it just makes this information readily available which is *very* helpful. Much needed and I think because of that this should go in 4.7. Rerolled patch to fix a usability issue on Settings > Upload ... using form_group_collapasible() cleans up page when you have more than roll with file permissions. ------------------------------------------------------------------------ Wed, 14 Sep 2005 17:39:02 +0000 : Souvent22 +1. Yes yes. patch worked well. Really like the statistics feature, and the planned extensibility with the drop-down action. hope this makes it in. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:31:58 +0000 : Dries Souvent22, m3avrck: please review patches more carefully! This patch needs a lot of work: 1. Many strings can't be translated. 2. Why do we sort the data in PHP and not in MySQL. 3. Why do we retrieve all records in MySQL but not show them all. Why do we show 10 users only? Problem (2) and (3) should be fixed by creating a table that can be (i) sorted and (ii) paged. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:34:24 +0000 : m3avrck Dries didn't really look through the code too much, my fault! Just tested to make sure it *actually* works. Will do that tomorrow morning, unless Junyor can patch it before me :) ------------------------------------------------------------------------ Wed, 14 Sep 2005 23:59:54 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload8.patch (25.37 KB) This is as far as I'm getting tonight. Not everything is fully tested, but I believe I've addressed Dries' concerns. However, I've come up with a couple additional problems during testing and in code review. I'd appreciate help on my to do items, especially the last one. To do: - simplify user portion of upload_statistics - add revision deleting to upload_delete - figure out how to take revisions into account in queries (NOTE: the query in upload_total_space_used() is apparently wrong); right now, each revision just adds to totals or shows as an additional item in the 'my uploads' list Done: - changed a lot of double quotes to single quotes - added lots of t()s - greatly simplified admin portion of upload_statistics - moved disk space usage list to a separate tab - made disk space usage list show all users in a sortable table - fixed some comment typos - just return from menu callbacks instead of using theme('page') ------------------------------------------------------------------------ Thu, 15 Sep 2005 00:20:10 +0000 : moshe weitzman In upload_total_space_used(), it looks like your query will give right results if you JOIN to the node table instead of node_revisions. same for your "too many items in list" problem. I didn't test these theories, just perused the patch. ------------------------------------------------------------------------ Thu, 15 Sep 2005 21:53:43 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload9.patch (25.75 KB) And another update: - Fixed help texts so they actually display - Added new help texts - Split out admin statistics to upload_admin, since it was only three lines - Fixed a bug introduced in the last patch related to the display of per user statistics The problem in upload_total_space_used() and other queries I'm adding need to be addressed by changes to the files table, IMHO. That, and the changes for deleting revisions, will have to wait for a revision bug fix patch. That leaves cleaning up upload_statistics.

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: Souvent22 Status: patch (code needs review) Attachment: http://drupal.org/files/issues/upload.module_6.patch (26.79 KB) Ok, fixed a few things. From the top now: - Fixed the quries that were returning incorrect information. Namley, disk usage per user, and total disk usage. - Added a new function to calcuate the number of files a user has. - Formatting issues. Now, there is a serperate issue, which should be addressed in a seperate bug, which is the current layout of the file table. I believe a second table (associatve table) should be used to link revisions to files, instead of lots of redundant data in the files table. This is a normalization issue. However, I am using the system as is, and have adjusted the quries, etc. to fit. Souvent22 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 :) ------------------------------------------------------------------------ Tue, 06 Sep 2005 21:34:11 +0000 : Junyor Changes are needed after the revision patch. I'm going to have to rethink upload_delete() a bit. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:50:19 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload6.patch (23.58 KB) Updated patch to HEAD. I've rolled in the changes from http://drupal.org/node/30025, as they intermingle. I also removed some extraneous whitespace at the end of lines. I'm still hoping to get this into 4.7. ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:21:17 +0000 : m3avrck Settings > Uploads shows the same thing as Uploads ... looks like the settings from the other patch weren't moved over correctly? ------------------------------------------------------------------------ Tue, 13 Sep 2005 22:19:46 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload7.patch (23.92 KB) Indeed. Fixed. ------------------------------------------------------------------------ Wed, 14 Sep 2005 02:10:13 +0000 : m3avrck Attachment: http://drupal.org/files/issues/upload.module_5.patch (24.39 KB) Patch works great! Tested out all sorts of circumstances, patch works flawlessly. This is *much* needed functionality in core. This patch doesn't actually add anything that isn't in core, rather it just makes this information readily available which is *very* helpful. Much needed and I think because of that this should go in 4.7. Rerolled patch to fix a usability issue on Settings > Upload ... using form_group_collapasible() cleans up page when you have more than roll with file permissions. ------------------------------------------------------------------------ Wed, 14 Sep 2005 17:39:02 +0000 : Souvent22 +1. Yes yes. patch worked well. Really like the statistics feature, and the planned extensibility with the drop-down action. hope this makes it in. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:31:58 +0000 : Dries Souvent22, m3avrck: please review patches more carefully! This patch needs a lot of work: 1. Many strings can't be translated. 2. Why do we sort the data in PHP and not in MySQL. 3. Why do we retrieve all records in MySQL but not show them all. Why do we show 10 users only? Problem (2) and (3) should be fixed by creating a table that can be (i) sorted and (ii) paged. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:34:24 +0000 : m3avrck Dries didn't really look through the code too much, my fault! Just tested to make sure it *actually* works. Will do that tomorrow morning, unless Junyor can patch it before me :) ------------------------------------------------------------------------ Wed, 14 Sep 2005 23:59:54 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload8.patch (25.37 KB) This is as far as I'm getting tonight. Not everything is fully tested, but I believe I've addressed Dries' concerns. However, I've come up with a couple additional problems during testing and in code review. I'd appreciate help on my to do items, especially the last one. To do: - simplify user portion of upload_statistics - add revision deleting to upload_delete - figure out how to take revisions into account in queries (NOTE: the query in upload_total_space_used() is apparently wrong); right now, each revision just adds to totals or shows as an additional item in the 'my uploads' list Done: - changed a lot of double quotes to single quotes - added lots of t()s - greatly simplified admin portion of upload_statistics - moved disk space usage list to a separate tab - made disk space usage list show all users in a sortable table - fixed some comment typos - just return from menu callbacks instead of using theme('page') ------------------------------------------------------------------------ Thu, 15 Sep 2005 00:20:10 +0000 : moshe weitzman In upload_total_space_used(), it looks like your query will give right results if you JOIN to the node table instead of node_revisions. same for your "too many items in list" problem. I didn't test these theories, just perused the patch. ------------------------------------------------------------------------ Thu, 15 Sep 2005 21:53:43 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload9.patch (25.75 KB) And another update: - Fixed help texts so they actually display - Added new help texts - Split out admin statistics to upload_admin, since it was only three lines - Fixed a bug introduced in the last patch related to the display of per user statistics The problem in upload_total_space_used() and other queries I'm adding need to be addressed by changes to the files table, IMHO. That, and the changes for deleting revisions, will have to wait for a revision bug fix patch. That leaves cleaning up upload_statistics. ------------------------------------------------------------------------ Thu, 15 Sep 2005 23:01:20 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload10.patch (26.01 KB) OK, all done. upload_statistics() has been cleansed. ------------------------------------------------------------------------ Thu, 15 Sep 2005 23:19:19 +0000 : m3avrck Dries, just a heads up that Junyor, Souvent, and myself have been discussing this issue all day in #upload ... many hours went into this patch, all of your concerns have been addressed, along with many others we have discovered. This patch is rock solid and we'll be posting one final patch very soon that completes this. Better administrative options is all this patch introduces, no changes to how files are handled. Also, this patch uncovered 2 bugs inherent to revisions that were overlooked so this patch has been very worthwhile :)

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 (code needs review) Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload11.patch (25.85 KB) I agree with Souvent that the revision information should be moved to another table. It's causing a lot of excess data in the files table. Also, most of the queries related to revisions in upload.module are currently broken. My patch uses the queries as they should be given this change in the file table. @Stefan: I copied the form_set_error() stuff from node.module, which also doesn't set the first parameter. I've looked at the way several functions use form_set_error() and the first parameter seems to be the name of the form that must be corrected. According to nodeapi_example_nodeapi[1], "the user must correct the error before being able to submit the [form]." I don't think that should apply to this form, just as it doesn't apply to the update form on the node admin page. Is there a good alternative to theme('box')? [1] http://drupaldocs.org/api/head/function/nodeapi_example_nodeapi @Souvent: I'm not sure all those changes are necessary. 1) You added a separate upload_file_count function which does an additional query. All the file count and size information comes from one query in my version, which I think is faster. 2) You removed some commented out code: cool. 3) You changed the number of entries on the file list page from 50 to 25. The node, comment, and watchdog overview pages all use 50, so I was just going for consistency. 4) You updated a bunch of queries to work-around bugs introduced by the revision patch. I'd like to fix the problems with the revision patch in a separate patch instead of adding work-arounds. 5) You changed the way upload_admin_usage gathers space usage about users. Instead of a single query, there's now a query per user. That's a bit expensive, isn't it? 6) The query in upload_admin_usage() was indeed broken. However, I seemed to be able to fix it by doing the group by on u.uid instead of f.fid. I've attached a new patch with #2 and #6 addressed. 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 :) ------------------------------------------------------------------------ Tue, 06 Sep 2005 21:34:11 +0000 : Junyor Changes are needed after the revision patch. I'm going to have to rethink upload_delete() a bit. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:50:19 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload6.patch (23.58 KB) Updated patch to HEAD. I've rolled in the changes from http://drupal.org/node/30025, as they intermingle. I also removed some extraneous whitespace at the end of lines. I'm still hoping to get this into 4.7. ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:21:17 +0000 : m3avrck Settings > Uploads shows the same thing as Uploads ... looks like the settings from the other patch weren't moved over correctly? ------------------------------------------------------------------------ Tue, 13 Sep 2005 22:19:46 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload7.patch (23.92 KB) Indeed. Fixed. ------------------------------------------------------------------------ Wed, 14 Sep 2005 02:10:13 +0000 : m3avrck Attachment: http://drupal.org/files/issues/upload.module_5.patch (24.39 KB) Patch works great! Tested out all sorts of circumstances, patch works flawlessly. This is *much* needed functionality in core. This patch doesn't actually add anything that isn't in core, rather it just makes this information readily available which is *very* helpful. Much needed and I think because of that this should go in 4.7. Rerolled patch to fix a usability issue on Settings > Upload ... using form_group_collapasible() cleans up page when you have more than roll with file permissions. ------------------------------------------------------------------------ Wed, 14 Sep 2005 17:39:02 +0000 : Souvent22 +1. Yes yes. patch worked well. Really like the statistics feature, and the planned extensibility with the drop-down action. hope this makes it in. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:31:58 +0000 : Dries Souvent22, m3avrck: please review patches more carefully! This patch needs a lot of work: 1. Many strings can't be translated. 2. Why do we sort the data in PHP and not in MySQL. 3. Why do we retrieve all records in MySQL but not show them all. Why do we show 10 users only? Problem (2) and (3) should be fixed by creating a table that can be (i) sorted and (ii) paged. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:34:24 +0000 : m3avrck Dries didn't really look through the code too much, my fault! Just tested to make sure it *actually* works. Will do that tomorrow morning, unless Junyor can patch it before me :) ------------------------------------------------------------------------ Wed, 14 Sep 2005 23:59:54 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload8.patch (25.37 KB) This is as far as I'm getting tonight. Not everything is fully tested, but I believe I've addressed Dries' concerns. However, I've come up with a couple additional problems during testing and in code review. I'd appreciate help on my to do items, especially the last one. To do: - simplify user portion of upload_statistics - add revision deleting to upload_delete - figure out how to take revisions into account in queries (NOTE: the query in upload_total_space_used() is apparently wrong); right now, each revision just adds to totals or shows as an additional item in the 'my uploads' list Done: - changed a lot of double quotes to single quotes - added lots of t()s - greatly simplified admin portion of upload_statistics - moved disk space usage list to a separate tab - made disk space usage list show all users in a sortable table - fixed some comment typos - just return from menu callbacks instead of using theme('page') ------------------------------------------------------------------------ Thu, 15 Sep 2005 00:20:10 +0000 : moshe weitzman In upload_total_space_used(), it looks like your query will give right results if you JOIN to the node table instead of node_revisions. same for your "too many items in list" problem. I didn't test these theories, just perused the patch. ------------------------------------------------------------------------ Thu, 15 Sep 2005 21:53:43 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload9.patch (25.75 KB) And another update: - Fixed help texts so they actually display - Added new help texts - Split out admin statistics to upload_admin, since it was only three lines - Fixed a bug introduced in the last patch related to the display of per user statistics The problem in upload_total_space_used() and other queries I'm adding need to be addressed by changes to the files table, IMHO. That, and the changes for deleting revisions, will have to wait for a revision bug fix patch. That leaves cleaning up upload_statistics. ------------------------------------------------------------------------ Thu, 15 Sep 2005 23:01:20 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload10.patch (26.01 KB) OK, all done. upload_statistics() has been cleansed. ------------------------------------------------------------------------ Thu, 15 Sep 2005 23:19:19 +0000 : m3avrck Dries, just a heads up that Junyor, Souvent, and myself have been discussing this issue all day in #upload ... many hours went into this patch, all of your concerns have been addressed, along with many others we have discovered. This patch is rock solid and we'll be posting one final patch very soon that completes this. Better administrative options is all this patch introduces, no changes to how files are handled. Also, this patch uncovered 2 bugs inherent to revisions that were overlooked so this patch has been very worthwhile :) ------------------------------------------------------------------------ Fri, 16 Sep 2005 00:53:35 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/upload.module_6.patch (26.79 KB) Ok, fixed a few things. From the top now: - Fixed the quries that were returning incorrect information. Namley, disk usage per user, and total disk usage. - Added a new function to calcuate the number of files a user has. - Formatting issues. Now, there is a serperate issue, which should be addressed in a seperate bug, which is the current layout of the file table. I believe a second table (associatve table) should be used to link revisions to files, instead of lots of redundant data in the files table. This is a normalization issue. However, I am using the system as is, and have adjusted the quries, etc. to fit. ------------------------------------------------------------------------ Fri, 16 Sep 2005 09:29:28 +0000 : stefan nagtegaal Hi guys! Sorry for jumping in so lately on the threat, but I reviewed the code and I must say that I like what I see.. Though, some problems code-wise imo: <?php +      form_set_error('', t('Please select some items to perform the update on.')); ?> Why is the first argument not set? IMO this should always be set, otherwise use drupal_set_message('error', $message). Secondly you guys are using: <?php +  $output .= theme('box', t('Statistics'), $content); ?> By the 'introduce theme wrapper function [1]' threath, comment #37 Dries encourages people to remove the usage of theme('box').. So, looking at the future you guys can probably do the same? Try not using the theme('box'), but try something else... For what's left, I love the functionality!!! Very, very good work.. [1] http://drupal.org/node/23584

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: m3avrck Status: patch (code needs review) Stefan: Souvent, Junyor and myself all discussed your ideas. The first, form_error('') is indeed valid. The content tab in admin uses the same interface setup and uses the function in the exact same way. Check node.module to see this. Second, while we agree with the theme('box') issue, the rest of the Drupal still uses theme('box') in the same type of ways. Since there is no "better" way to do this, we went with consisentancy since we can't come up with a better alternative. Also, we have all reached an agreement, this patch does indeed work and works *very* well. The issues with files and revisions is completely seperate. This patch works 100% and if later on their are core changes to files and revisions, we'll update upload.module accordingly (which we hope to do too :)). New patch in a bit to fix the other outstanding problems. m3avrck 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 :) ------------------------------------------------------------------------ Tue, 06 Sep 2005 21:34:11 +0000 : Junyor Changes are needed after the revision patch. I'm going to have to rethink upload_delete() a bit. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:50:19 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload6.patch (23.58 KB) Updated patch to HEAD. I've rolled in the changes from http://drupal.org/node/30025, as they intermingle. I also removed some extraneous whitespace at the end of lines. I'm still hoping to get this into 4.7. ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:21:17 +0000 : m3avrck Settings > Uploads shows the same thing as Uploads ... looks like the settings from the other patch weren't moved over correctly? ------------------------------------------------------------------------ Tue, 13 Sep 2005 22:19:46 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload7.patch (23.92 KB) Indeed. Fixed. ------------------------------------------------------------------------ Wed, 14 Sep 2005 02:10:13 +0000 : m3avrck Attachment: http://drupal.org/files/issues/upload.module_5.patch (24.39 KB) Patch works great! Tested out all sorts of circumstances, patch works flawlessly. This is *much* needed functionality in core. This patch doesn't actually add anything that isn't in core, rather it just makes this information readily available which is *very* helpful. Much needed and I think because of that this should go in 4.7. Rerolled patch to fix a usability issue on Settings > Upload ... using form_group_collapasible() cleans up page when you have more than roll with file permissions. ------------------------------------------------------------------------ Wed, 14 Sep 2005 17:39:02 +0000 : Souvent22 +1. Yes yes. patch worked well. Really like the statistics feature, and the planned extensibility with the drop-down action. hope this makes it in. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:31:58 +0000 : Dries Souvent22, m3avrck: please review patches more carefully! This patch needs a lot of work: 1. Many strings can't be translated. 2. Why do we sort the data in PHP and not in MySQL. 3. Why do we retrieve all records in MySQL but not show them all. Why do we show 10 users only? Problem (2) and (3) should be fixed by creating a table that can be (i) sorted and (ii) paged. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:34:24 +0000 : m3avrck Dries didn't really look through the code too much, my fault! Just tested to make sure it *actually* works. Will do that tomorrow morning, unless Junyor can patch it before me :) ------------------------------------------------------------------------ Wed, 14 Sep 2005 23:59:54 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload8.patch (25.37 KB) This is as far as I'm getting tonight. Not everything is fully tested, but I believe I've addressed Dries' concerns. However, I've come up with a couple additional problems during testing and in code review. I'd appreciate help on my to do items, especially the last one. To do: - simplify user portion of upload_statistics - add revision deleting to upload_delete - figure out how to take revisions into account in queries (NOTE: the query in upload_total_space_used() is apparently wrong); right now, each revision just adds to totals or shows as an additional item in the 'my uploads' list Done: - changed a lot of double quotes to single quotes - added lots of t()s - greatly simplified admin portion of upload_statistics - moved disk space usage list to a separate tab - made disk space usage list show all users in a sortable table - fixed some comment typos - just return from menu callbacks instead of using theme('page') ------------------------------------------------------------------------ Thu, 15 Sep 2005 00:20:10 +0000 : moshe weitzman In upload_total_space_used(), it looks like your query will give right results if you JOIN to the node table instead of node_revisions. same for your "too many items in list" problem. I didn't test these theories, just perused the patch. ------------------------------------------------------------------------ Thu, 15 Sep 2005 21:53:43 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload9.patch (25.75 KB) And another update: - Fixed help texts so they actually display - Added new help texts - Split out admin statistics to upload_admin, since it was only three lines - Fixed a bug introduced in the last patch related to the display of per user statistics The problem in upload_total_space_used() and other queries I'm adding need to be addressed by changes to the files table, IMHO. That, and the changes for deleting revisions, will have to wait for a revision bug fix patch. That leaves cleaning up upload_statistics. ------------------------------------------------------------------------ Thu, 15 Sep 2005 23:01:20 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload10.patch (26.01 KB) OK, all done. upload_statistics() has been cleansed. ------------------------------------------------------------------------ Thu, 15 Sep 2005 23:19:19 +0000 : m3avrck Dries, just a heads up that Junyor, Souvent, and myself have been discussing this issue all day in #upload ... many hours went into this patch, all of your concerns have been addressed, along with many others we have discovered. This patch is rock solid and we'll be posting one final patch very soon that completes this. Better administrative options is all this patch introduces, no changes to how files are handled. Also, this patch uncovered 2 bugs inherent to revisions that were overlooked so this patch has been very worthwhile :) ------------------------------------------------------------------------ Fri, 16 Sep 2005 00:53:35 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/upload.module_6.patch (26.79 KB) Ok, fixed a few things. From the top now: - Fixed the quries that were returning incorrect information. Namley, disk usage per user, and total disk usage. - Added a new function to calcuate the number of files a user has. - Formatting issues. Now, there is a serperate issue, which should be addressed in a seperate bug, which is the current layout of the file table. I believe a second table (associatve table) should be used to link revisions to files, instead of lots of redundant data in the files table. This is a normalization issue. However, I am using the system as is, and have adjusted the quries, etc. to fit. ------------------------------------------------------------------------ Fri, 16 Sep 2005 09:29:28 +0000 : stefan nagtegaal Hi guys! Sorry for jumping in so lately on the threat, but I reviewed the code and I must say that I like what I see.. Though, some problems code-wise imo: <?php +      form_set_error('', t('Please select some items to perform the update on.')); ?> Why is the first argument not set? IMO this should always be set, otherwise use drupal_set_message('error', $message). Secondly you guys are using: <?php +  $output .= theme('box', t('Statistics'), $content); ?> By the 'introduce theme wrapper function [1]' threath, comment #37 Dries encourages people to remove the usage of theme('box').. So, looking at the future you guys can probably do the same? Try not using the theme('box'), but try something else... For what's left, I love the functionality!!! Very, very good work.. [1] http://drupal.org/node/23584 ------------------------------------------------------------------------ Fri, 16 Sep 2005 11:18:02 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload11.patch (25.85 KB) I agree with Souvent that the revision information should be moved to another table. It's causing a lot of excess data in the files table. Also, most of the queries related to revisions in upload.module are currently broken. My patch uses the queries as they should be given this change in the file table. @Stefan: I copied the form_set_error() stuff from node.module, which also doesn't set the first parameter. I've looked at the way several functions use form_set_error() and the first parameter seems to be the name of the form that must be corrected. According to nodeapi_example_nodeapi[1], "the user must correct the error before being able to submit the [form]." I don't think that should apply to this form, just as it doesn't apply to the update form on the node admin page. Is there a good alternative to theme('box')? [1] http://drupaldocs.org/api/head/function/nodeapi_example_nodeapi @Souvent: I'm not sure all those changes are necessary. 1) You added a separate upload_file_count function which does an additional query. All the file count and size information comes from one query in my version, which I think is faster. 2) You removed some commented out code: cool. 3) You changed the number of entries on the file list page from 50 to 25. The node, comment, and watchdog overview pages all use 50, so I was just going for consistency. 4) You updated a bunch of queries to work-around bugs introduced by the revision patch. I'd like to fix the problems with the revision patch in a separate patch instead of adding work-arounds. 5) You changed the way upload_admin_usage gathers space usage about users. Instead of a single query, there's now a query per user. That's a bit expensive, isn't it? 6) The query in upload_admin_usage() was indeed broken. However, I seemed to be able to fix it by doing the group by on u.uid instead of f.fid. I've attached a new patch with #2 and #6 addressed. ------------------------------------------------------------------------ Fri, 16 Sep 2005 14:11:38 +0000 : m3avrck Junyor, Souvent and I spent many hours last night after you posted your patch fixing many many bugs. 1. Yes we have another count query but this one actually works ;-) count(*) has the same aggregate problems as sum() and because of the nature of the files table, will *only* work this way without subselects. either the tables get changed around a bit or subselects be allowed. since the later assumes everyone is using MySQL 4.1 seesm like the former is the answer. We have already created an issue for this: http://drupal.org/node/31354 In addition, this patch *won't* get in if it assumes that another patch will to fix the table, this is not good. Souvent and myself have already committed ourselves to this patch and the other patch and will update things accordingly. If our propsed patch gets in with the correct queries, we'll post a patch later to fix the files table. Once that patch goes in, we'll post a patch to "re-fix" the uploads module to get it up to speed. Sure this is double work, but this is the best method and only way we can get this patch in now. We'll take care of it :) 3. We can change these back to 50, was just a bit too many in our opinions but consisently rules out. 4. We didn't introduce any "workarounds". Rather, we got it working as is :) Once the updated revisions patches go in: http://drupal.org/node/31355 we'll accordingly refactor this patch. Again, this patch can't wait for others, otherwise it won't go in. 5. Again, same problem with subselects. 6. We already fixed it. Again, the biggest problem is the fact that Drupal does *not* support subselects, hence we have to be creative in our queries :) Sure a better files table would help, *however* this won't necessarily fix all of the queries magically. I still see the subselects coming into play. We rigorously tested all of the queries last night and they do indeed your work. Your patch was broken in many cases because of this. We can roll another patch addressing Stefan's concerns.

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: m3avrck Status: patch (code needs review) Attachment: http://drupal.org/files/issues/upload.module_7_0.patch (27.35 KB) Small typo in query just discovered, fixed now. m3avrck 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 :) ------------------------------------------------------------------------ Tue, 06 Sep 2005 21:34:11 +0000 : Junyor Changes are needed after the revision patch. I'm going to have to rethink upload_delete() a bit. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:50:19 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload6.patch (23.58 KB) Updated patch to HEAD. I've rolled in the changes from http://drupal.org/node/30025, as they intermingle. I also removed some extraneous whitespace at the end of lines. I'm still hoping to get this into 4.7. ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:21:17 +0000 : m3avrck Settings > Uploads shows the same thing as Uploads ... looks like the settings from the other patch weren't moved over correctly? ------------------------------------------------------------------------ Tue, 13 Sep 2005 22:19:46 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload7.patch (23.92 KB) Indeed. Fixed. ------------------------------------------------------------------------ Wed, 14 Sep 2005 02:10:13 +0000 : m3avrck Attachment: http://drupal.org/files/issues/upload.module_5.patch (24.39 KB) Patch works great! Tested out all sorts of circumstances, patch works flawlessly. This is *much* needed functionality in core. This patch doesn't actually add anything that isn't in core, rather it just makes this information readily available which is *very* helpful. Much needed and I think because of that this should go in 4.7. Rerolled patch to fix a usability issue on Settings > Upload ... using form_group_collapasible() cleans up page when you have more than roll with file permissions. ------------------------------------------------------------------------ Wed, 14 Sep 2005 17:39:02 +0000 : Souvent22 +1. Yes yes. patch worked well. Really like the statistics feature, and the planned extensibility with the drop-down action. hope this makes it in. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:31:58 +0000 : Dries Souvent22, m3avrck: please review patches more carefully! This patch needs a lot of work: 1. Many strings can't be translated. 2. Why do we sort the data in PHP and not in MySQL. 3. Why do we retrieve all records in MySQL but not show them all. Why do we show 10 users only? Problem (2) and (3) should be fixed by creating a table that can be (i) sorted and (ii) paged. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:34:24 +0000 : m3avrck Dries didn't really look through the code too much, my fault! Just tested to make sure it *actually* works. Will do that tomorrow morning, unless Junyor can patch it before me :) ------------------------------------------------------------------------ Wed, 14 Sep 2005 23:59:54 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload8.patch (25.37 KB) This is as far as I'm getting tonight. Not everything is fully tested, but I believe I've addressed Dries' concerns. However, I've come up with a couple additional problems during testing and in code review. I'd appreciate help on my to do items, especially the last one. To do: - simplify user portion of upload_statistics - add revision deleting to upload_delete - figure out how to take revisions into account in queries (NOTE: the query in upload_total_space_used() is apparently wrong); right now, each revision just adds to totals or shows as an additional item in the 'my uploads' list Done: - changed a lot of double quotes to single quotes - added lots of t()s - greatly simplified admin portion of upload_statistics - moved disk space usage list to a separate tab - made disk space usage list show all users in a sortable table - fixed some comment typos - just return from menu callbacks instead of using theme('page') ------------------------------------------------------------------------ Thu, 15 Sep 2005 00:20:10 +0000 : moshe weitzman In upload_total_space_used(), it looks like your query will give right results if you JOIN to the node table instead of node_revisions. same for your "too many items in list" problem. I didn't test these theories, just perused the patch. ------------------------------------------------------------------------ Thu, 15 Sep 2005 21:53:43 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload9.patch (25.75 KB) And another update: - Fixed help texts so they actually display - Added new help texts - Split out admin statistics to upload_admin, since it was only three lines - Fixed a bug introduced in the last patch related to the display of per user statistics The problem in upload_total_space_used() and other queries I'm adding need to be addressed by changes to the files table, IMHO. That, and the changes for deleting revisions, will have to wait for a revision bug fix patch. That leaves cleaning up upload_statistics. ------------------------------------------------------------------------ Thu, 15 Sep 2005 23:01:20 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload10.patch (26.01 KB) OK, all done. upload_statistics() has been cleansed. ------------------------------------------------------------------------ Thu, 15 Sep 2005 23:19:19 +0000 : m3avrck Dries, just a heads up that Junyor, Souvent, and myself have been discussing this issue all day in #upload ... many hours went into this patch, all of your concerns have been addressed, along with many others we have discovered. This patch is rock solid and we'll be posting one final patch very soon that completes this. Better administrative options is all this patch introduces, no changes to how files are handled. Also, this patch uncovered 2 bugs inherent to revisions that were overlooked so this patch has been very worthwhile :) ------------------------------------------------------------------------ Fri, 16 Sep 2005 00:53:35 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/upload.module_6.patch (26.79 KB) Ok, fixed a few things. From the top now: - Fixed the quries that were returning incorrect information. Namley, disk usage per user, and total disk usage. - Added a new function to calcuate the number of files a user has. - Formatting issues. Now, there is a serperate issue, which should be addressed in a seperate bug, which is the current layout of the file table. I believe a second table (associatve table) should be used to link revisions to files, instead of lots of redundant data in the files table. This is a normalization issue. However, I am using the system as is, and have adjusted the quries, etc. to fit. ------------------------------------------------------------------------ Fri, 16 Sep 2005 09:29:28 +0000 : stefan nagtegaal Hi guys! Sorry for jumping in so lately on the threat, but I reviewed the code and I must say that I like what I see.. Though, some problems code-wise imo: <?php +      form_set_error('', t('Please select some items to perform the update on.')); ?> Why is the first argument not set? IMO this should always be set, otherwise use drupal_set_message('error', $message). Secondly you guys are using: <?php +  $output .= theme('box', t('Statistics'), $content); ?> By the 'introduce theme wrapper function [1]' threath, comment #37 Dries encourages people to remove the usage of theme('box').. So, looking at the future you guys can probably do the same? Try not using the theme('box'), but try something else... For what's left, I love the functionality!!! Very, very good work.. [1] http://drupal.org/node/23584 ------------------------------------------------------------------------ Fri, 16 Sep 2005 11:18:02 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload11.patch (25.85 KB) I agree with Souvent that the revision information should be moved to another table. It's causing a lot of excess data in the files table. Also, most of the queries related to revisions in upload.module are currently broken. My patch uses the queries as they should be given this change in the file table. @Stefan: I copied the form_set_error() stuff from node.module, which also doesn't set the first parameter. I've looked at the way several functions use form_set_error() and the first parameter seems to be the name of the form that must be corrected. According to nodeapi_example_nodeapi[1], "the user must correct the error before being able to submit the [form]." I don't think that should apply to this form, just as it doesn't apply to the update form on the node admin page. Is there a good alternative to theme('box')? [1] http://drupaldocs.org/api/head/function/nodeapi_example_nodeapi @Souvent: I'm not sure all those changes are necessary. 1) You added a separate upload_file_count function which does an additional query. All the file count and size information comes from one query in my version, which I think is faster. 2) You removed some commented out code: cool. 3) You changed the number of entries on the file list page from 50 to 25. The node, comment, and watchdog overview pages all use 50, so I was just going for consistency. 4) You updated a bunch of queries to work-around bugs introduced by the revision patch. I'd like to fix the problems with the revision patch in a separate patch instead of adding work-arounds. 5) You changed the way upload_admin_usage gathers space usage about users. Instead of a single query, there's now a query per user. That's a bit expensive, isn't it? 6) The query in upload_admin_usage() was indeed broken. However, I seemed to be able to fix it by doing the group by on u.uid instead of f.fid. I've attached a new patch with #2 and #6 addressed. ------------------------------------------------------------------------ Fri, 16 Sep 2005 14:11:38 +0000 : m3avrck Junyor, Souvent and I spent many hours last night after you posted your patch fixing many many bugs. 1. Yes we have another count query but this one actually works ;-) count(*) has the same aggregate problems as sum() and because of the nature of the files table, will *only* work this way without subselects. either the tables get changed around a bit or subselects be allowed. since the later assumes everyone is using MySQL 4.1 seesm like the former is the answer. We have already created an issue for this: http://drupal.org/node/31354 In addition, this patch *won't* get in if it assumes that another patch will to fix the table, this is not good. Souvent and myself have already committed ourselves to this patch and the other patch and will update things accordingly. If our propsed patch gets in with the correct queries, we'll post a patch later to fix the files table. Once that patch goes in, we'll post a patch to "re-fix" the uploads module to get it up to speed. Sure this is double work, but this is the best method and only way we can get this patch in now. We'll take care of it :) 3. We can change these back to 50, was just a bit too many in our opinions but consisently rules out. 4. We didn't introduce any "workarounds". Rather, we got it working as is :) Once the updated revisions patches go in: http://drupal.org/node/31355 we'll accordingly refactor this patch. Again, this patch can't wait for others, otherwise it won't go in. 5. Again, same problem with subselects. 6. We already fixed it. Again, the biggest problem is the fact that Drupal does *not* support subselects, hence we have to be creative in our queries :) Sure a better files table would help, *however* this won't necessarily fix all of the queries magically. I still see the subselects coming into play. We rigorously tested all of the queries last night and they do indeed your work. Your patch was broken in many cases because of this. We can roll another patch addressing Stefan's concerns. ------------------------------------------------------------------------ Fri, 16 Sep 2005 14:48:34 +0000 : m3avrck Stefan: Souvent, Junyor and myself all discussed your ideas. The first, form_error('') is indeed valid. The content tab in admin uses the same interface setup and uses the function in the exact same way. Check node.module to see this. Second, while we agree with the theme('box') issue, the rest of the Drupal still uses theme('box') in the same type of ways. Since there is no "better" way to do this, we went with consisentancy since we can't come up with a better alternative. Also, we have all reached an agreement, this patch does indeed work and works *very* well. The issues with files and revisions is completely seperate. This patch works 100% and if later on their are core changes to files and revisions, we'll update upload.module accordingly (which we hope to do too :)). New patch in a bit to fix the other outstanding problems. ------------------------------------------------------------------------ Fri, 16 Sep 2005 15:38:59 +0000 : m3avrck Attachment: http://drupal.org/files/issues/upload.module_7.patch (27.36 KB) Updated patch, addressed remaining issues and cleaned up a few small things. Ready to go from this side!

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: Cvbge Status: patch (ready to be committed) Hello, I've scanned the patch and you need to change $result = db_query('SELECT fid,filesize FROM {files} f INNER JOIN {node} n ON f.nid = n.nid WHERE n.uid = %d GROUP BY fid', $uid); in upload_space_used() to $result = db_query('SELECT fid,filesize FROM {files} f INNER JOIN {node} n ON f.nid = n.nid WHERE n.uid = %d GROUP BY fid, filesize', $uid); (i.e. add filesize to GROUP statement. Otherwise it won't work in postgres. This should not change anything, as all rows with the same fid have the same filesize (if I understand the meaning of fid correctly)). The same goes to $result = db_query('SELECT fid,filesize FROM {files} GROUP BY fid'); in upload_total_space_used() - needs filesize in GROUP BY. Cvbge 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 :) ------------------------------------------------------------------------ Tue, 06 Sep 2005 21:34:11 +0000 : Junyor Changes are needed after the revision patch. I'm going to have to rethink upload_delete() a bit. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:50:19 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload6.patch (23.58 KB) Updated patch to HEAD. I've rolled in the changes from http://drupal.org/node/30025, as they intermingle. I also removed some extraneous whitespace at the end of lines. I'm still hoping to get this into 4.7. ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:21:17 +0000 : m3avrck Settings > Uploads shows the same thing as Uploads ... looks like the settings from the other patch weren't moved over correctly? ------------------------------------------------------------------------ Tue, 13 Sep 2005 22:19:46 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload7.patch (23.92 KB) Indeed. Fixed. ------------------------------------------------------------------------ Wed, 14 Sep 2005 02:10:13 +0000 : m3avrck Attachment: http://drupal.org/files/issues/upload.module_5.patch (24.39 KB) Patch works great! Tested out all sorts of circumstances, patch works flawlessly. This is *much* needed functionality in core. This patch doesn't actually add anything that isn't in core, rather it just makes this information readily available which is *very* helpful. Much needed and I think because of that this should go in 4.7. Rerolled patch to fix a usability issue on Settings > Upload ... using form_group_collapasible() cleans up page when you have more than roll with file permissions. ------------------------------------------------------------------------ Wed, 14 Sep 2005 17:39:02 +0000 : Souvent22 +1. Yes yes. patch worked well. Really like the statistics feature, and the planned extensibility with the drop-down action. hope this makes it in. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:31:58 +0000 : Dries Souvent22, m3avrck: please review patches more carefully! This patch needs a lot of work: 1. Many strings can't be translated. 2. Why do we sort the data in PHP and not in MySQL. 3. Why do we retrieve all records in MySQL but not show them all. Why do we show 10 users only? Problem (2) and (3) should be fixed by creating a table that can be (i) sorted and (ii) paged. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:34:24 +0000 : m3avrck Dries didn't really look through the code too much, my fault! Just tested to make sure it *actually* works. Will do that tomorrow morning, unless Junyor can patch it before me :) ------------------------------------------------------------------------ Wed, 14 Sep 2005 23:59:54 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload8.patch (25.37 KB) This is as far as I'm getting tonight. Not everything is fully tested, but I believe I've addressed Dries' concerns. However, I've come up with a couple additional problems during testing and in code review. I'd appreciate help on my to do items, especially the last one. To do: - simplify user portion of upload_statistics - add revision deleting to upload_delete - figure out how to take revisions into account in queries (NOTE: the query in upload_total_space_used() is apparently wrong); right now, each revision just adds to totals or shows as an additional item in the 'my uploads' list Done: - changed a lot of double quotes to single quotes - added lots of t()s - greatly simplified admin portion of upload_statistics - moved disk space usage list to a separate tab - made disk space usage list show all users in a sortable table - fixed some comment typos - just return from menu callbacks instead of using theme('page') ------------------------------------------------------------------------ Thu, 15 Sep 2005 00:20:10 +0000 : moshe weitzman In upload_total_space_used(), it looks like your query will give right results if you JOIN to the node table instead of node_revisions. same for your "too many items in list" problem. I didn't test these theories, just perused the patch. ------------------------------------------------------------------------ Thu, 15 Sep 2005 21:53:43 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload9.patch (25.75 KB) And another update: - Fixed help texts so they actually display - Added new help texts - Split out admin statistics to upload_admin, since it was only three lines - Fixed a bug introduced in the last patch related to the display of per user statistics The problem in upload_total_space_used() and other queries I'm adding need to be addressed by changes to the files table, IMHO. That, and the changes for deleting revisions, will have to wait for a revision bug fix patch. That leaves cleaning up upload_statistics. ------------------------------------------------------------------------ Thu, 15 Sep 2005 23:01:20 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload10.patch (26.01 KB) OK, all done. upload_statistics() has been cleansed. ------------------------------------------------------------------------ Thu, 15 Sep 2005 23:19:19 +0000 : m3avrck Dries, just a heads up that Junyor, Souvent, and myself have been discussing this issue all day in #upload ... many hours went into this patch, all of your concerns have been addressed, along with many others we have discovered. This patch is rock solid and we'll be posting one final patch very soon that completes this. Better administrative options is all this patch introduces, no changes to how files are handled. Also, this patch uncovered 2 bugs inherent to revisions that were overlooked so this patch has been very worthwhile :) ------------------------------------------------------------------------ Fri, 16 Sep 2005 00:53:35 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/upload.module_6.patch (26.79 KB) Ok, fixed a few things. From the top now: - Fixed the quries that were returning incorrect information. Namley, disk usage per user, and total disk usage. - Added a new function to calcuate the number of files a user has. - Formatting issues. Now, there is a serperate issue, which should be addressed in a seperate bug, which is the current layout of the file table. I believe a second table (associatve table) should be used to link revisions to files, instead of lots of redundant data in the files table. This is a normalization issue. However, I am using the system as is, and have adjusted the quries, etc. to fit. ------------------------------------------------------------------------ Fri, 16 Sep 2005 09:29:28 +0000 : stefan nagtegaal Hi guys! Sorry for jumping in so lately on the threat, but I reviewed the code and I must say that I like what I see.. Though, some problems code-wise imo: <?php +      form_set_error('', t('Please select some items to perform the update on.')); ?> Why is the first argument not set? IMO this should always be set, otherwise use drupal_set_message('error', $message). Secondly you guys are using: <?php +  $output .= theme('box', t('Statistics'), $content); ?> By the 'introduce theme wrapper function [1]' threath, comment #37 Dries encourages people to remove the usage of theme('box').. So, looking at the future you guys can probably do the same? Try not using the theme('box'), but try something else... For what's left, I love the functionality!!! Very, very good work.. [1] http://drupal.org/node/23584 ------------------------------------------------------------------------ Fri, 16 Sep 2005 11:18:02 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload11.patch (25.85 KB) I agree with Souvent that the revision information should be moved to another table. It's causing a lot of excess data in the files table. Also, most of the queries related to revisions in upload.module are currently broken. My patch uses the queries as they should be given this change in the file table. @Stefan: I copied the form_set_error() stuff from node.module, which also doesn't set the first parameter. I've looked at the way several functions use form_set_error() and the first parameter seems to be the name of the form that must be corrected. According to nodeapi_example_nodeapi[1], "the user must correct the error before being able to submit the [form]." I don't think that should apply to this form, just as it doesn't apply to the update form on the node admin page. Is there a good alternative to theme('box')? [1] http://drupaldocs.org/api/head/function/nodeapi_example_nodeapi @Souvent: I'm not sure all those changes are necessary. 1) You added a separate upload_file_count function which does an additional query. All the file count and size information comes from one query in my version, which I think is faster. 2) You removed some commented out code: cool. 3) You changed the number of entries on the file list page from 50 to 25. The node, comment, and watchdog overview pages all use 50, so I was just going for consistency. 4) You updated a bunch of queries to work-around bugs introduced by the revision patch. I'd like to fix the problems with the revision patch in a separate patch instead of adding work-arounds. 5) You changed the way upload_admin_usage gathers space usage about users. Instead of a single query, there's now a query per user. That's a bit expensive, isn't it? 6) The query in upload_admin_usage() was indeed broken. However, I seemed to be able to fix it by doing the group by on u.uid instead of f.fid. I've attached a new patch with #2 and #6 addressed. ------------------------------------------------------------------------ Fri, 16 Sep 2005 14:11:38 +0000 : m3avrck Junyor, Souvent and I spent many hours last night after you posted your patch fixing many many bugs. 1. Yes we have another count query but this one actually works ;-) count(*) has the same aggregate problems as sum() and because of the nature of the files table, will *only* work this way without subselects. either the tables get changed around a bit or subselects be allowed. since the later assumes everyone is using MySQL 4.1 seesm like the former is the answer. We have already created an issue for this: http://drupal.org/node/31354 In addition, this patch *won't* get in if it assumes that another patch will to fix the table, this is not good. Souvent and myself have already committed ourselves to this patch and the other patch and will update things accordingly. If our propsed patch gets in with the correct queries, we'll post a patch later to fix the files table. Once that patch goes in, we'll post a patch to "re-fix" the uploads module to get it up to speed. Sure this is double work, but this is the best method and only way we can get this patch in now. We'll take care of it :) 3. We can change these back to 50, was just a bit too many in our opinions but consisently rules out. 4. We didn't introduce any "workarounds". Rather, we got it working as is :) Once the updated revisions patches go in: http://drupal.org/node/31355 we'll accordingly refactor this patch. Again, this patch can't wait for others, otherwise it won't go in. 5. Again, same problem with subselects. 6. We already fixed it. Again, the biggest problem is the fact that Drupal does *not* support subselects, hence we have to be creative in our queries :) Sure a better files table would help, *however* this won't necessarily fix all of the queries magically. I still see the subselects coming into play. We rigorously tested all of the queries last night and they do indeed your work. Your patch was broken in many cases because of this. We can roll another patch addressing Stefan's concerns. ------------------------------------------------------------------------ Fri, 16 Sep 2005 14:48:34 +0000 : m3avrck Stefan: Souvent, Junyor and myself all discussed your ideas. The first, form_error('') is indeed valid. The content tab in admin uses the same interface setup and uses the function in the exact same way. Check node.module to see this. Second, while we agree with the theme('box') issue, the rest of the Drupal still uses theme('box') in the same type of ways. Since there is no "better" way to do this, we went with consisentancy since we can't come up with a better alternative. Also, we have all reached an agreement, this patch does indeed work and works *very* well. The issues with files and revisions is completely seperate. This patch works 100% and if later on their are core changes to files and revisions, we'll update upload.module accordingly (which we hope to do too :)). New patch in a bit to fix the other outstanding problems. ------------------------------------------------------------------------ Fri, 16 Sep 2005 15:38:59 +0000 : m3avrck Attachment: http://drupal.org/files/issues/upload.module_7.patch (27.36 KB) Updated patch, addressed remaining issues and cleaned up a few small things. Ready to go from this side! ------------------------------------------------------------------------ Fri, 16 Sep 2005 15:43:04 +0000 : m3avrck Attachment: http://drupal.org/files/issues/upload.module_7_0.patch (27.35 KB) Small typo in query just discovered, fixed now. ------------------------------------------------------------------------ Fri, 16 Sep 2005 17:34:26 +0000 : Souvent22 Looks solid, and works solid. +1. This is a big help the the uplaods arena. I also like how this patch has been planned; regarding known issues (file table, etc.), and def. going to address those, but for now, it would be nice to have this functionality at a minnimum.

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: praseodym Status: patch (ready to be committed) Sounds useful to me too. Not having read all the comments, would it be possible to get some nice table of how much space users use? I have a site with a small 100 MB of uploads, and sometimes I want to know who uploaded the most. praseodym 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 :) ------------------------------------------------------------------------ Tue, 06 Sep 2005 21:34:11 +0000 : Junyor Changes are needed after the revision patch. I'm going to have to rethink upload_delete() a bit. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:50:19 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload6.patch (23.58 KB) Updated patch to HEAD. I've rolled in the changes from http://drupal.org/node/30025, as they intermingle. I also removed some extraneous whitespace at the end of lines. I'm still hoping to get this into 4.7. ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:21:17 +0000 : m3avrck Settings > Uploads shows the same thing as Uploads ... looks like the settings from the other patch weren't moved over correctly? ------------------------------------------------------------------------ Tue, 13 Sep 2005 22:19:46 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload7.patch (23.92 KB) Indeed. Fixed. ------------------------------------------------------------------------ Wed, 14 Sep 2005 02:10:13 +0000 : m3avrck Attachment: http://drupal.org/files/issues/upload.module_5.patch (24.39 KB) Patch works great! Tested out all sorts of circumstances, patch works flawlessly. This is *much* needed functionality in core. This patch doesn't actually add anything that isn't in core, rather it just makes this information readily available which is *very* helpful. Much needed and I think because of that this should go in 4.7. Rerolled patch to fix a usability issue on Settings > Upload ... using form_group_collapasible() cleans up page when you have more than roll with file permissions. ------------------------------------------------------------------------ Wed, 14 Sep 2005 17:39:02 +0000 : Souvent22 +1. Yes yes. patch worked well. Really like the statistics feature, and the planned extensibility with the drop-down action. hope this makes it in. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:31:58 +0000 : Dries Souvent22, m3avrck: please review patches more carefully! This patch needs a lot of work: 1. Many strings can't be translated. 2. Why do we sort the data in PHP and not in MySQL. 3. Why do we retrieve all records in MySQL but not show them all. Why do we show 10 users only? Problem (2) and (3) should be fixed by creating a table that can be (i) sorted and (ii) paged. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:34:24 +0000 : m3avrck Dries didn't really look through the code too much, my fault! Just tested to make sure it *actually* works. Will do that tomorrow morning, unless Junyor can patch it before me :) ------------------------------------------------------------------------ Wed, 14 Sep 2005 23:59:54 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload8.patch (25.37 KB) This is as far as I'm getting tonight. Not everything is fully tested, but I believe I've addressed Dries' concerns. However, I've come up with a couple additional problems during testing and in code review. I'd appreciate help on my to do items, especially the last one. To do: - simplify user portion of upload_statistics - add revision deleting to upload_delete - figure out how to take revisions into account in queries (NOTE: the query in upload_total_space_used() is apparently wrong); right now, each revision just adds to totals or shows as an additional item in the 'my uploads' list Done: - changed a lot of double quotes to single quotes - added lots of t()s - greatly simplified admin portion of upload_statistics - moved disk space usage list to a separate tab - made disk space usage list show all users in a sortable table - fixed some comment typos - just return from menu callbacks instead of using theme('page') ------------------------------------------------------------------------ Thu, 15 Sep 2005 00:20:10 +0000 : moshe weitzman In upload_total_space_used(), it looks like your query will give right results if you JOIN to the node table instead of node_revisions. same for your "too many items in list" problem. I didn't test these theories, just perused the patch. ------------------------------------------------------------------------ Thu, 15 Sep 2005 21:53:43 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload9.patch (25.75 KB) And another update: - Fixed help texts so they actually display - Added new help texts - Split out admin statistics to upload_admin, since it was only three lines - Fixed a bug introduced in the last patch related to the display of per user statistics The problem in upload_total_space_used() and other queries I'm adding need to be addressed by changes to the files table, IMHO. That, and the changes for deleting revisions, will have to wait for a revision bug fix patch. That leaves cleaning up upload_statistics. ------------------------------------------------------------------------ Thu, 15 Sep 2005 23:01:20 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload10.patch (26.01 KB) OK, all done. upload_statistics() has been cleansed. ------------------------------------------------------------------------ Thu, 15 Sep 2005 23:19:19 +0000 : m3avrck Dries, just a heads up that Junyor, Souvent, and myself have been discussing this issue all day in #upload ... many hours went into this patch, all of your concerns have been addressed, along with many others we have discovered. This patch is rock solid and we'll be posting one final patch very soon that completes this. Better administrative options is all this patch introduces, no changes to how files are handled. Also, this patch uncovered 2 bugs inherent to revisions that were overlooked so this patch has been very worthwhile :) ------------------------------------------------------------------------ Fri, 16 Sep 2005 00:53:35 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/upload.module_6.patch (26.79 KB) Ok, fixed a few things. From the top now: - Fixed the quries that were returning incorrect information. Namley, disk usage per user, and total disk usage. - Added a new function to calcuate the number of files a user has. - Formatting issues. Now, there is a serperate issue, which should be addressed in a seperate bug, which is the current layout of the file table. I believe a second table (associatve table) should be used to link revisions to files, instead of lots of redundant data in the files table. This is a normalization issue. However, I am using the system as is, and have adjusted the quries, etc. to fit. ------------------------------------------------------------------------ Fri, 16 Sep 2005 09:29:28 +0000 : stefan nagtegaal Hi guys! Sorry for jumping in so lately on the threat, but I reviewed the code and I must say that I like what I see.. Though, some problems code-wise imo: <?php +      form_set_error('', t('Please select some items to perform the update on.')); ?> Why is the first argument not set? IMO this should always be set, otherwise use drupal_set_message('error', $message). Secondly you guys are using: <?php +  $output .= theme('box', t('Statistics'), $content); ?> By the 'introduce theme wrapper function [1]' threath, comment #37 Dries encourages people to remove the usage of theme('box').. So, looking at the future you guys can probably do the same? Try not using the theme('box'), but try something else... For what's left, I love the functionality!!! Very, very good work.. [1] http://drupal.org/node/23584 ------------------------------------------------------------------------ Fri, 16 Sep 2005 11:18:02 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload11.patch (25.85 KB) I agree with Souvent that the revision information should be moved to another table. It's causing a lot of excess data in the files table. Also, most of the queries related to revisions in upload.module are currently broken. My patch uses the queries as they should be given this change in the file table. @Stefan: I copied the form_set_error() stuff from node.module, which also doesn't set the first parameter. I've looked at the way several functions use form_set_error() and the first parameter seems to be the name of the form that must be corrected. According to nodeapi_example_nodeapi[1], "the user must correct the error before being able to submit the [form]." I don't think that should apply to this form, just as it doesn't apply to the update form on the node admin page. Is there a good alternative to theme('box')? [1] http://drupaldocs.org/api/head/function/nodeapi_example_nodeapi @Souvent: I'm not sure all those changes are necessary. 1) You added a separate upload_file_count function which does an additional query. All the file count and size information comes from one query in my version, which I think is faster. 2) You removed some commented out code: cool. 3) You changed the number of entries on the file list page from 50 to 25. The node, comment, and watchdog overview pages all use 50, so I was just going for consistency. 4) You updated a bunch of queries to work-around bugs introduced by the revision patch. I'd like to fix the problems with the revision patch in a separate patch instead of adding work-arounds. 5) You changed the way upload_admin_usage gathers space usage about users. Instead of a single query, there's now a query per user. That's a bit expensive, isn't it? 6) The query in upload_admin_usage() was indeed broken. However, I seemed to be able to fix it by doing the group by on u.uid instead of f.fid. I've attached a new patch with #2 and #6 addressed. ------------------------------------------------------------------------ Fri, 16 Sep 2005 14:11:38 +0000 : m3avrck Junyor, Souvent and I spent many hours last night after you posted your patch fixing many many bugs. 1. Yes we have another count query but this one actually works ;-) count(*) has the same aggregate problems as sum() and because of the nature of the files table, will *only* work this way without subselects. either the tables get changed around a bit or subselects be allowed. since the later assumes everyone is using MySQL 4.1 seesm like the former is the answer. We have already created an issue for this: http://drupal.org/node/31354 In addition, this patch *won't* get in if it assumes that another patch will to fix the table, this is not good. Souvent and myself have already committed ourselves to this patch and the other patch and will update things accordingly. If our propsed patch gets in with the correct queries, we'll post a patch later to fix the files table. Once that patch goes in, we'll post a patch to "re-fix" the uploads module to get it up to speed. Sure this is double work, but this is the best method and only way we can get this patch in now. We'll take care of it :) 3. We can change these back to 50, was just a bit too many in our opinions but consisently rules out. 4. We didn't introduce any "workarounds". Rather, we got it working as is :) Once the updated revisions patches go in: http://drupal.org/node/31355 we'll accordingly refactor this patch. Again, this patch can't wait for others, otherwise it won't go in. 5. Again, same problem with subselects. 6. We already fixed it. Again, the biggest problem is the fact that Drupal does *not* support subselects, hence we have to be creative in our queries :) Sure a better files table would help, *however* this won't necessarily fix all of the queries magically. I still see the subselects coming into play. We rigorously tested all of the queries last night and they do indeed your work. Your patch was broken in many cases because of this. We can roll another patch addressing Stefan's concerns. ------------------------------------------------------------------------ Fri, 16 Sep 2005 14:48:34 +0000 : m3avrck Stefan: Souvent, Junyor and myself all discussed your ideas. The first, form_error('') is indeed valid. The content tab in admin uses the same interface setup and uses the function in the exact same way. Check node.module to see this. Second, while we agree with the theme('box') issue, the rest of the Drupal still uses theme('box') in the same type of ways. Since there is no "better" way to do this, we went with consisentancy since we can't come up with a better alternative. Also, we have all reached an agreement, this patch does indeed work and works *very* well. The issues with files and revisions is completely seperate. This patch works 100% and if later on their are core changes to files and revisions, we'll update upload.module accordingly (which we hope to do too :)). New patch in a bit to fix the other outstanding problems. ------------------------------------------------------------------------ Fri, 16 Sep 2005 15:38:59 +0000 : m3avrck Attachment: http://drupal.org/files/issues/upload.module_7.patch (27.36 KB) Updated patch, addressed remaining issues and cleaned up a few small things. Ready to go from this side! ------------------------------------------------------------------------ Fri, 16 Sep 2005 15:43:04 +0000 : m3avrck Attachment: http://drupal.org/files/issues/upload.module_7_0.patch (27.35 KB) Small typo in query just discovered, fixed now. ------------------------------------------------------------------------ Fri, 16 Sep 2005 17:34:26 +0000 : Souvent22 Looks solid, and works solid. +1. This is a big help the the uplaods arena. I also like how this patch has been planned; regarding known issues (file table, etc.), and def. going to address those, but for now, it would be nice to have this functionality at a minnimum. ------------------------------------------------------------------------ Sun, 18 Sep 2005 17:21:22 +0000 : Cvbge Hello, I've scanned the patch and you need to change $result = db_query('SELECT fid,filesize FROM {files} f INNER JOIN {node} n ON f.nid = n.nid WHERE n.uid = %d GROUP BY fid', $uid); in upload_space_used() to $result = db_query('SELECT fid,filesize FROM {files} f INNER JOIN {node} n ON f.nid = n.nid WHERE n.uid = %d GROUP BY fid, filesize', $uid); (i.e. add filesize to GROUP statement. Otherwise it won't work in postgres. This should not change anything, as all rows with the same fid have the same filesize (if I understand the meaning of fid correctly)). The same goes to $result = db_query('SELECT fid,filesize FROM {files} GROUP BY fid'); in upload_total_space_used() - needs filesize in GROUP BY. ------------------------------------------------------------------------ Sun, 18 Sep 2005 17:52:23 +0000 : m3avrck Attachment: http://drupal.org/files/issues/upload.module_7_0_0.patch (27.37 KB) Updated SQL as per Cvbge's comments.

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: praseodym Status: patch (ready to be committed) See the above comment as a +1 and some future ideas :). praseodym 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 :) ------------------------------------------------------------------------ Tue, 06 Sep 2005 21:34:11 +0000 : Junyor Changes are needed after the revision patch. I'm going to have to rethink upload_delete() a bit. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:50:19 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload6.patch (23.58 KB) Updated patch to HEAD. I've rolled in the changes from http://drupal.org/node/30025, as they intermingle. I also removed some extraneous whitespace at the end of lines. I'm still hoping to get this into 4.7. ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:21:17 +0000 : m3avrck Settings > Uploads shows the same thing as Uploads ... looks like the settings from the other patch weren't moved over correctly? ------------------------------------------------------------------------ Tue, 13 Sep 2005 22:19:46 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload7.patch (23.92 KB) Indeed. Fixed. ------------------------------------------------------------------------ Wed, 14 Sep 2005 02:10:13 +0000 : m3avrck Attachment: http://drupal.org/files/issues/upload.module_5.patch (24.39 KB) Patch works great! Tested out all sorts of circumstances, patch works flawlessly. This is *much* needed functionality in core. This patch doesn't actually add anything that isn't in core, rather it just makes this information readily available which is *very* helpful. Much needed and I think because of that this should go in 4.7. Rerolled patch to fix a usability issue on Settings > Upload ... using form_group_collapasible() cleans up page when you have more than roll with file permissions. ------------------------------------------------------------------------ Wed, 14 Sep 2005 17:39:02 +0000 : Souvent22 +1. Yes yes. patch worked well. Really like the statistics feature, and the planned extensibility with the drop-down action. hope this makes it in. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:31:58 +0000 : Dries Souvent22, m3avrck: please review patches more carefully! This patch needs a lot of work: 1. Many strings can't be translated. 2. Why do we sort the data in PHP and not in MySQL. 3. Why do we retrieve all records in MySQL but not show them all. Why do we show 10 users only? Problem (2) and (3) should be fixed by creating a table that can be (i) sorted and (ii) paged. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:34:24 +0000 : m3avrck Dries didn't really look through the code too much, my fault! Just tested to make sure it *actually* works. Will do that tomorrow morning, unless Junyor can patch it before me :) ------------------------------------------------------------------------ Wed, 14 Sep 2005 23:59:54 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload8.patch (25.37 KB) This is as far as I'm getting tonight. Not everything is fully tested, but I believe I've addressed Dries' concerns. However, I've come up with a couple additional problems during testing and in code review. I'd appreciate help on my to do items, especially the last one. To do: - simplify user portion of upload_statistics - add revision deleting to upload_delete - figure out how to take revisions into account in queries (NOTE: the query in upload_total_space_used() is apparently wrong); right now, each revision just adds to totals or shows as an additional item in the 'my uploads' list Done: - changed a lot of double quotes to single quotes - added lots of t()s - greatly simplified admin portion of upload_statistics - moved disk space usage list to a separate tab - made disk space usage list show all users in a sortable table - fixed some comment typos - just return from menu callbacks instead of using theme('page') ------------------------------------------------------------------------ Thu, 15 Sep 2005 00:20:10 +0000 : moshe weitzman In upload_total_space_used(), it looks like your query will give right results if you JOIN to the node table instead of node_revisions. same for your "too many items in list" problem. I didn't test these theories, just perused the patch. ------------------------------------------------------------------------ Thu, 15 Sep 2005 21:53:43 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload9.patch (25.75 KB) And another update: - Fixed help texts so they actually display - Added new help texts - Split out admin statistics to upload_admin, since it was only three lines - Fixed a bug introduced in the last patch related to the display of per user statistics The problem in upload_total_space_used() and other queries I'm adding need to be addressed by changes to the files table, IMHO. That, and the changes for deleting revisions, will have to wait for a revision bug fix patch. That leaves cleaning up upload_statistics. ------------------------------------------------------------------------ Thu, 15 Sep 2005 23:01:20 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload10.patch (26.01 KB) OK, all done. upload_statistics() has been cleansed. ------------------------------------------------------------------------ Thu, 15 Sep 2005 23:19:19 +0000 : m3avrck Dries, just a heads up that Junyor, Souvent, and myself have been discussing this issue all day in #upload ... many hours went into this patch, all of your concerns have been addressed, along with many others we have discovered. This patch is rock solid and we'll be posting one final patch very soon that completes this. Better administrative options is all this patch introduces, no changes to how files are handled. Also, this patch uncovered 2 bugs inherent to revisions that were overlooked so this patch has been very worthwhile :) ------------------------------------------------------------------------ Fri, 16 Sep 2005 00:53:35 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/upload.module_6.patch (26.79 KB) Ok, fixed a few things. From the top now: - Fixed the quries that were returning incorrect information. Namley, disk usage per user, and total disk usage. - Added a new function to calcuate the number of files a user has. - Formatting issues. Now, there is a serperate issue, which should be addressed in a seperate bug, which is the current layout of the file table. I believe a second table (associatve table) should be used to link revisions to files, instead of lots of redundant data in the files table. This is a normalization issue. However, I am using the system as is, and have adjusted the quries, etc. to fit. ------------------------------------------------------------------------ Fri, 16 Sep 2005 09:29:28 +0000 : stefan nagtegaal Hi guys! Sorry for jumping in so lately on the threat, but I reviewed the code and I must say that I like what I see.. Though, some problems code-wise imo: <?php +      form_set_error('', t('Please select some items to perform the update on.')); ?> Why is the first argument not set? IMO this should always be set, otherwise use drupal_set_message('error', $message). Secondly you guys are using: <?php +  $output .= theme('box', t('Statistics'), $content); ?> By the 'introduce theme wrapper function [1]' threath, comment #37 Dries encourages people to remove the usage of theme('box').. So, looking at the future you guys can probably do the same? Try not using the theme('box'), but try something else... For what's left, I love the functionality!!! Very, very good work.. [1] http://drupal.org/node/23584 ------------------------------------------------------------------------ Fri, 16 Sep 2005 11:18:02 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-cvs.junyor.upload11.patch (25.85 KB) I agree with Souvent that the revision information should be moved to another table. It's causing a lot of excess data in the files table. Also, most of the queries related to revisions in upload.module are currently broken. My patch uses the queries as they should be given this change in the file table. @Stefan: I copied the form_set_error() stuff from node.module, which also doesn't set the first parameter. I've looked at the way several functions use form_set_error() and the first parameter seems to be the name of the form that must be corrected. According to nodeapi_example_nodeapi[1], "the user must correct the error before being able to submit the [form]." I don't think that should apply to this form, just as it doesn't apply to the update form on the node admin page. Is there a good alternative to theme('box')? [1] http://drupaldocs.org/api/head/function/nodeapi_example_nodeapi @Souvent: I'm not sure all those changes are necessary. 1) You added a separate upload_file_count function which does an additional query. All the file count and size information comes from one query in my version, which I think is faster. 2) You removed some commented out code: cool. 3) You changed the number of entries on the file list page from 50 to 25. The node, comment, and watchdog overview pages all use 50, so I was just going for consistency. 4) You updated a bunch of queries to work-around bugs introduced by the revision patch. I'd like to fix the problems with the revision patch in a separate patch instead of adding work-arounds. 5) You changed the way upload_admin_usage gathers space usage about users. Instead of a single query, there's now a query per user. That's a bit expensive, isn't it? 6) The query in upload_admin_usage() was indeed broken. However, I seemed to be able to fix it by doing the group by on u.uid instead of f.fid. I've attached a new patch with #2 and #6 addressed. ------------------------------------------------------------------------ Fri, 16 Sep 2005 14:11:38 +0000 : m3avrck Junyor, Souvent and I spent many hours last night after you posted your patch fixing many many bugs. 1. Yes we have another count query but this one actually works ;-) count(*) has the same aggregate problems as sum() and because of the nature of the files table, will *only* work this way without subselects. either the tables get changed around a bit or subselects be allowed. since the later assumes everyone is using MySQL 4.1 seesm like the former is the answer. We have already created an issue for this: http://drupal.org/node/31354 In addition, this patch *won't* get in if it assumes that another patch will to fix the table, this is not good. Souvent and myself have already committed ourselves to this patch and the other patch and will update things accordingly. If our propsed patch gets in with the correct queries, we'll post a patch later to fix the files table. Once that patch goes in, we'll post a patch to "re-fix" the uploads module to get it up to speed. Sure this is double work, but this is the best method and only way we can get this patch in now. We'll take care of it :) 3. We can change these back to 50, was just a bit too many in our opinions but consisently rules out. 4. We didn't introduce any "workarounds". Rather, we got it working as is :) Once the updated revisions patches go in: http://drupal.org/node/31355 we'll accordingly refactor this patch. Again, this patch can't wait for others, otherwise it won't go in. 5. Again, same problem with subselects. 6. We already fixed it. Again, the biggest problem is the fact that Drupal does *not* support subselects, hence we have to be creative in our queries :) Sure a better files table would help, *however* this won't necessarily fix all of the queries magically. I still see the subselects coming into play. We rigorously tested all of the queries last night and they do indeed your work. Your patch was broken in many cases because of this. We can roll another patch addressing Stefan's concerns. ------------------------------------------------------------------------ Fri, 16 Sep 2005 14:48:34 +0000 : m3avrck Stefan: Souvent, Junyor and myself all discussed your ideas. The first, form_error('') is indeed valid. The content tab in admin uses the same interface setup and uses the function in the exact same way. Check node.module to see this. Second, while we agree with the theme('box') issue, the rest of the Drupal still uses theme('box') in the same type of ways. Since there is no "better" way to do this, we went with consisentancy since we can't come up with a better alternative. Also, we have all reached an agreement, this patch does indeed work and works *very* well. The issues with files and revisions is completely seperate. This patch works 100% and if later on their are core changes to files and revisions, we'll update upload.module accordingly (which we hope to do too :)). New patch in a bit to fix the other outstanding problems. ------------------------------------------------------------------------ Fri, 16 Sep 2005 15:38:59 +0000 : m3avrck Attachment: http://drupal.org/files/issues/upload.module_7.patch (27.36 KB) Updated patch, addressed remaining issues and cleaned up a few small things. Ready to go from this side! ------------------------------------------------------------------------ Fri, 16 Sep 2005 15:43:04 +0000 : m3avrck Attachment: http://drupal.org/files/issues/upload.module_7_0.patch (27.35 KB) Small typo in query just discovered, fixed now. ------------------------------------------------------------------------ Fri, 16 Sep 2005 17:34:26 +0000 : Souvent22 Looks solid, and works solid. +1. This is a big help the the uplaods arena. I also like how this patch has been planned; regarding known issues (file table, etc.), and def. going to address those, but for now, it would be nice to have this functionality at a minnimum. ------------------------------------------------------------------------ Sun, 18 Sep 2005 17:21:22 +0000 : Cvbge Hello, I've scanned the patch and you need to change $result = db_query('SELECT fid,filesize FROM {files} f INNER JOIN {node} n ON f.nid = n.nid WHERE n.uid = %d GROUP BY fid', $uid); in upload_space_used() to $result = db_query('SELECT fid,filesize FROM {files} f INNER JOIN {node} n ON f.nid = n.nid WHERE n.uid = %d GROUP BY fid, filesize', $uid); (i.e. add filesize to GROUP statement. Otherwise it won't work in postgres. This should not change anything, as all rows with the same fid have the same filesize (if I understand the meaning of fid correctly)). The same goes to $result = db_query('SELECT fid,filesize FROM {files} GROUP BY fid'); in upload_total_space_used() - needs filesize in GROUP BY. ------------------------------------------------------------------------ Sun, 18 Sep 2005 17:52:23 +0000 : m3avrck Attachment: http://drupal.org/files/issues/upload.module_7_0_0.patch (27.37 KB) Updated SQL as per Cvbge's comments. ------------------------------------------------------------------------ Sun, 18 Sep 2005 20:17:41 +0000 : praseodym Sounds useful to me too. Not having read all the comments, would it be possible to get some nice table of how much space users use? I have a site with a small 100 MB of uploads, and sometimes I want to know who uploaded the most. ------------------------------------------------------------------------ Sun, 18 Sep 2005 20:40:39 +0000 : m3avrck praseodym, already built in this patch, there is a tab with basic file upload statistics right now :-) hope to add more statistical information later but this patch sets the stage for everything.