WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#11149 closed defect (bug) (fixed)

Images are still visible if trashed or if their parent post is unpublished

Reported by: aldenta Owned by: aldenta
Milestone: 2.9 Priority: normal
Severity: blocker Version: 2.9
Component: Media Keywords: 2nd-opinion
Focuses: Cc:

Description

If a post thumbnail is assigned, and then the image is trashed, the post thumbnail still appears since the _thumbnail_id meta key exists.

I see a couple of different ideas that might provide a solution. I'm happy to write the patch but I need some direction on the best option.

  1. Update get_post_image_id() in post-image-template.php to check if the image has been trashed before returning the id. This adds a little bit of extra overhead but would keep the post thumbnail in place if they restore the image. One possible downside is that if the user later restores the image from the Media page then they may be surprised that the post thumbnail has also returned.
  1. Delete the _thumbnail_id when the photo is trashed. This would mean that if the user clicks Undo then the photo thumbnail is no longer attached. It may be nice to delete the _thumbnail_id when a photo is deleted anyway to keep the table a little more tidy.

Any thoughts on the best approach to this?

Thanks.

John

Change History (26)

comment:1 aldenta4 years ago

  • Milestone 2.9 deleted
  • Owner set to aldenta
  • Status changed from new to assigned

comment:2 scribu4 years ago

The problem with option 2 is that it deletes the purpose of the trash functionality by adding a modal window. So I would go with 1.

comment:3 ryan4 years ago

  • Component changed from Media to Trash

comment:4 ryan4 years ago

  • Milestone set to 2.9

comment:5 caesarsgrunt4 years ago

All trashed images still appear in all locations until they are permanently deleted (manually or 30 days after trashing).

It is not possible (sensibly) to automate the removal of images from all posts when they are trashed, and this does not currently happen for deleting images, either. Therefore, I see no reason to automate the removal of trashed images from post thumbnails either.

comment:6 caesarsgrunt4 years ago

I can't find post thumbnails in the UI anymore; are they still there?

comment:7 nacin4 years ago

Your theme needs to support it, and support was pulled from the default theme in [12180]

comment:8 follow-up: caesarsgrunt4 years ago

Oh; pity. Why on earth was it removed?

I'll look for a theme which has it to test this...

comment:9 in reply to: ↑ 8 nacin4 years ago

Replying to caesarsgrunt:

Oh; pity. Why on earth was it removed?

Good question. For testing, I've simply patched the default theme back up with what was reverted, [12133]. Diff: http://core.trac.wordpress.org/changeset/12133?format=diff&new=12133.

You can enable it in the backend, if that's all you're looking for, w/ add_theme_support( 'post-thumbnails' ); in the theme functions.php.

comment:10 westi4 years ago

  • Severity changed from normal to blocker

Maybe we should be using get_post_status() to check that the image is visible as well as check post_status on the individual attachment post related to the image.

comment:11 follow-up: caesarsgrunt4 years ago

What makes this a blocker?

Trashed images are also still visible in all other places. Why is this one so much more important than the others?

comment:12 in reply to: ↑ 11 westi4 years ago

Replying to caesarsgrunt:

What makes this a blocker?

Trashed images are also still visible in all other places. Why is this one so much more important than the others?

It is not just Trashed images that would be visible but ones whos parent is private or draft could end up visible here - I don't think that would normally happen but we need to think about it.

comment:13 caesarsgrunt4 years ago

  • Component changed from Trash to Media
  • Summary changed from Post thumbnail remains after image is trashed to Images are still visible if trashed or if their parent post is unpublished

Still, the same applies to images within the post, and always has done.

Also, I don't think it can be fixed for images in the post, except by renaming the image file.

comment:14 westi4 years ago

Note: If you trash a media item it disappears from the gallery on a post

comment:15 westi4 years ago

For consistent behaviour I think we should be check 'post_status' == 'inherit' on the id stored in post_meta for the post image.

comment:16 caesarsgrunt4 years ago

Interesting. I'm not familiar with galleries, but presumably they are generated on the fly and check the image status, rather than having static code in the post.

This would be a good way to fix this bug and make images more easily editable overall - instead of inserting static html into the post when an image is added, just add a shortcode such as [image id=99 align=right]. The handler for this shortcode would then check the image status, and return an empty string if the status is wrong.

comment:17 westi4 years ago

Looks like that it is only gallerys that exhibit the correct behaviour.

Now wondering if we should disable Trash for Media.

comment:18 follow-up: caesarsgrunt4 years ago

Depends what you mean by "correct".
Firstly, note that the problem here is basically that you can still see the image if you know the URL. But, this also applies to posts.
Secondly, note that the behaviour when you delete an image (pre 2.9) is not "correct" either. The code for the image is still in the post, it's just that the image file doesn't exist any more.

I strongly oppose the idea of disabling Trash for Media.

I propose taking the two actions described below :

1. Check whether post_status == 'inherit' before showing the post thumbnail. Do this before 2.9. This would leave us with Galleries and Thumbnails behaving in an optimal way, and images in posts behaving more or less as they currently do.

2. Change the system for adding images to posts, so that it uses a shortcode rather than inserting direct html. Do this in 3.0. This allows images to be somewhat dynamic, with the ability for plugins to alter their display etc - and enabling us to only show them if post_status == 'inherit'.

comment:19 in reply to: ↑ 18 nacin4 years ago

Replying to caesarsgrunt:

Firstly, note that the problem here is basically that you can still see the image if you know the URL. But, this also applies to posts.

No; trashed posts return a not found, same as if it was a draft, future, etc.

1. Check whether post_status == 'inherit' before showing the post thumbnail. Do this before 2.9. This would leave us with Galleries and Thumbnails behaving in an optimal way, and images in posts behaving more or less as they currently do.

This would work, except that images in posts would not behave as they currently do. When you delete an image, the HTML remains, but the file is gone.

On the other hand, when you trash an image, the HTML and the file both remain, so it will still display. Big problem.

This was extensively discussed in the dev meetup for today. https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2009-12-10&sort=asc. The decision the core devs decided on -- and, FWIW, most participants agreed to -- would be to disable Trash for Media, but keep the code in place and allow a plugin to turn it back on. Thus, Trash would function as if it was turned off, and fall back to the "Delete Permanently" links.

There were many alternatives discussed, but nothing presented itself as a viable solution, especially with 2.9 so close.

2. Change the system for adding images to posts, so that it uses a shortcode rather than inserting direct html. Do this in 3.0. This allows images to be somewhat dynamic

It seems there was traction for an [image] shortcode for 3.0; I imagine that will be the basis of Trash for Media.

comment:20 follow-up: caesarsgrunt4 years ago

OK, well since I didn't participate in the dev chat I'll obviously just have to put up with the ultimate decision of the core devs. I would first like to add my vote for another solution, though, which was touched upon in the dev chat :

Move trashed images into a "trashed" folder, or add a ".trashed" suffix to their names. Then when untrashing them, this change could be reverted. This would "solve" the direct linking problem, since trashed images would then behave in the same way as deleted ones currently do.

That said, and having thought the matter over, I don't think Media is one of the most important places for Trash. I've never deleted an image and then wanted it back... The main purpose of Trash for media, in my eyes, is for consistency with Posts and Pages.

comment:21 in reply to: ↑ 20 nacin4 years ago

westi, do you want someone to patch this?

Replying to caesarsgrunt:

Move trashed images into a "trashed" folder, or add a ".trashed" suffix to their names. Then when untrashing them, this change could be reverted. This would "solve" the direct linking problem, since trashed images would then behave in the same way as deleted ones currently do.

The problem there was pointed out by markjaquith, in that "For file-moving schemes, it would have to be unpredictable." prettyboymp made a good point that we could make it unpredictable then store in in post meta, I don't think it caught on because it's too much to rush into 2.9 and there are probably better ways to tackle this in 3.0.

comment:22 azaozz4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [12377]) Disable trash for attachments for now pending improvements in 3.0, fixes #11149

comment:23 azaozz4 years ago

Can be enabled by adding define('MEDIA_TRASH', true); in wp-settings.php. If there are any attachments in the trash, it will still show on the Media Library screen until they are restored or deleted.

comment:24 azaozz4 years ago

Meant to say "define('MEDIA_TRASH', true); in wp-config.php"...

comment:25 nacin4 years ago

wp-admin.css could use a version bump.

comment:26 westi4 years ago

(In [12386]) Bump the version number on the wp-admin.css files for [12377]. See #11149 props nacin.

Note: See TracTickets for help on using tickets.