Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Problem/Motivation
It would be useful to have an option to force delete the associated file every time a media is deleted.
Our sites editor does not always know the consequences of keeping an old file physically on the server
Comment | File | Size | Author |
---|
Issue fork media_file_delete-3244890
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
larowlanUpdating title.
This is a good idea.
Comment #4
chrisgross CreditAttribution: chrisgross commentedHere's a patch that adds a setting for the default value, as well as the corresponding settings form and permission. I actually went a bit further and also added an additional setting that removes the ability for users to change this value, which is done by simply hiding the checkbox on the media delete form.
Comment #5
chrisgross CreditAttribution: chrisgross commentedComposer is choking on the missing new line, which has never happened to me before, so here's an updated patch.Nope, this wasn't it.Comment #6
chrisgross CreditAttribution: chrisgross commentedOkay, so I guess info.yml files don't like being patched the old way these days. Here is a variant that follows the recommendations for patching info files according to this article
So I guess if the maintainers want to commit this, use whatever version actually works?
Update: This one works.
Comment #7
larowlanLet's inject the config factory instead of using \Drupal
We also need:
* a post update hook to create the config object
* a config/install version of the config object
* a config/schema for the config object
* tests for the new functionality
* an update path test to ensure the post update hook works as expected
thanks for working on this!
happy to guide through any/all of the above steps if you like
Comment #8
chrisgross CreditAttribution: chrisgross commented@larowlan haha well I was going for quick and simple since the default config is empty and I have a bunch of looming deployments and migrations that are heavily dependent on core's still very broken and unfinished media implementation (what this module does NEEDS to have been in core already and I can't believe it's not, but our composer.json is already full of countless one-off contrib modules like this one filling in the many gaps). If I can find the time I will try to look into doing it the way you suggested.
Comment #9
larowlanTotally understand mate, our composer.patches.json for projects looks similar :)
No urgency, folks who need this now can use it via a patch
Comment #10
chrisgross CreditAttribution: chrisgross commentedOk, I whipped together a version that I think handles the config better. No tests at this point, but let me know if this seems right.
Comment #11
larowlanAccording to the docs, we need a 'TRUE' for the argument here
Thanks for those bits, nice one
Comment #12
chrisgross CreditAttribution: chrisgross commentedI haven't done much configuration in code in D9, as I've most done this with YAML, so it took me a while to figure out why some examples I found were using TRUE and some were not. I believe TRUE should be used when the data is "trusted" and doesn't need to be validated. I'm not 100% sure I know what that means, other than the fact that is hard-coded, like in .install files vs data entered via a config form (even though the latter only uses preprogrammed true and false values). In that case, I would use TRUE in the update hook but not in the settings form. Is that distinction accurate? I will update the patch when I get a chance to further review. I also don't really have much experience at all writing tests but I will see what I can figure out.
Comment #13
larowlanYeah, only in the update hook, per the docs on top of hook_update_n in module.api.php
Comment #14
chrisgross CreditAttribution: chrisgross commentedOkay, here's an updated patch that trusts the data and generally cleans up the update hook and I believe actually gets the version number right.
One point of discussion may be what the defaults for these new settings should be. Currently, I have defaulted them to FALSE so as not to change the current behavior of this module. But I also know that, as a user, I feel like I might generally expect both of these things to be TRUE by default, but I'm not sure. The values and code that uses them could also potentially be inverted. Perhaps that would be up to you, as the maintainer, to decide?
I think the tests might be a bit over my head and this point but I will have to do some more research.
Comment #15
larowlanI think defaulting to FALSE is the correct behaviour.
Comment #18
Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commentedHello guys! :)
I applied patch #14 by @chrisgross, to this issue's issue branch, rebased the branch, fixed drupal coding standards, renamed the media_file_delete settings page label, added a menu link to the settings page, added the "delete_file_default" value to the MediaDeleteMultipleForm as well as the "disable_delete_control" setting and added tests.
Please ignore commit 87b889b2 - Removed installation update hook, since the settings did not exist before (not needed) I reverted that change later, sorry for the confusion.
Let's wait for the tests to pass and then somebody can review the MR! :)
Comment #19
Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commentedCould someone give an example, how to write update path tests here? As of issue tag "Needs update path tests"?
Comment #20
AnybodyThank you very much @Grevil, great work! LGTM
I think now it would make sense @larowlan and @chrisgross have a look again finally.
Comment #21
Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commentedAdded settings for testing the default settings on multiple entity deletion.
Comment #22
larowlanhiding patches now there's an MR
Comment #23
larowlanOn the basis that the update hook here is super simple, opting to remove the update path tests tag.
Left a couple of minor amendments on the review, but this is very close.
Sorry for the delay in reviewing, been a busy few months
Comment #24
Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commentedImplemented your suggestions! Feel free to have a look at the changes. :)
Comment #26
larowlanThanks all, I'll cut a new minor release with this
Comment #27
larowlanhttps://www.drupal.org/project/media_file_delete/releases/1.3.0 🎉