When replacing a file ( with the module opt-out option ), the previous file is not physically deleted.
The whole correct functionning of unused files and temporary files is blocked for years
https://www.drupal.org/project/drupal/issues/2821423
Could the module delete the file on replacement, instead of only change its state to unused ?
- When replacing a file and opt out the override
- the unused file is passed as temporary
- the file disappeared from the db after cron
- the file is not deleted physically after cron
changing the conf to $config['file.settings']['make_unused_managed_files_temporary'] = TRUE; has no effect
On the replacement submit button , delete physically the previous file
| Comment | File | Size | Author |
|---|---|---|---|
| #24 | media_entity_file_replace_replace_file_3255452_24.patch | 6.9 KB | flitt1 |
| #16 | media_entity_file_replace-3255452-16.patch | 6.94 KB | rosk0 |
| #13 | configurable-prompt-original-file-delete-3255452#13.patch | 6.61 KB | matoeil |
Comments
Comment #2
bkosborneCan you confirm that it works as expected when this module is completely uninstalled?
Comment #3
matoeil commentedwhen i have tested it recently , this was my conclusion
drupal default behaviour:
- When replacing a file
- the unused file is not passed as temporary
- the file is not deleted after cron
- it is because https://www.drupal.org/node/2891902
same with adding $config['file.settings']['make_unused_managed_files_temporary'] = TRUE;
- When replacing a file
- the unused file is passed as temporary
- the file is deleted after cron
same with the media entity file replace activated and having $config['file.settings']['make_unused_managed_files_temporary'] = TRUE;
- When replacing a file and opt out the override
- the unused file is passed as temporary
- the file disappeared from the db after cron
- the file is not deleted physically after cron
So it does not work as expected , no matter what.
The suppression of unused files can lead to dataloss as long as there is a bug with drupal on that matter.
That"s why i am suggestiing this "task issue"
Comment #4
bkosborneI'll see if I can reproduce
Comment #5
matoeil commentedThis would delete physically the original file on replacement with no override
Comment #6
bkosborneI was not able to reproduce this. After uploading a replacement file and opting out of the override, the original file remains but is marked as temporary (I have $config['file.settings']['make_unused_managed_files_temporary'] = TRUE;). It is eventually deleted by cron as expected.
I'm not sure if there is maybe some other combination of modules that is causing this issue for you?
You state "the file disappeared from the db after cron" and also "the file is not deleted physically after cron"
That is troublesome to hear. The process that deletes the record from the database is a Drupal core process. It will also delete the file at the same time it deleted the record from the database. Do you have anything in your Drupal watchdog logs?
Comment #7
matoeil commentedThe audit file module shows that it is not always the case.
But my point is that one should not use $config['file.settings']['make_unused_managed_files_temporary'] = TRUE; on production as long as there are bugs in drupal, related to the unused managed files.
https://www.drupal.org/project/drupal/issues/2821423
So the original files after replacement will never be deleted physically.
That s why i am suggesting this patch.
Comment #8
bkosborneOh I see. Okay. I think in this case we need to make this a configuration option. So we probably need a config form for the module now to allow site admins to toggle this behavior on and off. I think it probably is a good idea anyway, as many users probably DO want the old file deleted immediately.
Comment #9
matoeil commentedwould that do ?
Comment #10
matoeil commentedComment #13
matoeil commentedComment #14
maacl commentedNot exactly related, but I tried to find a solution in a different way and created this module: https://www.drupal.org/project/media_files_handler Would love to get some feedback or testing.
Comment #15
matoeil commentedComment #16
rosk0Re-roll for the latest module version.
No functional code changes, but removed typo in form of
:qfrom// Delete physically the original file.:qand fixed up code styling for new changes.All credits should go to @matoeil .
Comment #17
chris matthews commentedYes, 💯
Comment #18
bkosborneI'm concerned that the scope of this otherwise simple module being expanded too far. This issue is really about changing Drupal's behavior to immediately and automatically delete the old file, but that's only relevant when the functionality of this module is not used. I'd rather this be included in some other contrib module
Comment #19
matoeil commentedAs previously said on #7 though:
- this module does not work as it should at the moment
- when doing replacement , the old file is never deleted , which causes data protection issues
- unless ['make_unused_managed_files_temporary'] = TRUE is set, which is highly not recommended because drupal core has issues with unmanaged files
-these core bugs have been on for 6years so it might not been fixed exactly tomorrow.
may i leave your reconsider as deleting the file immediately would fix the problem?
Comment #20
bkosborneI think there may just be a misunderstanding of this module's purpose. This module enhances the media entity edit form to add an optional feature that allows files to be replaced. This is available via a checkbox. If the checkbox is checked, the original file is replaced with the uploaded version. If the checkbox is not checked, this module does nothing. It preserve's core's original behavior. The module is not attempting to do any more than this. I think you're expecting that the module will delete the original file, even if the checkbox to replace it is not checked? That's not the intent of the module.
Comment #21
matoeil commentedThnaks for taking that time to reply.
I understand your point, but why absolutely wanting to preserve drupal core's behaviour when it is not working properly ?
That module could fill a Drupal flaw.
Unless you do settings that are not recommended in production, that sentence is wrong :
"If unchecked, the filename of the replacement file will be used, and the original file may be deleted if no previous revision references it"
The original file is never deleted !
In other words, for sites in production, the media entity file replace module does not work as it says it should.
Comment #22
bkosborneI think I assume that most people have
make_unused_managed_files_temporaryenabled, but considering core doesn't do that by default, I understand there's probably a large number of sites without that enabled. I personally have it enabled on the sites I manage because the tradeoffs of not having it enabled are worse IMO.Can I ask, why are you not using the checkbox to overwrite the original file? If you did, then you wouldn't have a problem, right? The old file is replaced and there's nothing to delete.
Comment #23
matoeil commentedThe problem is the module leave to content editors of the site, that are not Drupal experts, to choose one option or the other, from one media to another, without understanding fully the consequences of it ( and without knowing how the site has been configured )
If the use of this module was to always overwrite the file , it would be better then to be configurable globally, and not left to the choice of the content editor on every media
Comment #24
flitt1 commentedI've updated the patch from #16 to apply to version 1.2. Nothing has been changed except to make that patch work against the new version.