WordPress.org

Make WordPress Core

Opened 4 weeks ago

Closed 3 weeks ago

Last modified 3 weeks ago

#54708 closed defect (bug) (fixed)

Fix block theme featured image preview

Reported by: adamsilverstein Owned by: audrasjb
Milestone: 5.9 Priority: normal
Severity: normal Version: trunk
Component: Revisions Keywords: has-patch has-screenshots commit dev-reviewed
Focuses: Cc:

Description (last modified by adamsilverstein)

(Improve _set_preview for case when autosave missing)

Problem: Autosaves are deleted when a new autosave is sent with the same data. In the block editor, this causes the preview data to be missing on previews.

Bug exhibited: When using block themes, post previews do not properly display the currently set featured image. Several other data transformations are also missing because _set_preview exits early when it can't locate the autosave.

When investigating an issue with previews in block themes not working correctly, I found an underlying bug that prevents autosaves from being used correctly for previews in some cases. (See PR comment).

In src/wp-includes/rest-api/endpoints/class-wp-rest-autosaves-controller.php in the create_post_autosave function - when the autosave sent to the autosave REST endpoint exactly matches the previous autosave, the controller deletes the old autosave revision, then returns an error "There is nothing to save. The autosave and the post content are the same."

Deleting the old autosave revision means that later when src/wp-includes/revision.php _set_preview gets called, and it calls wp_get_post_autosave the autosave is missing, and the preview data swaps are not activated.

The end result of this is that currently if you set a preview image using a block theme on a published post and preview it, the featured image is not displayed correctly. Skipping deleting the previous autosave fixes the issue.

Attachments (6)

block_preview_fail.mp4 (3.8 MB) - added by adamsilverstein 4 weeks ago.
54708.diff (691 bytes) - added by adamsilverstein 4 weeks ago.
54708.2.diff (1.3 KB) - added by adamsilverstein 4 weeks ago.
54708.3.diff (896 bytes) - added by adamsilverstein 4 weeks ago.
089bb353f3d436ac10ac9a46df69a053.gif (1.2 MB) - added by audrasjb 4 weeks ago.
Before 54708.3.diff - I can reproduce the issue
626066db5cb15bae17b08eefd0aa6a6a.gif (4.6 MB) - added by audrasjb 4 weeks ago.
After 54708.3.diff - the issue no longer occurs

Change History (30)

#1 @adamsilverstein
4 weeks ago

  • Description modified (diff)

This ticket was mentioned in PR #2092 on WordPress/wordpress-develop by adamsilverstein.


4 weeks ago

  • Keywords has-patch added

#3 @adamsilverstein
4 weeks ago

  • Keywords has-patch removed

I found several users reporting related bugs on trac: #49532, #52933

I tracked down the source of why we chose to delete this autosave in the endpoint: https://core.trac.wordpress.org/ticket/43316#comment:62 --

"if the post content is identical to the new content that is being auto-saved, the previous autosave revision is deleted to prevent having a redundant revision (i.e. if the update of the previous autosaved revision will make it identical to the current post, there is no need of it)."

This makes sense... except that since we return an error here, a new autosave is never created. So the post now lacks any autosave, and this later causes _set_preview to bail early - since it relies on the autosave, breaking preview functionality.

I would propose that since the autosave already exists with the correct data, we should leave it in place so previews continue working as expected. An alternate solution would be to update the login in _set_preview to work even without an autosave present.

Last edited 4 weeks ago by adamsilverstein (previous) (diff)

#4 @adamsilverstein
4 weeks ago

  • Keywords dev-feedback has-patch 2nd-opinion added
  • Owner set to adamsilverstein
  • Status changed from new to assigned

In 54708.2.diff:

When sending the data to the autosave REST API endpoint:

  • If no autosave exists for the user, create one
  • If an autosave exists for the user, update it

Also:

  • Remove logic that previously would delete existing autosaves that matched the post content which had the unfortunate side effect of breaking previews because preview setup requires and checks for the presence of an autosave.

#5 @adamsilverstein
4 weeks ago

  • Description modified (diff)
  • Summary changed from Retain existing autosave when new autosave is the same to Improve `_set_preview` for case when autosave missing

#6 follow-up: @adamsilverstein
4 weeks ago

After some digging here the underlying bug is actually in _set_preview - when an autosave is missing for a post, the current function bails early, and never applies the tem or thumbnail filters:

add_filter( 'get_the_terms', '_wp_preview_terms_filter', 10, 3 );
add_filter( 'get_post_metadata', '_wp_preview_post_thumbnail_filter', 10, 3 );

The reason an autosave may not exist is: when the autosave content, title and excerpt and content exactly match it's parent post, we delete it. In this case, we can skip the reassignment of these values, but still apply the correct filters to the preview.

#7 follow-up: @adamsilverstein
4 weeks ago

In 54708.3.diff:

  • In _set_preview, when the autosave is missing, skip assignment of content, title and excerpt but still apply filters to get_the_terms and get_post_metadata.

#8 in reply to: ↑ 7 @walbo
4 weeks ago

54708.3.diff looks good to me.

Can confirm the patch fixes the featured image in block based themes when previewing a post. Also tested in twenty twenty one to confirm there is no regression in non block based themes

#9 @adamsilverstein
4 weeks ago

  • Milestone changed from Awaiting Review to 5.9
  • Version set to trunk

Marking this for 5.9 since it corrects a pretty serious regression where featured images no longer show consistently in previews for block themes. Alternately we could land after release and backport in the first "point" release. I would appreciate a 2nd committer opinion here.

This problem has actually existed for as long as the autosave endpoint, and back in 2018 we added an extra query variable to previews in Gutenberg just to work around the issue (see https://github.com/WordPress/gutenberg/issues/9151).

The block theme featured image rendering surfaced this bug again, fixing it in core makes more sense though, this also fixes term filtering for previews which would also break when autosaves are missing.

#10 @adamsilverstein
4 weeks ago

  • Description modified (diff)
  • Summary changed from Improve `_set_preview` for case when autosave missing to Fix block theme featured image preview

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


4 weeks ago

#12 @adamsilverstein
4 weeks ago

  • Description modified (diff)

@audrasjb
4 weeks ago

Before 54708.3.diff - I can reproduce the issue

@audrasjb
4 weeks ago

After 54708.3.diff - the issue no longer occurs

#13 @audrasjb
4 weeks ago

  • Keywords has-screenshots added

I was able to reproduce the issue, and 54708.3.diff fixes it on my side.

#14 @Mamaduka
3 weeks ago

I tested the patch, and the fix works as expected. Thanks, @adamsilverstein.

#15 @audrasjb
3 weeks ago

  • Keywords commit added; 2nd-opinion removed

Marking for commit since it was successfully tested.

#16 in reply to: ↑ 6 @hellofromTonya
3 weeks ago

  • Keywords dev-reviewed added; dev-feedback removed

Replying to adamsilverstein:

After some digging here the underlying bug is actually in _set_preview - when an autosave is missing for a post, the current function bails early, and never applies the tem or thumbnail filters:

add_filter( 'get_the_terms', '_wp_preview_terms_filter', 10, 3 );
add_filter( 'get_post_metadata', '_wp_preview_post_thumbnail_filter', 10, 3 );

The reason an autosave may not exist is: when the autosave content, title and excerpt and content exactly match it's parent post, we delete it. In this case, we can skip the reassignment of these values, but still apply the correct filters to the preview.

Aha, great find Adam! I too was able to reproduce the issue. Can confirm 54708.3.diff resolved it.

#17 @adamsilverstein
3 weeks ago

I will be away from my computer for a couple of weeks: anyone is welcome to commit this.

#18 @audrasjb
3 weeks ago

Alright, let's get this fixed then.
Self assigning for commit.

#19 @audrasjb
3 weeks ago

  • Owner changed from adamsilverstein to audrasjb

#20 @audrasjb
3 weeks ago

  • Component changed from General to Media

#22 @audrasjb
3 weeks ago

  • Component changed from Media to Revisions

Let's switch the ticket to the Revisions component :)

Checks are passing. Committing the patch.

#23 @audrasjb
3 weeks ago

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

In 52433:

Revisions: Improve _set_preview for case when autosave is missing.

This change fixes an issue where autosaves are deleted when a new autosave is sent with the same data. In the block editor, this causes the preview data to be missing on post previews. The end result of this is that if one set a preview image using a block theme on a published post and preview it, the featured image is not displayed correctly. Skipping deleting the previous autosave fixes the issue.

Props adamsilverstein, walbo, audrasjb, Mamaduka, hellofromTonya.
Fixes #54708.

Note: See TracTickets for help on using tickets.