Unable to remove missing image from body content [#1937864]

Repeatable: Always
Steps to repeat:
1. Upload an image via the media browser and insert into the body of a content item.
2. Via Content -> Files - use the media browser to delete the image from the site.
3. Re-vist the content item that was using the image - you'll see the image missing icon (grey with a broken white box - image-x-generic.png).
4. Remove the missing image from the content body, and click save.

Expected Results:
An updated content item (node) without the image (or missing image icon).

Actual Results:
Recoverable fatal error: Argument 1 passed to file_usage_delete() must be an instance of stdClass, boolean given, called in /Users/tony/Projects/Drupal/public/sites/all/modules/media/includes/media.filter.inc on line 72 and defined in file_usage_delete() (line 689 of /Users/tony/Projects/Drupal/public/includes/file.inc).

Note: If the missing image (image-x-generic.png) is left in the body of the content item, the text of the item, and other elements in the body can be saved and updated okay. The error above only occurs when you try to remove the missing image placeholder.

Comment	File	Size	Author
#11	interdiff.txt	1.85 KB	slashrsm
#11	media_fatal_wysiwyg_remove_1937864_11.patch	2.53 KB	slashrsm
#11
#2	media_fatal_wysiwyg_remove_1937864_2.patch	695 bytes	slashrsm
#2
	image-missing.png	18.87 KB	anthony.bouch

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

ParisLiakos CreditAttribution: ParisLiakos commented 18 March 2013 at 19:57

Priority:	Normal	» Major
Issue tags:		+7.x-2.0 alpha blocker

Comment #2

slashrsm CreditAttribution: slashrsm commented 20 March 2013 at 15:31

Status:

Active

» Needs review

File	Size
media_fatal_wysiwyg_remove_1937864_2.patch	695 bytes

Attached patch kinda fixes it....

I am thinking... Is it OK to allow delete of a file that is in use on an entity? Shouldn't we prevent people from doing this?

Comment #3

azinck CreditAttribution: azinck commented 20 March 2013 at 15:37

This pattern is very common in Drupal. You can delete a taxonomy term even if other entities reference it, etc. The file deletion form does warn you if the file's in use. It could be nicer and give you a list of all the places it's in use but to me that's a different question. Your patch looks sensible to me though I haven't yet tested it.

Comment #4

slashrsm CreditAttribution: slashrsm commented 20 March 2013 at 16:29

You are right. Maybe we should at least make this warning more visible, as it can be easily overlooked now.

Entities that use a given file can be seen at Usage tab (file/FID/usage).

Comment #5

azinck CreditAttribution: azinck commented 20 March 2013 at 17:05

I agree with making the warning more noticeable. And thanks for the tip about the usage tab; I guess I've always overlooked that one.

Comment #6

slashrsm CreditAttribution: slashrsm commented 20 March 2013 at 17:06

Actually... We could make it more visible AND link to usage list. What do you think about that?

Comment #7

azinck CreditAttribution: azinck commented 20 March 2013 at 17:20

Very smart.

I don't have a good sandbox environment in front of me: is the usage list subject to node_access limitations (in other words, do nodes that you don't have permission to see get listed here)? If not, then I'd advise adding something to the usage page that says something like "plus 42 pieces of content you don't have access to".