Overview:
The Media module delete files from the library when the "Media file selector" is used with an image field. This happen with a fresh install of Drupal, using the latest stable version (7.x-1.3) or the dev version (7.x-1.x-dev) of Media module.

I set the priority to critical since it break any content that are using the delete image. It can be extremely frustrating, especially when the image is not use in many location and you realise that it is now gone a few days after it has been deleted, making it very hard to find it back on the backups.

Steps to reproduce the bug:
1. Install Drupal 7
2. Install CTools (7.x-1.3) + Media (7.x-1.3 or 7.x-1.x-dev)
NOTE: I only tried with 1.3 and the dev version. Earlier versions may also have the bug.
3. Modify the article content type to add an image field (lets call it "Preview image"), using the "Media file selector".
4. Create an article and choose a "Preview image" from the library (or upload a new one now).
5. Save the article
6. Edit the article
7. Choose a new "Preview image" and save the article.
8. Go to "Find content", media tab.

Expected to see:
Both images uploaded to the library.

What I see:
Only the second image. The first image has been deleted. If it was used somewhere else, it now show a broken image since the image has been deleted not only from the library, but also from the file system, loosing any version of the image and its metadata.

NOTE: The bug may be part of "File entity", I haven't tried to isolate it.

CommentFileSizeAuthor
#15 media-1968442-prevent-files-deletion.patch7.15 KBgaellafond
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gaellafond’s picture

gaellafond CreditAttribution: gaellafond commented

The bug seems to be related to File Field (Drupal core). When the value of the field change, it automatically delete the previous associated file. This could be easily fixed using a hook_file_predelete, but that hook has been added in Drupal 8 (not available in Drupal 7).

I'm currently trying to develop a Media file selector widget for text field. I'm not sure if that will works. At lease, that type of field do not have side-effect on the file system.

Suggestions are welcome

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

I would suggest you try http://drupal.org/project/file_lock

gaellafond’s picture

gaellafond CreditAttribution: gaellafond commented

Thanks Dave. I will try it right now.

So far, I have these solutions:

  1. Use the Multimedia asset (deprecated), which do not delete entries from the DB nor file system. But it's deprecated...
  2. Use the File lock module, which prevent file field from deleting entries, but it is in Dev... (it turns out that it even prevent manual deletion and its actions can not be undone)

I'm very surprise that the Multimedia asset has been deprecated without having a suitable replacement. Is it because nobody notice (or care about) that the file field delete entries from the library?

gaellafond’s picture

gaellafond CreditAttribution: gaellafond commented

File lock module is not suitable. Once a file is locked using this module, it can NOT be unlock (unless there is an hidden option somewhere). That mean that the files can not be manually deleted. Even disabling the module do not fix the problem. The file are stuck in the system for ever.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Priority: Critical » Major

When using File Entity + Media 2.x, you can easily delete files using file/[file:id]/delete which ignores the file usage.

I'm not sure what more we can do here. The multimedia asset field builds on top of the core file field, which makes assumptions about file usage that are hard to work around. Plus we're not likely going to make any changes to the Media field since it is deprecated.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Related:
#1240834: Cannot delete images added via an image field on an entity, what is previously deleted

paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
CreditAttribution: paulmckibben commented

This is also a problem in the 7.x-2.x branch with the File/Image field with Media widget. The behavior is counter to the idea that you're building a media library. Removing media from content should not remove the corresponding file entity from the library.

I realize this is legacy behavior of the core file field, but this behavior doesn't jibe with the concept of a media library.

Would it make sense to find a way to couple the media browser widget with an entity reference field?

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Title: The "Media file selector" delete file from the library » RFC: How to prevent files from being deleted by file/media fields when unused

Would it make sense to find a way to couple the media browser widget with an entity reference field?

Likely for D8, but at this point not for D7. I think we need to come up with a viable solution right now that doesn't add a dependency.

Options:

  • File lock module
  • Have Media add file_usage for any file added in the media browser, and never delete it. Should file_entity do this for every file added via file/add as well? Maybe some kind of $file->in_library property that adds usage whenever file_save() is called.
  • Work on making #1399846: Make unused file 'cleanup' configurable land in Drupal core and help push a backport to D7.
  • Your solution and thoughts here
Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Version: 7.x-1.3 » 7.x-2.x-dev
Issue tags: +7.x-2.0 release blocker
Devin Carlson’s picture

Devin Carlson CreditAttribution: Devin Carlson commented

Marked #1239056: Remove Media at content edit page cause the file to be completely deleted from server/database? as a duplicate.

gmclelland’s picture

gmclelland CreditAttribution: gmclelland commented

Also considered the use case of files inserted via wysiwyg in custom content panes created with Panels/ctools and https://drupal.org/project/fieldable_panels_panes.

I build a lot of Panel pages that have custom content panes which contain links to files that are uploaded via the wysiwyg with the media module. Without the file_lock module all of those links are going to break when the files are deleted on cron since they aren't given a usage because they aren't tied to a node or field.

I imagine this is a problem for all entities besides nodes? ex. BEANS or Boxes with media inserted via wysiwyg body field.

gaellafond’s picture

gaellafond CreditAttribution: gaellafond commented

I have a proposition, why not continue to support the deprecated field "Multimedia asset (deprecated)", since it is doing the job? There is a few quirks around it, but it's much better than having the library corrupted...

gaellafond’s picture

gaellafond CreditAttribution: gaellafond commented

After investigation, I found out that the problem is not part of the Media module, it's a Drupal design flaw. In other words, it's not a bug, it's a designed feature that do not work with the concept of a file library.

I'm currently writing my own module to fix this issue for good. It will be a module especially made to fix Drupal bad deleting behaviour.

I'm forced to work on it since I can't not continue using the Multimedia asset (deprecated); it only allow users to choose images (PDF and other files from the library are not shown in the media browser).

I'm trying to add a artificial file usage using file_usage_add() to link all files to the media module. Something that tells Drupal: "Don't delete this file, it's used by the Media Library". Similar to what File lock module do, I suppose.

I may combine that with the WYSIWYG patch to record image usage in nodes:
https://drupal.org/node/1268116#comment-6988574
Hopefully that works with the media module... I will have a closer look at that patch.

Then, when a user try to delete a file (not the system, a real human user, I'm not sure yet how I will tell the difference), I will only allow deletion if the file is only used by the Media Library.

"What is in the library stays in the library until the user explicitly required a file delete."

If the Drupal is flexible enough, I might even list the file usage and add a Force delete button to the Delete dialog...

[Edit]

I found an easy way to fix it, but it involve modifying the Media module.
1. Add a fake file usage to all files to prevent Drupal from deleting files
2. Add a check to ignore that fake file usage when checking if the file can be deleted
3. Force delete the image if the file only have the fake file usage

I have done it using the File lock module, since it's doing all the "fake file usage" job. But I don't like that module. It add dependency, required proper configuration, and do not clean-up it's "fake file usage" when uninstalling the module.

I have 2 suggestions:
1. I propose my modifications to the media module, with soft dependency on "File lock module" (the Media module behave exactly as it does now it File lock is not present). This solution has the advantage of having minimal impact on the media module code.
2. I add the needed requirement to do the fake file usage in media module. I can do it as an extra sub-module or directly in media module with a configuration (checkbox) to allow user to choose if the module manage the file delete or let Drupal do it, to have the module behave like it does now (I don't understand why someone would want that but I guess it's better to let the user decide). This solution has no dependency and less configuration, but a bit more code modification and more impact.

Which one would be preferable?

I already implemented the solution 1 and started to work on solution 2.

gaellafond’s picture

gaellafond CreditAttribution: gaellafond commented

I implemented a new Media sub-module to do the file lock. When enabled, it lock all files. When uninstalled, it unlock all files (so it's risk free). Beside that, it behave like to the File lock module, but it's a lot more simple since it doesn't need to lock files according to some regex logic. When enabled, everything is locked. It's configuration free.

I also modified the Media module to ignore the locks from my module and from the File lock module (for the ones that has locked their files using it). The modification to the Media module are minimal, and the Media module do not have any dependency on my new Sub-module.

So far it fixes all my problems and hopefully it will help others.

I will intensively test it tomorrow and post the patch here.

gaellafond’s picture

gaellafond CreditAttribution: gaellafond commented
Version: 7.x-2.x-dev » 7.x-1.x-dev
FileSize
media-1968442-prevent-files-deletion.patch7.15 KB

Patch
* Added a Sub-module (Media file delete manager) that automatically lock all files,
* Modified Media module to delete file when they are only locked by the Sub-module ("File lock" module still lock the files).
I made the patch for 7.x-1.x-dev, but it also works with 7.x-1.3 (the version I'm using).
I didn't tried with 7.x-2.x-dev, but I'm pretty sure the same logic can be apply.
For those using "File lock" module, I will create a patch latter today to unlock the files when you uninstall the module.

After applying the patch, you have to go to the modules interface and enable the module:
Media > Media file delete manager
It may take some time if you have a large library of files.

NOTE: I changed the version because my patch is for version 7.x-1.x-dev. Just tell me if I did something wrong...

[Edit]
"File lock" module patch, to clean-up the locks on files:
https://drupal.org/node/2107925#comment-7951063

cato’s picture

cato CreditAttribution: cato commented
Component: Media field » Code

Still seeing this behaviour in 7.x-2.x-dev...

dandolino’s picture

dandolino CreditAttribution: dandolino commented

Still the same behavior in Media 7.x-2.0-alpha4
Any advice. For a module that is so popular, this is odd.
I'll give "File Lock" a try.
Thanks

Devin Carlson’s picture

Devin Carlson CreditAttribution: Devin Carlson commented

The latest patch in #1399846-263: Make unused file 'cleanup' configurable should be good to go, we just need someone to RTBC it.

steinmb’s picture

steinmb CreditAttribution: steinmb as a volunteer commented
Version: 7.x-1.x-dev » 7.x-2.x-dev
Related issues: +#2801777: Give users the option to prevent drupal from automatically marking unused files as temporary

We might end up backporting #2801777: Give users the option to prevent drupal from automatically marking unused files as temporary

flaviovs’s picture

flaviovs CreditAttribution: flaviovs commented

Your solution and thoughts here

Deleting files associated with nodes is a feature, and a good thing. You don't want dangling files cluttering your precious hard drive after nodes vanish. All that said, why not leverage this and create a Media library content type to hold global-to-be-used-everywhere images, and upload the images there? This brings several other benefits:

  • You add another higher-level form of organization and grouping to your media library
  • You can use Drupal access control so that only authorized users can add/edit/delete Media libraries
  • It's builtin. Already available. No need to change this module, or any other whatsoever

Drawbacks:

  • Media libraries are nodes, so they can be viewed by URL, will be indexed by search etc. (at least these two can be solved by additional modules).
  • If someone erases a Media library node, then all images are gone too, and this might break your site hard. Of course, there are many things that someone with enough powers can do to break your site hard, so you'd better to manage your permissions properly, wouldn't you?
steinmb’s picture

steinmb CreditAttribution: steinmb as a volunteer commented
Category: Bug report » Feature request
Priority: Major » Normal
Status: Active » Postponed
Issue tags: -7.x-2.0 release blocker

Needs to be addressed in core. D8 core first and then backported to D7. Until that we have working solution, this issue should be postponed. I do not consider this issue to be a release blocker. Feel free to disagree though. Might no even belong in Media but in the File_entity —module.

Chris Matthews’s picture

Chris Matthews CreditAttribution: Chris Matthews as a volunteer commented
Status: Postponed » Closed (outdated)

Recent versions of media have resolved most of peoples concerns and is compatible with entity translation, multilingual and various advanced configurations. Due to the high volume of inactive and most often irrelevant issues we are Closing this as (outdated). If for whatever reason this issue is important to you AND you still have issues after checking the media recipe documentation, then let us know and we will review your concerns.

Otherwise, see the recipe documentation for how to configure media and for troubleshooting tips OR refer to the media_dev distribution if you want to see a working media setup.

As mentioned, feel free to make some noise in this issue if you still feel it is important to you or someone else.

Thanks,

Media team