backdrop
[UX] Allow people to overwrite existing files without visiting 'Manage files'
I'm logging this as a potential bug because the way I'm thinking it should work is different than what I'm experiencing and I'd like to understand why it works the way it does, and possibly discuss changing to how I think it should work.
Scenario 1
- Upload a new file to a file or image field.
- Immediately remove the file.
- Re-upload the same file.
Note that the resulting filename in step 1 and 3 are the same, which would indicate that the first file is actually removed from the system and re-uploaded.
Scenario 2
- Upload a new file to a file or image field.
- Save the node.
- Edit the node.
- Remove the file you uploaded in step 1.
- Re-upload the same file.
Note that the resulting filename in step 1 and step 5 are different. Re-uploading the file appends _0 to the filename within the files directory and the original file was never removed from the file system.
Also note that even after a node save in between step 4 and 5, the file that is re-uploaded is renamed and the original never deleted.
Expectation
I would have expected that during step 4 in scenario 2 that the file would be removed from the file system as well as the database and therefor upon re-upload of a file with the same name that the file name not change. Otherwise, how else are files ever deleted?
I would have expected that during step 4 in scenario 2 that the file would be removed from the file system as well as the database and therefor upon re-upload of a file with the same name that the file name not change.
The file is not deleted when you click "Remove". It's deleted when you click "Save" on the node. Otherwise if a user edits a node, removes a file, but doesn't click save (closes the window instead), no changes should be made and the file should remain.
So while the file is removed from the node form but not deleted from disk, when a second file is uploaded with the same name, there are two files at the same location, and _0 gets attached to the end of the second one.
Otherwise, how else are files ever deleted?
The original file is deleted when the node is saved.
I've thought about how to solve this problem several times, but I've always come up short. How can you deal with two files that need to be at the same location at the same time?
I'm glad to know it's supposed to work that way, but it would seem this might be a bug since it's not removing the file upon a node save.
My second note under scenario 2:
Also note that even after a node save in between step 4 and 5, the file that is re-uploaded is renamed and the original never deleted.
So that's one issue. Not sure If perhaps I should split that out into a new issue or if I should log a new issue for this next part. Let me know what you want.
As for the second (overwriting a file), it I wonder if a new button could be introduced beside "Remove" called "Overwrite" which could remove the icon/thumbnail and file information displayed and replace it with a new upload widget that would specifically remove the existing file and allow the upload of a new one. Behind the scene it would actually upload a new file with the appended _0 but flag it as an overwrite in the database so that when node_save happened, it could then delete the first file and rename the new file using the intended filename which is currently stored in the filename field of the files_managed table.
Edit: looks like the function file_copy() has a flag for replacing, so perhaps that should be invoked for "Overwrite" by setting something in the upload widget that is presented after you click the "Overwrite" button and then the submit handler takes if from there. (Sorry, my lack of core is probably showing.)
but it would seem this might be a bug since it's not removing the file upon a node save.
Ah, I think this behavior changed in early D8/Backdrop. The file instead is marked "temporary" in the database and cleaned up on cron jobs. This makes it so you can delete thousands of nodes and not have to worry about the relatively expensive file cleanup during the current PHP request.
Behind the scene it would actually upload a new file with the appended _0 but flag it as an overwrite in the database so that when node_save happened
The problem with this is if the image is embedded into a WYSIWYG, the current path would have to be used to display the image correctly. Of course, the "right" path is still the old file. So you'd embed the "wrong" path (with _0 appended) in the img tag in the WYSIWYG, but then on save, the file location would be moved and you'd end up with a broken image :confused: You'd have to modify the body field to update any references to the _0 path.
This module uses a novel solution of simply swapping the file location places: https://www.drupal.org/project/upload_replace, that seems like something that might work and has probably been tested (around 1000 installs) for any odd side-effects caused by this approach.
Ah, ok. I see now that it does delete on cron. Thank you.
Also I'll give that module a go. We can close this if you'd rather this was solved by contrib. I'll probably have better luck porting that module than modifying core anyway. :wink:
I think this would be a nice core improvement. The file renaming problem has been plaguing us since 2007 at least when I was the maintainer of ImageField for D5/6.
I'm not sure the contrib module is free of security holes... it seems like you could accidentally (or intentionally) replace a file owned by a different node or user just by naming it the same thing. Tread with caution. :wink:
...it seems like you could accidentally (or intentionally) replace a file owned by a different node or user just by naming it the same thing.
Other than that, the solution of appending _[increment] to old files instead of new ones seems like a perfect solution for this fundamental Drupal architectural issue. Do we have any way to track file ownership (for example through the owner of the node to which the file is attached to)? If so, we could perhaps handle this with permissions ("replace own files"). Or would this be easier if files were entities somehow (File Entity (Fieldable Files))?
Just brainstorming here, but this is a thing to consider for when we plan for Media in core :wink:
...anyways, since this will most likely be a major API change, setting for 2.x seems more appropriate I believe.
I think the workflow I'd like to work towards here is this:
- Choose a file to upload.
- If a duplicate is detect, alert the user and ask what they want to do: Rename or Overwrite.
So... I know this may not be the correct way to go about this, but after digging a while I noticed that within the file_managed_file_submit() function, it checks for status of a file before deleting it. I simply removed this check so that if you remove a file it actually deletes it... I'm not sure why that status check is even necessary.
So that totally backfired because it doesn't remove the field usages and the node still thinks there is a field in the field_data_ table for that field and throws errors when you try to save. Back do the drawing board.
My current working theory is that if we add a 'Replace' button alongside of the 'Delete' button, we could detect it's key in file_managed_file_submit() and somehow trigger whatever needs to happen there. Still trying to figure out what all needs to happen though. I think we want to delete the original file, remove all reference to it in whichever field_data_ table it happens to be within (though the comment in file_managed_file_submit() states that it's the implementing module's job), and provide a new field.
This seems like it would be all much easier if the 'Remove' button actually removed the file though. Then there would be no need for a new button and it wouldn't be confusing as to why when you click 'Remove' the file isn't actually removed.
After looking through core for occurrences of file_delete(), it would seem that files are pretty much never deleted actually.
file.module
- this only deletes a file if it's temporary (status == 0)
image.field.inc
- this only deletes a default image from the admin form
user.entity.inc
- this only deletes user pics so has nothing to do with fields
Seems like a bug to me if core never actually deletes files that are uploaded through fields. No?
A bug with core...who will you file it for?
Drupal 7? Will you need to check it if applies to Drupal 8 as well?
Backdrop?
Anyways, this is an interesting thread to read through this morning. Thanks for the effort!
This is for Backdrop. I looked through Drupal 7 and it's so different that I'm not even going there. And I'm not even prepared to look at Drupal 8 at this point.
Doesnt https://www.drupal.org/project/upload_replace solve this problem though? Add an arbitrary flag to old versions of the file and have them cleaned by cron?
That module only works on a node save, so I don't think it does. I think this should be solved before a node save (or at least that is the expectation from my users).
That module only works on a node save
You sure? It implements hook_file_update(). Quick look says that hook is called by file_save() which is called by file_move() which happens during filefield uploading a file, not just on node save.
If users are confused by the fact that both my_file and my_file_0 are still there, can we modify the process of renaming to call old files my_file_old or my_file_expired or something equally dramatic?
I put a dpm within an implementation of hook_file_update and got nothing back on removing a file or uploading a new one, but only on node save, so my assumption was that it's only called on node save. I'd be happy to be wrong. The module itself (with basic conversion to Backdrop) still gives me a fatal error on a node save.
EntityStorageException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'public://field/image/logoGraphics_0.jpg' for key 'uri' in NodeStorageController->save() (line 301 of /Users/sirkitree/repos/backdrop/core/modules/node/node.entity.inc).
I don't think the confusion is the _0 but the fact that the file they're trying to use is renamed at all. The assumption is that by clicking "Remove", the file is removed, like hitting delete on any traditional file system.
That being said, the typical workflow is that a user uploads a file, references it in their content and saves. Then goes back in to edit the node, replace the file, and hit save (without ever checking the file's name). So if on this save the old file is totally removed and the new file is renamed to the original filename of the file being uploaded, then that would suffice for this use case.
I'll try getting that module to work again and see if it actually accomplishes this.
btw, thank you for the ideas. I appreciate not feeling like I'm talking to myself :wink:
@klonos this is the issue you were talking about today. It's tagged with 2.x right now, though, it looks like you added that tag? I'm also going to add the [UX] indicator since the use-case described by @sirkitree in the last comment is the most common, and needs to be addressed.
