WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 3 years ago

Last modified 12 months ago

#12922 closed defect (bug) (fixed)

Setting featured image does not require the post to be saved

Reported by: rooodini Owned by: flixos90
Milestone: 4.6 Priority: normal
Severity: normal Version: 3.0
Component: Post Thumbnails Keywords: has-patch commit
Focuses: Cc:
PR Number:

Description

This may not be a bug... But it is not intuitive behaviour (I'd expect it to work more like Post Tags).

It's probably quite clear, but here's a test case anyway:

  1. Create a new draft post
  2. Enter a title
  3. Click "Save draft"
  4. Click "Set featured image", pick an image, click "Use as featured image"
  5. Close the dialog
  6. Navigate away and come back (without clicking "Save draft" or "Publish"). The featured image has been added to the post.

Attachments (7)

12922.diff (4.4 KB) - added by flixos90 3 years ago.
patch to only save featured image on "Update"
12922.2.diff (4.4 KB) - added by flixos90 3 years ago.
12922.3.diff (4.1 KB) - added by flixos90 3 years ago.
12922.4.diff (4.2 KB) - added by flixos90 3 years ago.
12922.5.diff (8.3 KB) - added by flixos90 3 years ago.
12922.6.diff (6.9 KB) - added by flixos90 3 years ago.
12922.7.diff (6.4 KB) - added by flixos90 3 years ago.

Download all attachments as: .zip

Change History (46)

#1 @dd32
10 years ago

  • Component changed from General to Administration
  • Keywords 2nd-opinion dev-feedback added; needs-patch removed
  • Milestone changed from Unassigned to 3.0

As with all operations in the media dialogue, the operation happens when you click on the action links.

Aside from inserting data into the post content (images/gallery) all operations are saved at that point in time.

Thats the expected behaviour to me.

#2 @nacin
10 years ago

I'd agree. It's expected behavior, or at least has been for a while now, and it certainly won't change for 3.0. I say that because media/upload will probably be a big 3.1 project, and most of the UI will change.

#3 @rooodini
10 years ago

  • Milestone changed from 3.0 to Future Release

Yeah I had in mind that this would just be for a future release.

I take your point about the media dialogue.

I prefer the way post tags work.. I click "Add tag", the tag appears to have been changed, but nothing is saved until I click Save or Publish.

The featured image seems more related to the post and less to general media. That's why I imagine it working more like post tags and less like the rest of the media dialogue.

#4 @hakre
9 years ago

Both sides are somehow right. Wontfix?

#5 @scribu
9 years ago

#17100 was closed as duplicate.

It highlights the fact that all custom fields behave this way.

#6 @ocean90
9 years ago

  • Keywords ux-feedback added; 2nd-opinion dev-feedback removed

#7 @nacin
7 years ago

  • Milestone Future Release deleted
  • Resolution set to duplicate
  • Status changed from new to closed

All custom fields do behave this way, but we can make it so featured images don't. The patch on #21776 handles this, so closing this as a duplicate.

This ticket was mentioned in Slack in #core by mboynes. View the logs.


4 years ago

#9 @mboynes
4 years ago

  • Resolution duplicate deleted
  • Status changed from closed to reopened

This appears to still be an issue. I looked around for more recent information on this issue, but failed to find any. Not sure if there was a regression since #21776 or if that didn't resolve it as advertised.

#10 @SergeyBiryukov
4 years ago

  • Component changed from Administration to Post Thumbnails
  • Milestone set to Awaiting Review

#11 @SergeyBiryukov
4 years ago

#33505 was marked as a duplicate.

#12 @flixos90
3 years ago

While I see the point that setting the thumbnail without a page reload is not in line with the rest of the Edit Post data, I don't think we should change that, especially with a possible revamp of the post editor in mind (AJAX saving, see #7756) where the current implementation should already be good for.

Also, any user that relies on the featured image being managed directly via AJAX, would be confused if this suddenly stopped working.

Last edited 3 years ago by flixos90 (previous) (diff)

#13 @flixos90
3 years ago

  • Keywords needs-patch added; ux-feedback removed

We had some further discussions (at the Contributor Day in Nuremberg) and came to the conclusion that we can probably fix this without affecting too many users (contrary to my previous comment). Most people aren't aware that this bug even exists, and they assume that managing the featured image works in the way that this ticket is hinting it should.

About the previous concerns whether this is expected behavior or not: While generally media-related changes are saved immediately with AJAX, this shouldn't be the case in the context of posts. The featured image does not change the attachment itself, but the post that it is attached to (similar like inserting media into the post content). Therefore it should only do something on hitting the "Update" button.

To fix the ticket, we should move away from managing the featured image through AJAX and instead use a hidden input field to store the image ID (or 0 for no featured image or to remove the existing featured image). The POST data this would send could then be handled in edit_post() (in a similar fashion to the current AJAX implementation).

@flixos90
3 years ago

patch to only save featured image on "Update"

#14 @flixos90
3 years ago

  • Keywords has-patch added; needs-patch removed

12922.diff is a take on this ticket.

The featured thumbnail ID is stored in a hidden input field with name _thumbnail_id which is submitted when hitting the "Update" button and then handled by edit_post().

The AJAX function for set-post-thumbnail is no longer used, instead a new AJAX function for get-post-thumbnail-html is used to render the metabox content (basically providing a preview, similar to the original set-post-thumbnail function, only without actually modifying the post).

Additionally I moved all thumbnail management for the post editor into the media-editor.js file so that we don't need to use the global JS function WPRemoveThumbnail any longer.

This ticket was mentioned in Slack in #core by flixos90. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by flixos90. View the logs.


3 years ago

#17 @flixos90
3 years ago

  • Milestone changed from Awaiting Review to 4.6

This bug was supposed to be fixed in 3.5. I will compare 3.5 with trunk to test their behaviors.

#18 @flixos90
3 years ago

Reference: This bug was fixed in 21770 and then reappeared after 22979.

@flixos90
3 years ago

@flixos90
3 years ago

#19 @flixos90
3 years ago

  • Owner set to flixos90
  • Status changed from reopened to accepted

After comparing, the initial patch appears to integrate the original changes from 21770 into trunk well. In 12922.3.diff I did some fine tuning by removing duplicate code and fixing some docs.

@flixos90
3 years ago

#20 @flixos90
3 years ago

  • Keywords commit added

12922.4.diff has some minor improvements in the wp_ajax_get_post_thumbnail_html() function.

#21 follow-up: @swissspidy
3 years ago

Additionally I moved all thumbnail management for the post editor into the media-editor.js file so that we don't need to use the global JS function WPRemoveThumbnail any longer.

Should we remove/deprecate this function then? It's not used anywhere else.

#22 in reply to: ↑ 21 @flixos90
3 years ago

Replying to swissspidy:

Additionally I moved all thumbnail management for the post editor into the media-editor.js file so that we don't need to use the global JS function WPRemoveThumbnail any longer.

Should we remove/deprecate this function then? It's not used anywhere else.

Sounds good to me. Also, we might consider doing something similar to wp_ajax_set_post_thumbnail().

#23 @ocean90
3 years ago

  • Keywords commit removed

It seems like the latest patch doesn't handle the "Preview Changes" button for published posts. Possibly related to #20299.

#24 follow-up: @westonruter
3 years ago

BTW: The Customize Posts plugin allows for a featured image change to be fully previewed in the Customizer. From first glance at the patch on this ticket, it also changes the behavior of the Featured Image metabox on the edit post screen to prevent the featured image from being set as soon as it is selected: instead, it populated a hidden input with the attachment ID which then gets handled during a save_post action. See https://github.com/xwp/wp-customize-posts/blob/0.6.1/php/class-wp-customize-featured-image-controller.php

#25 in reply to: ↑ 24 ; follow-ups: @flixos90
3 years ago

Replying to westonruter:

BTW: The Customize Posts plugin allows for a featured image change to be fully previewed in the Customizer. From first glance at the patch on this ticket, it also changes the behavior of the Featured Image metabox on the edit post screen to prevent the featured image from being set as soon as it is selected: instead, it populated a hidden input with the attachment ID which then gets handled during a save_post action.

Not setting the featured image immediately was the original cause for this ticket. When we get this fixed, the Customize Posts plugin needs to be adjusted to that as well I think. If you have any concerns that this would not be possible with the approach of the current patch, please let me know.

Replying to ocean90:

It seems like the latest patch doesn't handle the "Preview Changes" button for published posts.

I will look at this by the end of this week. If it is indeed caused by to #20299, can we ignore it for the sake of this ticket? Or do we need to workaround?

#26 in reply to: ↑ 25 @westonruter
3 years ago

Replying to flixos90:

Not setting the featured image immediately was the original cause for this ticket. When we get this fixed, the Customize Posts plugin needs to be adjusted to that as well I think. If you have any concerns that this would not be possible with the approach of the current patch, please let me know.

Yes, Customize Posts can disable its implementation once it is in core. I shared the code from the Customize Posts implementation in case it is helpful for you.

#27 in reply to: ↑ 25 @ocean90
3 years ago

Replying to flixos90:

Replying to ocean90:

It seems like the latest patch doesn't handle the "Preview Changes" button for published posts.

I will look at this by the end of this week. If it is indeed caused by to #20299, can we ignore it for the sake of this ticket? Or do we need to workaround?

Not updating the featured image in a preview doesn't sound like a good idea.

@flixos90
3 years ago

@flixos90
3 years ago

#28 @flixos90
3 years ago

I have two updated patches which take different approaches on fixing the previewing issues:

12922.5.diff actually uses revision post meta to store the post thumbnail. It does not open up post meta to revisions generally, but adds an exception for the _thumbnail_id meta key. The patch ensures that this meta field is stored as revision meta for all revisions and also covers autosaving.

12922.6.diff uses a similar approach like we currently have for the post format. It simply passes the current value of the _thumbnail_id to the preview as a query argument.

After having thought about and tested both implementations, I think we should rather go with the latter for now. While it is more a workaround than an actual solution, we can still change it when we actually start supporting revision meta at some point (see #20299). The first approach works as well, but I'm not really fond of using revision meta for this change before we generally open it up, especially since this would include adding all these exceptions for the _thumbnail_id meta key.

Furthermore the revision meta approach could easily cause other issues we wouldn't wanna deal with that late in Beta cycle. The second one is more straightforward, so we might be able to deliver it with 4.6.

This ticket was mentioned in Slack in #core by flixos90. View the logs.


3 years ago

#30 @Kelderic
3 years ago

I noticed this when I was creating the Featured Galleries plugin, which behaves very similar to Featured Images. I solved it by storing two separate DB entries. A temp and a perm. The temp is updating via AJAX, and it is substituted into pages if the page is currently a preview. The perm value is updating when the page is saved. Finally whenever the page is opened, the temp is overwritten by the perm.

I'm not sure if this would have a huge impact on DB size, but it does solve all problems with previewing.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


3 years ago

#32 @joemcgill
3 years ago

Replying to flixos90:

After having thought about and tested both implementations, I think we should rather go with the latter for now. While it is more a workaround than an actual solution, we can still change it when we actually start supporting revision meta at some point (see #20299). The first approach works as well, but I'm not really fond of using revision meta for this change before we generally open it up, especially since this would include adding all these exceptions for the _thumbnail_id meta key.

I had the same reservations about adding exceptions for _thumbnail_id to revision post meta when I was looking over the two approaches. The approach to use query variables for previews in 12922.6.diff definitely feels like a workaround to me, but is a decent improvement until there is a better approach for handling revision post meta, because it gives people the ability to preview featured image changes on published posts without accidentally updating the post.

Previewing changes to drafts still seems to save any changes to the featured image, but I think it's fine to leave that behavior since the goal here is to keep someone from accidentally changing the image of their published post without realizing it.

#33 @ocean90
3 years ago

Let's do 12922.6.diff. I've only a couple of minor things which can be fixed on commit:

@flixos90
3 years ago

#34 @flixos90
3 years ago

  • Keywords commit added

12922.7.diff is an updated patch that takes care of the above issues.

#35 @joemcgill
3 years ago

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

In 38118:

Post Thumbnails: Only update featured images when saving a post.

Previously, changing the post thumbnail of a published post in the edit screen
would immediately apply the change, rather than waiting for the post to be
saved before applying the update. This could lead to someone unintentionally
editing the post thumbnail on a published post, and made it impossible to
preview changes to post thumbnails on published posts before saving the change.

This introduces a new Ajax handler, wp_ajax_get_post_thumbnail_html() to
retrieve the HTML for the post thumbnail meta box without updating the post
meta value for _thumbnail_id. It also allows post thumbnail changes to be
previewed by passing the _thumbnail_id as a query variable to the preview
screen and adding a new filter, _wp_preview_post_thumbnail_filter(), which
gets applied to get_post_metadata during the post preview process.

Props flixos90.
Fixes #12922.

#36 @ocean90
3 years ago

In 38137:

Post Thumbnails: Remove an unused nonce in _wp_post_thumbnail_html().

See #12922.

#37 @ocean90
3 years ago

In 38263:

Post Thumbnails: Restore thumbnail support for media files.

  • Allow to add/remove a featured image to attachment:audio and attachment:video post types, see [27657].
  • Change conditionals to check for theme OR post type support.
  • Add tests for #12922.

Broken in [37658].

Props flixos90, joemcgill, DrewAPicture, wonderboymusic.
See #12922.
Fixes #37658.

#38 @ocean90
3 years ago

In 38264:

Post Thumbnails: Restore thumbnail support for media files.

  • Allow to add/remove a featured image to attachment:audio and attachment:video post types, see [27657].
  • Change conditionals to check for theme OR post type support.
  • Add tests for #12922.

Broken in [37658].

Merge of [38263] to the 4.6 branch.

Props flixos90, joemcgill, DrewAPicture, wonderboymusic.
See #12922.
See #37658.

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


12 months ago

Note: See TracTickets for help on using tickets.