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

Command icon 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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

matoeil created an issue. See original summary.

larowlan’s picture

Title: "delete file per default" option » Add a settings form to allow setting a default value for the 'also delete associated file' checkbox
Version: 1.1.0 » 1.x-dev

Updating title.

This is a good idea.

marcoscano made their first commit to this issue’s fork.

chrisgross’s picture

Status: Active » Needs review
FileSize
3.82 KB

Here'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.

chrisgross’s picture

Composer 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.

chrisgross’s picture

Okay, 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.

larowlan’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs update path, +Needs update path tests
+++ b/src/Form/MediaDeleteForm.php
@@ -69,4 +69,8 @@ class MediaDeleteForm extends ContentEntityDeleteForm {
+    $config = \Drupal::config('media_file_delete.settings');
...
+        '#default_value' => $config->get('media_file_delete.delete_file_default'),

@@ -76,2 +80,3 @@ class MediaDeleteForm extends ContentEntityDeleteForm {
+        '#access' => (bool) !$config->get('media_file_delete.disable_delete_control')

Let'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

chrisgross’s picture

@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.

larowlan’s picture

Totally understand mate, our composer.patches.json for projects looks similar :)

No urgency, folks who need this now can use it via a patch

chrisgross’s picture

Ok, 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.

larowlan’s picture

+++ b/media_file_delete.install
@@ -0,0 +1,12 @@
+  $config->save();

According to the docs, we need a 'TRUE' for the argument here

Thanks for those bits, nice one

chrisgross’s picture

I 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.

larowlan’s picture

Yeah, only in the update hook, per the docs on top of hook_update_n in module.api.php

chrisgross’s picture

Okay, 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.

larowlan’s picture

I think defaulting to FALSE is the correct behaviour.

Grevil made their first commit to this issue’s fork.

Grevil’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

Hello 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! :)

Grevil’s picture

Could someone give an example, how to write update path tests here? As of issue tag "Needs update path tests"?

Anybody’s picture

Thank you very much @Grevil, great work! LGTM

I think now it would make sense @larowlan and @chrisgross have a look again finally.

Grevil’s picture

Added settings for testing the default settings on multiple entity deletion.

larowlan’s picture

Status: Needs review » Needs work
Issue tags: -Needs update path, -Needs update path tests

On 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

Grevil’s picture

Status: Needs work » Needs review

Implemented your suggestions! Feel free to have a look at the changes. :)

  • larowlan committed 0b24700 on 1.x authored by Grevil
    Issue #3244890 by Grevil, chrisgross, larowlan, marcoscano: Add a...
larowlan’s picture

Status: Needs review » Fixed

Thanks all, I'll cut a new minor release with this

larowlan’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.