Skip to content

Image Library displaying files that cannot be used. #3857

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Graham-72 opened this issue Jun 7, 2019 · 36 comments
Open

Image Library displaying files that cannot be used. #3857

Graham-72 opened this issue Jun 7, 2019 · 36 comments

Comments

@Graham-72
Copy link

On one of my older Backdrop sites I find that if I try to re-use one of the image files displayed in the Image Library I am prevented from doing so and get the following message
Screenshot_2019-06-06 Edit Page Test page 1 more about books

This would seem to be because Backdrop does not have a record of a reference to this file in its file usage list, perhaps because the node using it was created in an earlier version of Backdrop?

I would prefer to be able to re-use any image file that already exists on the server. It seems wrong to be shown images that cannot be used.

I have looked at the code for function file_managed_file_validate in file.module which produces this message and have tried to understand its purpose but am struggling with the wording of the comment If referencing an existing file, only allow if there are existing references. This prevents unmanaged files from being deleted if this item were to be deleted. What does the second sentence mean?

@klonos
Copy link
Member

klonos commented Jun 7, 2019

Yep, I am struggling with that inline comment too. It needs to be simplified and things explained better/more.

@laryn
Copy link
Contributor

laryn commented Mar 19, 2021

I just ran into this too -- it's confusing. I suppose this image was uploaded through the WYSIWYG editor (which doesn't manage files but does allow you to upload them). Now it's being referenced in a file field, which does manage files -- so if it allowed us to link the image, add a usage, etc. we might later delete it from the image field, which would then delete it from the system because the only recorded usage of the file had been removed. This would break the unmanaged usage in the WYSIWYG editor. Does that sound plausible?

Regardless, I don't like this -- if it shows up in the image library we should allow it to be used I would think.

@klonos
Copy link
Member

klonos commented Mar 19, 2021

...this image was uploaded through the WYSIWYG editor (which doesn't manage files but does allow you to upload them).

I don't think that that statement is true. Try the following:

  1. On a vanilla installation of latest Backdrop, head to the files listing (admin/content/files)
  2. Verify that there are no files listed 👍🏼
  3. Keep this browser tab open, and open another one.
  4. Start creating a post node
  5. Click the "add image" button in the WYSIWYG and upload an image, but DO NOT click on the "Insert" button on the dialog just yet.
  6. Switch to the tab with the files listing (admin/content/files) -> notice that the image is added to the managed files, although without any usage count, and its status set to "Temporary" 👍🏼
  7. Back to the tab with the node edit form, click the "Insert" button on the dialog -> the image gets inserted to the WYSIWYG
  8. Back to the files listing -> refresh -> file still "Temporary" and no usage count 👍🏼
  9. Back to the node edit form, in the image field below the WYSIWYG, click "select existing file" -> the temporary image is shown 👍🏼
  10. Select the same image you added to the WYSIWYG, and add it to the image field.
  11. Back to the files listing -> refresh -> file still "Temporary" and no usage count 👍🏼
  12. Back to the node edit form, add a title to the post, and save it.
  13. Back to the files listing -> refresh -> file usage count is set to 2, and its status to "Permanent" 👍🏼
  14. Edit the post you created -> remove the image from the image field -> save.
  15. Back to the files listing -> refresh -> file usage count is reduced to 1, and its status remains "Permanent" 👍🏼
  16. Edit the post you created once more -> remove the image from the WYSIWYG -> save.
  17. Back to the files listing -> refresh -> file usage count is reduced to 0, and its status set to "Temporary" 👍🏼

...we might later delete it from the image field, which would then delete it from the system because the only recorded usage of the file had been removed.

Nope. See steps 14 to 17 above.

So it seems to me that images are managed whether they are being added to an image field or the WYSIWYG editor.

@laryn
Copy link
Contributor

laryn commented Mar 19, 2021

I stand corrected. I still don't know what this error means but I still don't like it and think if an image can't be used it shouldn't show up here.

@klonos
Copy link
Member

klonos commented Mar 19, 2021

...I still don't like it and think if an image can't be used it shouldn't show up here.

Yup, I think we can all agree re that 👍🏼

It would help if we had steps to reproduce this, because I couldn't in a vanilla installation of the latest 1.18.1 🤔 ...the most plausible scenario I can imagine is what @Graham-72 said:

This would seem to be because Backdrop does not have a record of a reference to this file in its file usage list, perhaps because the node using it was created in an earlier version of Backdrop?

Seems that we may need an update hook, to fix these older files.

@laryn
Copy link
Contributor

laryn commented Mar 19, 2021

Ah ha -- found a way. On one of the dev sites I'm working on, we're using the Video Embed Field module, which creates a thumbnail image from a Youtube or Vimeo video -- these thumbnails don't have any usages added in the Manage Files area. They show up in the Image Library, however, and give the error above.

Perhaps I need to file an issue with VEF to add usage to the thumbnails for this particular case...

@indigoxela
Copy link
Member

indigoxela commented Mar 20, 2021

There's an older issue where we handled this for core.
Yes, filing an issue with VEF makes sense (has already happened).

While the error is correct (this file really shouldn't get referenced), it seems more reasonable to not show these files in the first place. That's an easy little change in the view, which prevents WTF moments. Just filter for file usage >= 1.

@klonos
Copy link
Member

klonos commented Mar 21, 2021

Just filter for file usage >= 1.

@indigoxela I believe that the = bit will exclude all files added via the "+Add file" link in admin/content/files. So that's no good I'm afraid.

@klonos
Copy link
Member

klonos commented Mar 21, 2021

I unsuccessfully tried to reproduce this by doing the following on a fresh installation of Backdrop:

  1. Open 3 browser tabs
  2. On the 1st browser tab, go to admin/content/files. We'll use this tab to monitor the status and usage count of files added to the site at each given point.
  3. On the 2nd browser tab, start adding a piece of content of any type (I prefer Posts, because I can add images to the WYSIWYG or the "Image" field).
  4. Add an image to the "Image" field, but DO NOT SAVE the node yet.
  5. Switch to the 1st browser tab, and notice that the file has been added with no usage count, and its status is "Temporary".
  6. Switch to the 3rd browser tab, and start adding a piece of content there.
  7. Try adding the temporary image you uploaded earlier -> no errors 👍🏼
  8. Save the node in either of the 2nd or 3rd tab -> no errors, and the file gets a usage count and becomes permanent 👍🏼

I then tried reuse an image that's added in a taxonomy term description, or a custom block (to see if #5016 has anything to do with our issue here). These files are not available to be reused though 🤷🏼

At this point, I go back to "we need a way to reproduce this" ...we simply can't fix what we can't reproduce 🤷🏼

@indigoxela
Copy link
Member

I believe that the = bit will exclude all files added via the "+Add file" link in admin/content/files. So that's no good I'm afraid.

@klonos I don't think so, my local tests have been successful. It's not "=" btw, its "greater or equal one". You could also set it to "greater than zero".

@klonos
Copy link
Member

klonos commented Mar 21, 2021

@indigoxela when you said "Just filter for file usage >= 1." I thought you meant to exclude them 🤦🏼 ...and yeah, I read "greater than" as "less than". My bad.

@keiserjb
Copy link

I seem to have this all over the place on Backdrop sites that I have upgraded from Drupal in the past year. Let's take these six images. (Screenshot from Drupal 7)
image

File usage says 0 places, even though they are used in a published node. I can also add these images to new content in Drupal.

Now we go to the Backdrop site. There is still a published node using the images. Nothing in the use count.
image

I cannot add these images to new content in Backdrop. I get the error at the top of the issue.

@stpaultim
Copy link
Member

I'm getting close to launching a Drupal 7 to Backdropcms upgrade and have been running into this problem as well. I just haven't yet had time to trouble shoot it and find a work-a-round that will not frustrate the site editors. Hoping that the suggestion from @indigoxela will at least be a temporary fix.

@catorghans
Copy link

There's an older issue where we handled this for core. Yes, filing an issue with VEF makes sense (has already happened).

While the error is correct (this file really shouldn't get referenced), it seems more reasonable to not show these files in the first place. That's an easy little change in the view, which prevents WTF moments. Just filter for file usage >= 1.

Which view would need to be adjusted?

@olafgrabienski
Copy link

@catorghans It's the view called "Image Library" (path: admin/structure/views/view/image_library).

@catorghans
Copy link

@catorghans It's the view called "Image Library" (path: admin/structure/views/view/image_library).

It works now., thank you!
I would be in favor to have File usage "greater then zero" by default.
Mine was also caused by a D7 to backdrop upgrade.

@laryn
Copy link
Contributor

laryn commented Jan 11, 2024

If referencing an existing file, only allow if there are existing references. This prevents unmanaged files from being deleted if this item were to be deleted.

If I'm understanding this correctly, these images should be available when used in the WYSIWYG setting (since no usage is added or removed, so no danger of inadvertently deleting these images when removing the image from the editor window), but they should not be allowed in a managed file field setting (to prevent accidentally deleting them by adding a usage and then potentially removing it later and triggering garbage collection). Does that sound right?

@olafgrabienski
Copy link

If I'm understanding this correctly, these images should be available when used in the WYSIWYG setting (since no usage is added or removed, so no danger of inadvertently deleting these images when removing the image from the editor window) ...

Hm, as far as I know, file usage is also relevant in a WYSIWYG powered text field. When I add an image there, it becomes a (permanent) managed file. When I remove it again, its state changes to "Temporary".

@laryn
Copy link
Contributor

laryn commented Jan 11, 2024

Oh, you're right! For some reason I thought WYSIWYG ignored usage and all files added that way were permanent. So it seems like we really to just need to adjust the image library not to show images with no usage to avoid this confusion.

@GeoTimber
Copy link

I think the code below is the culprit and incorrect, if you want to use a file with usage count 0 ( no references ) then you should be allowed AND it should create a reference in the file_usage table with the file_usage_add( function when you use it again

This happened on a D7 migration too for us, not sure why the D7 site did not have this file in its file_usage table

core/modules/file/file.module

/**
 * Render API callback: Validates the managed_file element.
 *
 * This function is assigned as a #element_validate callback in
 * file_element_info().
 */
function file_managed_file_validate(&$element, &$form_state) {
  // If referencing an existing file, only allow if there are existing
  // references. This prevents unmanaged files from being deleted if this
  // item were to be deleted.
  $clicked_button = end($form_state['triggering_element']['#parents']);
  if ($clicked_button != 'remove_button' && !empty($element['fid']['#value'])) {
    if ($file = file_load($element['fid']['#value'])) {
      if ($file->status == FILE_STATUS_PERMANENT) {
        $references = file_usage_list($file);
        if (empty($references)) {
          form_error($element, t('The file used in the !name field may not be referenced.', array('!name' => $element['#title'])));
        }
      }
    }

@argiepiano
Copy link

argiepiano commented May 14, 2025

Hi @GeoTimber. Thanks for posting.

There is currently discussion about this topic in the Backdrop Zulip channel - you may want to join the conversation there.

These are some of the key points in the conversation:

  1. Permanent files with usage = 0 is an anomaly that have been found to be pretty common from D7 upgrades. Why this happens is still unclear (to me)
  2. Having permanent managed files with usage = 0 is an anomaly and should not happen in normal operations of Backdrop sites. Even when you create a file entity by uploading a file through the Manage files UI (admin/content/files), the file usage for the new file becomes = 1 , even though that file is technically not "used" anywhere in the site other than as a file entity. In fact there is code in Backdrop that will eliminate files when their use becomes 0
  3. It has been suggested that a quick fix for this is to run a custom script after upgrade to set all permanent managed files with usage = 0, to usage = 1 to prevent Backdrop from wrongly deleting files that are in use (e.g. if you have a file with usage = 0, then add that file to a node, then delete the node, the usage will go back to 0, causing Backdrop to completely remove that file). This is what Backdrop doesn't let you reference that file (which has a usage 0), as that file may be deleted.
  4. Others have argued that we should not remove files that have usage = 0. But this creates the problem that file entities will never be deleted when they are not needed anymore anywhere

I would be helpful to continue to dig. I proposed that this would be a good topic to discuss during the dev meetings on Thursdays.

@bennybobw
Copy link

bennybobw commented May 14, 2025

Just as a reference, I looked at three large D7 sites that we are in the process of migrating from D7 to Backdrop.

Site 1
Total managed files: 4358
Files no file_usage rows: 70

Site 2
Total managed files: 11626
Files no file_usage rows: 3835

Site 3
Total managed files: 61392
Files no file_usage rows: 7137

The thing that gets very hard to tell in these upgrades is whether the files with file_usage 0 are actually unused or not.

@bennybobw
Copy link

bennybobw commented May 14, 2025

Others have argued that we should not remove files that have usage = 0. But this creates the problem that file entities will never be deleted when they are not needed anymore anywhere

The reason I'm arguing for that is that I'd rather be able to go into backdrop and clean up unused files myself rather than have backdrop do it automatically without me knowing about it. It's easy enough to do with a views bulk operation.

At the end of our migrations we're going to have 30+ Drupal sites that we host/manage but our clients run. Deleting files that hit usage 0 is not something Drupal 7 did and should be highlighted somewhere as a breaking change in my opinion. It could lead to irrevocable data loss depending on when the file deletion is discovered.

Disk space is cheap. I'd rather err on the side of caution and have some extra files lying around than irrevocably lose some of our clients' files because of file_usage_delete change.

@GeoTimber
Copy link

GeoTimber commented May 14, 2025

Hi @argiepiano,
thanks for you reply,

Having permanent managed files with usage = 0 is an anomaly and should not happen in normal operations of Backdrop sites. Even when you create a file entity by uploading a file through the Manage files UI (admin/content/files), the file usage for the new file becomes = 1 , even though that file is technically not "used" anywhere in the site other than as a file entity. In fact there is code in Backdrop that will eliminate files when their use becomes 0

So you say during the migration I should create a file usage record for the file entities I want to keep and that did not have a file usage record already?

As suggested here ? #Backdrop > File may not be referenced in WYSIWYG @ 💬

Still i am not in favour of auto cleanup, or not being able to revive a file if it is no longer used.

  • I think the cleanup should be a configurable cron job running on x day old files, meanwhile you can re-use the non-used file, or turn the cleanup feature off.

  • And if Drupal 7 did not delete unused files the cron job to cleanup/delete files should be turned off by default doing a migration to maintain compatibility.

  • And the migration should not have left the files in an invalid state, if you cant have a permanent managed file without a file usage record then it needs to be solved before the import is completed.

The problem is probably caused by modules not registering their file usage properly or a bug in the registration in D7.

@bennybobw
Copy link

Ok we talked about this in the dev meeting. I think the files that get marked automatically for deletion in file_usage_delete is a separate issue really that should be split out into a new one because it's confusing the issue.

The question here really is whether you should be able to insert files that have no file_usage via the WYSIWYG and what should happen if you do. One idea I had was that we add a file_usage row in that case like we do as if you had added the file via file/add but quicksketch pointed out that the file might have been uploaded already to a field on the node while you're editing and might be marked as temporary until you save the node. I'm not totally sure it shows up in the image library in that case, but I wouldn't be sure what to do in that case.

The problem is probably caused by modules not registering their file usage properly or a bug in the registration in D7.

We did discuss this and everyone agreed that we should add a file_usage row for any files in file_managed that are getting migrated from Drupal 7 in the same upgrade hook that handles converting those files to file entities in Backdrop. I think we'll need a separate issue for that as well.

@indigoxela
Copy link
Member

indigoxela commented May 16, 2025

Just my 2c here: The library should not just allow to reference files that are in this "precarious" state of being permanent, but having zero usage, as this introduces a mean bug, which will definitely lead to vanishing files.

Not showing those files in the library in the first place is an easy task, as it's a view. But that still just hides the problem without solving it. And we want to solve it - files in "permanent" state with zero usage are an anomaly and we should fix that.

Some options I see:

  • An update hook specifically for D7 upgrades, which are known to cause that problem (with inline images/files)
  • An admin setting/config in core to disable file/image garbage collection entirely - either with UI, or only hidden settings (currently too much is hardcoded re that) - and that could be picked up by contrib
  • A contrib utility module that fixes the uncommon state by setting usage to file entity for those files - in a batch job
  • Fix it (intelligently) in core

That "intelligent" solution was something that quickly came to my mind (didn't look into details at all):

  • Do not further block referencing of those zero-usage files, but
  • When adding file usage and the current usage is zero, add 2 usages - one for the actual spot where it's used and one self-referencing, that will prevent automatic deletion when usage gets decreased again (if the node using it got deleted, for example)

@bennybobw what do you think about my suggestion of handling it when adding usage, but to not touch all the other files in that odd state? Good or bad idea?

@bennybobw
Copy link

bennybobw commented May 16, 2025

@bennybobw what do you thing about my suggestion of handling it when adding usage, but to not touch all the other files in that odd state? Good or bad idea?

Yeah I had this exact same idea in the dev meeting. @quicksketch brought up that it might be tricky to tell if a file has just been uploaded and is being inserted via the wysiwyg button or whether it's one of these files that has no file_usage. I think fixing the other issue is better where we just add file_usage to files getting migrated. Then I think we don't have to worry about this?

The library should not just allow to reference files that are in this "precarious" state of being permanent, but having zero usage, as this introduces a mean bug, which will definitely lead to vanishing files.

Ok this brings up something that I was trying to introduce in the dev meeting which is that in my mind the context in which a file gets uploaded leads to different expectations about how that file is handled when the node or entity is deleted.

  • Scenario 1 The file is attached directly to the entity. By that I mean you upload it not via a "library" widget. Examples of this are using the Backdrop default file or image field. If the node is deleted I expect that file to be removed when I delete the node.
  • Scenario 2 The file is used in the node, but exists separately in a "library". When you click the image widget via the WYSIWYG the tab on the right is called "Image Library". I expect that these images (or if we were to build out a generic "File Library browser" in Backdrop) to stick around even if I delete the node. To keep going with the "Library" metaphor, deleting the images uploaded via that widget in my mind would be the same thing as destroying the book when it's returned to the library. Just because the reference is removed in this node, doesn't mean that I want it removed from my library. What if I want to use that file or image in another node? I then have to re-upload it?

I think this is really unexpected behavior. Neither Drupal 11 or Wordpress delete the image that's inserted via their media libraries when a node or a post is deleted. I think it's confusing and unexpected behavior. @quicksketch brought up that it makes it hard to delete files. I disagree. I'd expect to go to my file library and delete the unwanted file there, and then I expect it to be deleted off the disk. The word "undeletable" is a bit of a misnomer in my mind. It's not automatically deleted but

I don't think files inserted via the wysiwyg button or another "Library" browser should be deleted when whatever entity is deleted. Because those files exist in the context of the Library, not the entity that they're being referenced from.

Another thing we discussed in the meeting was this:

  • Giving a warning when you're deleting an entity that files will be marked for deletion
  • Or (even better in my mind) -- allowing you to pick and choose which files get saved or deleted

But I think just behavior that matches the context of how those files were uploaded would be enough because the pick and choose thing sounds like a lot of work.

@indigoxela
Copy link
Member

@bennybobw I think, your idea of what the "file library" in Backdrop is, differs a lot from my idea. 😉

@bennybobw
Copy link

@bennybobw I think, your idea of what the "file library" in Backdrop is, differs a lot from my idea. 😉

I think what you're saying is that you would expect an image uploaded via that wysiwyg image library button to be deleted when you deleted the node. How does that square with the concept of uploaded media via media browsers in either Wordpress or Drupal? People coming from either of those platforms are going to be really confused I think.

@bennybobw
Copy link

In other words, the node "owns" the image or the file, whereas in my mind it exists as a separate independent entity owned by the "File Library" unless the file is attached directly to a file field on the node.

@bennybobw
Copy link

bennybobw commented May 16, 2025

To put it another way, when I click that button, I believe I've now opened a different view to admin/content/files -- I'm in the "File Library" and uploading files to that, not in the node context anymore. When I insert that to html, I'm merely referencing that file that exists in the file library.

@laryn
Copy link
Contributor

laryn commented May 16, 2025

I'm not sure I would have articulated (or understood) things in that way before @bennybobw typed this up, but I do admit that top-level view of context and context-shifting as to what's specific to the node (fields attached directly to the node) and what's opening up a separate context (image library viewing all available images from admin/content/files and potentially adding a new image to that collection) is compelling to me.

@bennybobw
Copy link

Just to give another example, when I create a taxonomy term on a node, that taxonomy term exists separately as an entity and doesn't get deleted when I delete the node, even if no other nodes reference that term.

@bennybobw
Copy link

I was noodling around some more on this and thought of another way to conceptualize this outside of file upload context which I think is still a good way of thinking about it.

The question is this: in Backdrop are files fully independent entities or are they subordinate entities? Both of these types of entities exist in Backdrop (although maybe not in core?)

Taxonomy Terms

These are clearly fully independent entities. If you delete a node that has a taxonomy term tagged to it, it doesn't delete the tag, even if the tag has no other references to it. In order to delete a taxonomy term you have to go to admin/structure/taxonomy/% and delete the tag there.

Paragraph Entities

These are clearly subordinate entities. They don't exist outside of the context of a node and as far as I know are never referenced by other nodes. When you delete a node, it also deletes the paragraph entity. You don't expect it to exist in another interface (and in fact there is no other way to manage paragraph entities).

What we're discussing here sounds like the following: a file can be either a fully-fledged entity or a subordinate entity depending upon the context in which that file is uploaded.

Part of what's confusing about this (I think) is that files in Drupal 7 weren't fully fledged entities at all and you had to install the media module which converted them into fully-fledged fieldable entities.

Additionally files appear to be more like Taxonomy Terms than Paragraphs entities because of two things:

  • They have their own UI for managing them at admin/content/files
  • They can be referenced by more than one entity external to themselves

There might be a more "computer science" way of discussing this that I don't know, but I'd be curious to hear people's reactions to this.

@indigoxela
Copy link
Member

While in-depth discussion is ongoing here, people might need a solution for the immediate problem, so here's a new contrib module: File Usage Fixer.

@GeoTimber
Copy link

I believe that files should have a corresponding entity that describes them ( file entity ) and that their should only be one type of file entity in Backdrop. For many reasons...

I also believe paragraphs was a great idea, but that paragraphs should have been full entities. I have been working with systems where after a few years we suddenly had a use case where we needed to be able to re-use the same paragraphs on other entities, this was not really possible and even let to problems with some developers ignoring this fact. If paragraphs should have been linked to one node only, then this information should have been a property of the paragraph full entity. Now I think paragraphs was a bad idea, use entities.

I also believe that a if you want files to be linked to one node only, be non-reusable ,and be deleted when that node is deleted then this information should be expressed as properties of the file or better file referencing entities, like the media entity, and if you want a WYSIWG editor module, set by field/other config on creation, as in the later drupal releases.

By following such a pattern you can achieve all possible use cases through normal entity relationship patterns and constraints.

And I think they went through the same discussion in developing the releases in Drupal 8

I think drupal 10 now asks you if you want to delete the file when you are the last user, and that if you want files to be fieldable you generally use the media module ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests