When attaching an already existing file to a node, the file is overwritten on the filesystem but duplicated in db
telcontar - May 21, 2006 - 11:25
| Project: | Attachment |
| Version: | 5.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | telcontar |
| Status: | closed |
Description
I just checked for this bug in the newest versions of filemanager and attachment modules under 4.6.6.
To reproduce this bug, please follow the following steps (do not 'hide' attachments):
- Attach a file, e.g. "test.txt", to a node
- Submit the attachment - take note of the filesize
- Edit the node and attach another file of the same name (presumably different filesize)
- Submit the node
The following happens:
- both attachments are in the db and are displayed when you view the node
- there is really only one file: the latter one (old one was overwritten)
If one edits the node and deletes only ONE of those two attachments:
- The file is deleted from the filesystem
- Because one database record is still present, it is still displayed in node view - with a broken link
One would expect that when attaching an already-existing file to a node, the old file is overwritten or the user is asked whether he really wants to overwrite, or the upload is aborted with an error message.
Attached file addresses this problem in the following way:
- Check if a file with that name is already attached to that node - if this is the case, update the existing record and overwrite the old file
- Delete the attachment only if there is exaclty one attachment of that name attached to the node. This is only to help users already affected by this problem - when they delete only one record, they might not want the file deleted from the filesystem.
Please review. Maybe this behaviour is not what you want.
| Attachment | Size |
|---|---|
| attachment-4.6.0.patch.txt | 1.29 KB |
#1
I posted this patch almost two months ago - can anyone confirm that they also have this problem? I tested this behaviour on several different machines. Project maintainer / anyone ?
Johannes
#2
hey, i just took over this module. i'm not sure quite what your patch does because the code it changes still exists in the 4.7 branch. i committed a few backports today to try to merge the two branches back together. i guess what i'm unsure of is when would the 'aid' value be false? why do we need to re-load from the database?
#3
This bug is still present in 5.x-1.x. When you attach a file that's already attached to a node (overwrite, as it were), the file is overwritten on the filesystem but entries are duplicated in the "attachment" table instead of updated. E.g. you have two entries for the same file, which is certainly wrong. The "size" field is also not updated in filemanager.
My "patch" was more of a hack to illustrate a point, really ;-)
I'm surprised no one but me ever ran into this? Doesn't anyone ever re-attach a newer version of a file to a node? :-)
#4
This patch should fix the bug. The problem was that in hook_nodeapi, $attachment['aid'] was not set when overwriting an already existing file. I do not think that UPDATE query has ever been executed for the 4.6, 4.7 or 5.x branch of this module.
This patch should also be applied to the 4.7 branch.
#5
I marked http://drupal.org/node/173391 as a duplicate of this issue, because this one is older and provides a patch.
#6
+1 the patch works and should be added to the next release. However, I'm a recycled usability engineer and I think we need to either add a confirmation so users understand they might lose the existing file or append a numeral to the file and add it to the node as another attachment. I'm probably going to code it for my users anyway, but how would you feel about the change in functionality?
#7
That's a good point, but how would you go about warning the user about the overwrite in the UI? A confirmation form using confirm_form()?
As this is a bug and you're proposing a change in functionality, I think you should open that as a separate "feature request" issue... And I can't speak for the maintainer but if you were to code it and provide a patch, I'm sure he'd be happy for your help!
#8
Yeah, I think confirm_form would be fine. I'm still deciding on how to handle it for my site-- I haven't figured out if the "right" answer is to just confirm the overwrite or if I want to go ahead and add another instance of the file. Once I do, i'll definitely submit it as a patch.
#9
I'd say the former, because that's what people would generally expect, i.e. you can't have the same file name twice in a folder on any filesystem that I know of.
I know you could add a "-1" etc but I think that becomes complicated and confusing rather quickly. I think simplicity should be one of the key elements of UI design, and if the user doesn't want to override, let them rename the file first.
If you must give the user the option of adding another instance, forget confirm_form() and write a custom form instead, where you give the user the options to overwrite / add another instance with numeral / cancel, with "overwrite" as the default.
What do you think?
-- telcontar
#10
Yep, I agree. It's simpler, cleaner, and you're right-- people are already used to that behavior. I hadn't considered that. I'll check out the confirm_form function and see if I can get a patch rolled up.
#11
Fixed and committed. I back-ported to 4.7, someone please test it there.
#12
Automatically closed -- issue fixed for two weeks with no activity.