Fix meta save causing autosave deletion, resulting in preview not displaying unsaved changes

[talldan]

Description

Fixes #12617 - when metaboxes are active, the preview does not show unsaved changes to posts.

A quick summary of the cause:

1. The user clicks 'preview', which first triggers an autosave.
2. After the autosave is complete, we POST to post.php to update metaboxes (with some special query arguments to ensure a full post update is not carried out).
3. The request to post.php updates the modified date of the post and triggers some housecleaning logic to remove old autosaves—now that the post is newer the autosave from step 1 is removed.
4. The preview is displayed, but since the autosave has been deleted, the preview falls back to the outdated post content.

This PR fixes the issue by changing the save order. First save the metaboxes, then the autosave.

It does so by introducing a new filter 'editor.updatePost', which the metabox code (in the edit-post package) uses to defer the post update until after metaboxes have been saved.

How has this been tested?

1. Create a new post.
2. Enable 'custom fields' from the options menu.
3. Add a custom field.
4. Add some post content.
5. Save the post.
6. Add more post content.
7. Click preview.

Expected:
The preview displays the content added in step 4.

Previous Behaviour:
The content added in step 4 wasn't displayed.

Screenshots

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[talldan]

Thanks to @noisysocks for being a helping hand in coming up with a strategy for this fix.

I do want to look into writing an e2e test for this (if possible), but thought I'd get the fix PR up as it's a fairly high priority one.

[talldan]

Does addFilter prevent multiple filters being registered or could it be worth a call to removeFilter first, in case this SET_META_BOXES_PER_LOCATIONS action is called more than once?

[talldan]

Cursory glance at the code and it looks like I should probably add a call to removeFilter to be safe.

[adamsilverstein]

yes, otherwise you will add multiple callbacks

[youknowriad]

This PR fixes the issue by changing the save order. First save the metaboxes, then the autosave.

This is a very risky change, I'm a bit concerned about introducing this so late in the process.

4. The preview is displayed, but since the autosave has been deleted, the preview falls back to the outdated post content.

According to your description, the autosave that was deleted is the one from the step 1, why can't we just show the autosave from step 3 instead when previewing?

[talldan]

why can't we just show the autosave from step 3 instead when previewing?

@youknowriad At that stage it's a POST request to post.php with only partial data:

gutenberg/packages/edit-post/src/store/effects.js

Lines 93 to 117 in 7980d19

	const baseFormData = new window.FormData( document.querySelector( '.metabox-base-form' ) );
	const formDataToMerge = [
	baseFormData,
	...getActiveMetaBoxLocations( state ).map( ( location ) => (
	new window.FormData( getMetaBoxContainer( location ) )
	) ),
	];
	// Merge all form data objects into a single one.
	const formData = reduce( formDataToMerge, ( memo, currentFormData ) => {
	for ( const [ key, value ] of currentFormData ) {
	memo.append( key, value );
	}
	return memo;
	}, new window.FormData() );
	additionalData.forEach( ( [ key, value ] ) => formData.append( key, value ) );
	// Save the metaboxes
	apiFetch( {
	url: window._wpMetaBoxUrl,
	method: 'POST',
	body: formData,
	parse: false,
	} )
	.then( () => store.dispatch( metaBoxUpdatesSuccess() ) );

The URL looks something like this:

"http://localhost:8888/wp-admin/post.php?post=1&action=edit&meta-box-loader=1"

The meta-box-loader param ensures the block editor isn't loaded:
https://github.com/WordPress/WordPress/blob/a89b6c042dc6f677edaf519af76abb0c4bda473e/wp-admin/includes/post.php#L2065-L2076

On the balance of it, I felt like that code would be more complicated to change.

[youknowriad]

Even if it's a partial request, it will update the existing post object already saved and create a revision I guess.

---

To clarify I'm not totally against changing the order of the save requests, but I do think it also impacts that code as I wonder if we should still do the autosave removal if we change the order?

That said, the approach in the PR has another benefit. As I think if done properly, it could be something we could leverage to fix this too #13413

We may need people familiar with Core PHP to help here so pinging @pento @desrosj @BE-Webdesign

[noisysocks]

That said, the approach in the PR has another benefit. As I think if done properly, it could be something we could leverage to fix this too #13413

My ulterior motive for suggesting this approach when discussing with @talldan was to introduce a way to fix #13413 😛

[talldan]

#12903 details that some plugin developers have code that depends on the order of the requests, currently detecting when the meta save has completed to trigger a side-effect. That means this PR could cause some breakage.

On the other hand, it might also make development using server-side hooks a bit easier, since devs would be able to rely only on the REST API request to determine when a save has occurred.

[dlh01]

I would second from experience that switching the save order would be a consequential change for plugin developers. In my opinion, it would at a minimum deserve a merge into core with related documentation early in a core release cycle.

An alternative approach was suggested recently on Trac, which addressed the problem via the redirect_post_location filter: https://core.trac.wordpress.org/ticket/45768#comment:7.

That alternative apparently works around the issue in #12617 by avoiding the second $_GET request entirely -- similar in effect to the approach @talldan suggested in #12617 (comment) but without modifying the autosave-deletion logic itself.

If the approach in this PR is chosen -- which still might be the best move, all things considered -- then I think the backwards-compatibility concerns that come with it could be lessened by also offering some comments on the alternative approaches, which I don't think have the same concerns.

[talldan]

Closing this for now, as commented on #12617 and #13232, this appears to have been fixed by a separate change.

edit: seems like I was wrong about that. Reopening this PR.

[talldan]

I've looked into rebasing this, but I don't think the solution here is viable any longer.

Originally this code used the fact that the effect that handled saving the post was just a simple function that a filter could be applied to.

Recently though, that code has been refactored and now a generator function is used to save the post. There might be ways to use a filter here, but I think it's just going to make the code very unusual and the solution even less acceptable.

I'll have a look again at possible server-side solutions for the bug.