Migrate Media File Delete Contrib to Core

There is also File Delete → with 14,312 reported installs

The File Delete module adds the ability to easily delete files —both private and public— within Drupal administration.

It changes files from the "Permanent" status to the "Temporary" status. These files will be deleted by Drupal during its cron runs.

If a file is registered as being used somewhere, the Module will not allow it to be deleted.

+1 from me, it has good test coverage and very little code

I suppose the 'Media File Delete settings' that live in the contrib module at /admin/config/media/media_file_delete/settings would live in core at /admin/config/media/media-settings

Moving to Drupal core ideas queue for consensus as to whether or not to move forward with the proposed idea to merge the functionality of the Media File Delete contrib module into Core.

Big +1 from me.

I would have assumed that deleting a media entity would also delete the files that were uploaded into that entity, and I think that is probably what a lot of content editors and site builders would have expected.

Think about the workflow from the point of view of a content editor who receives a request to delete an image:

1. We need to remove this image because (for instance) a person in it has withdrawn their consent.
2. We go to the media library and delete the image (aka the media entity, but editors don't think in terms of entities they just think about the thing they are deleting).
3. The editor think that's it done, they go back to the person and say that it's been deleted; But then, either: the editor check (just to be safe) and get confused when they find the image still actually exists, or they don't check and the person that originally requested the deletion sends a follow-up request asking why it was not actually deleted even though the editor said it was.

So, yes I'm 100% to this.

+1 from me, too. The lack of this functionality is really the only reason I'm hesitant about using Media for images. I don't want to accrue lots of orphan files over time.

Broadly +1 to this too, but we also need to resolve 🌱 Dealing with unexpected file deletion due to incorrect file usage Active . Potentially this could be the answer - make file deletion not automatic, but explicit and opt-in, and then assume that people aren't re-using the same file across media items and in other random places and deleting it without checking. Then just drop the entire file_usage system with no replacement.

-

Fwiw The current module doesn't show the option if file usage detects has a record of another usage

It also integrates with entity usage if that is available

+1 from me. This feels like table stakes for any file-based media type. I think deleting the associated file should be the default behavior; if it's not in the CMS, it doesn't make sense (from my own personal user perspective) for it to persist in the filesystem.

The Ideas project is being deprecated. As discussed in a core committer meeting issues that are adding modules are being moved to the Drupal CMS project for discussion.

I'm not sure why this didn't make it into 1.0.

With my Media maintainer hat on:

I don't even think we should add this to Drupal CMS. This should be converted to a core issue and the functionality should be merged into core directly. It's mind-boggling that it's not there already.

With my Site Builder hat on, +1000 to #17 ✨ Migrate Media File Delete Contrib to Core Active (FWIW :) )

Let's do that. I'm gonna jump the gun a bit and assume this should be a priority for 11.3.0. It's kind of nuts that core doesn't already support this, and it shouldn't be a heavy lift at all to merge the module into the main Media module.

Linking a known bug with i18n in the contrib module

All our projects have file deletion on usage 0 enabled and it works generally well, especially when files are mostly used on media entities.

Instead of a manual option, what I'd consider to restore is the old behavior of file usage removal, which didn't mark it as temporary to then remove it hours later but delete it immediately. Far less confusing IMHO and doesn't require a decision by the editor. Could be both as well, only show the manual option if automatic deletion isn't enabled.

Wasn't file usage kind of deprecated as a concept because it has never really worked? That's what I heard a few DrupalCons ago, anyway.

This will need framework manager review for the proposed changes to extant core APIs and subsystem maintainer sign-off (I would qualify except I wrote the patch).

It was disabled, yes. And I think there are some remaining issues with editor and revisions, but we don't use that. The last issue that affected us (when using file exclusively through media entities) was 🌱 Dealing with unexpected file deletion due to incorrect file usage Active . See the issue that @catch linked.

We've had it enabled for years and haven't seen a file deletion related to a bug in a long time. We sometimes have cases where editors link directly to files and that can break, that could possibly be prevented by better tooling.

🌱 Add a reliable entity-usage system to core Active is where we ended up on the file_usage issue - trying to bring entity_usage into core and deprecate file_usage. It is not really moving but I don't see another way to make it work.

I have a site that was experiencing missing files, this site has been around since Drupal 4.5, there is no way to rebuild the file_usage table, so it likely has examples of every file_usage bug since it was introduced and no matter how many we fix it's never corrected retrospectively.

Tests from the module (which are pretty thorough) have been ported over.

The update path had no fixture, so I had to regenerate it. That sucked.

Added a couple comments to MR, nothing major I think. Nice work!

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I understand the need to remove files related to media (it can even be considered a security issue that the related file is not removed). I add a hook for this in basically every project I do. So +1 for adding this to core. I also guess it is good for BC to make this optional.
Do we really need the disable_delete_control option though?

I would suggest that if the configuration specifies files should be deleted when media is deleted, then that should happen automatically, without presenting additional choices or options that might confuse editors. The distinction between files and media isn’t always clear, and adding more options could increase the potential for misunderstanding. We'd have to explain when files can be deleted, when they should be kept, and what happens if someone chooses not to delete a file but changes their mind later. We can avoid this complexity by keeping the configuration straightforward and not allowing individual overrides when deleting a media item.

Another detail if you check both checkboxes on admin/config/media/media-settings ("Delete files from file system when deleting media entities" & "Disable user control of file deletion") and if you then delete a media item you only get " are you sure you want to delete the media item" and that this action cannot be undone. but if you confirm the deletion, in the next step the status message says one item got deleted and one associated file. that way the user is only aware about the implication that the file is also deleted after the media item and the associated file got already deleted. It is correct that with the "Disable user control of file deletion" option active not to provide the "Also delete the associated files" option on the confirmation step, but nevertheless the user should be informed that the associated file is getting deleted on the confirmation step not only after the deletion already took place..

I agree with @seanb in #31. Presenting extra choices will be confusing for people who don't know that media and files aren't the same thing (which, I think we can reasonably assume, is most editors).

This also greatly simplifies the settings forms, tests, and delete forms. The whole patch can shed hundreds of lines.

To me, this is a win. Removing the subsystem maintainer review tag because two of three subsystem maintainers have now examined this patch and agreed on the changes needed.

after pulling the latest changes i run again into the 'disable_delete_control' is not a supported key when activating the When deleting media items, also delete the associated files checkbox.

@rkoller, that option is removed. It's just delete_files now, and if that's set, files are deleted when media are, full stop, end of story. As @seanB mentioned previously in this issue, asking the user to choose every time (or even allowing them to) is probably confusing for editors, and I agree -- let's go with simplicity!

yep i agree with that. i was just confused that that old key came up again after i already successfully tested an iteration of the MR that was using the new key and approach. and now running into that error again was confusing, in particular with no trace of the old key in the code anymore. but i've set up a new fresh testing instance and there is everything working properly again with this MR (see #34) .

This doesn't cover replacing files on medias, those won't be deleted then, even when the media later on is being deleted. And if you use revisions, then you need to act on revisions being deleted as well. I'm not sure right now if revision file usage is correctly updated when you delete a media with multiple revisions.

It obviously also only works for media entities, and not when you use file fields on other entity types, especially files are still somewhat frequently used directly as often no need to have them managed.

I still think this is is essentially duplicating existing functionality that we have that is just disabled out of the box. It's not perfect, but in the current state, this introduces a lot more gaps and problems than the file usage based deletion has IMHO. I'd really prefer if we'd focus on improving that instead of a semi-parallel solution.

As berdir correctly points out, there are a lot of cases that this patch currently doesn't handle. Before I continue, I think it is important to highlight that most editors will not know there is a difference between files and media. I've seen cases where a users accidentally uploads an excel file containing passwords (which also ended up in google). When this user deletes the media item, it is extremely important that the associated file is also deleted. I think this is the main use case we should be trying to solve in this issue.

If I'm not mistaken, we have (at least) the following cases.

Deletes:
1. A user deletes a media item without revisions
This is what the patch in this issue solves. It is a relatively simple use case, since we have a clear indication that the users wants a file removed (as long as it does not have other usages).
2. A user deletes a media item and all its revisions
This becomes a bit more tricky, but in this case I think we can safely delete all files related to revisions of the media item (as long as they do not have other usages).
3. A user deletes a single revision of a media item
This is even more tricky, the file can still be referenced by other revisions of the media item. And example of what this can lead to is 🐛 File usage is not tracked by revision, leading to private files embedded in text fields in old revisions being accessible when they shouldn't be Active . If we want to properly clean up these files, we need to track file usage per revision. We should probably handle this in a followup (if there is not an issue for this already.

Updates:
4. A user updates a media item without revisions
This should be relatively simple. If the original file is not used in other places than the media item, we can simply remove the old file.
5. A user creates a new revision for a media item
This is just as tricky as deleting a single revision. We basically need usage tracking per revision to solve this properly.

If possible, I think we should focus our efforts for this issue on 1. and 2. We should create a followup for issue 4. We should create different followups for case 3. and 5. and block those on tracking file usage per revision.

If we want to properly clean up these files, we need to track file usage per revision. We should probably handle this in a followup (if there is not an issue for this already.

FWIW, my understanding is that we are tracking each usage of a revision. So deleting a single revision should work fine. Looking at \Drupal\file\Plugin\Field\FieldType\FileFieldItemList::delete(), I think we also handle the case when an entity with multiple revision gets deleted, we explicitly delete all usages there. We also deal with translations, which is another fun edge case that I didn't even mention yet. Especially with documents, it's common that different translations reference different files, which this also doesn't handle yet.

Again, there are _many_ cases to consider here.

What we don't track is which revision uses which file, which is what the issue you referenced is about. That is indeed hard to solve because file_usage as it is now only has a single ID as reference.

What I think we should do:

* Change \Drupal\file\FileUsage\FileUsageBase::delete() to delete a file immediately once all usages are gone, instead of the weird delete-after-6h that we have now.
* Expose make_unused_managed_files_temporary in the UI, with some warnings that there *might* be edge cases that might lead to the deletion of files
* Possibly in this issue: Expose this information on the deletion screen. Whether or not the file will be deleted (based on whether or not something else uses it). This is similar to entity_usage features, which display a warning/error on the deletion screen if you attempt to delete an entity that is used by something else. Additional options: a) Allow to override that and allow to force delete a file *even* if there are usages. Maybe a files tab on medias that lists all files used by revisions of this media entity and whether or not they are used elswhere with force delete options.

Change \Drupal\file\FileUsage\FileUsageBase::delete() to delete a file immediately once all usages are gone, instead of the weird delete-after-6h that we have now.

Agree this is a key change from the current behaviour, which is super confusing for editors, who expect the file to be deleted right away.