Fix metabox reordering

[ribaricplusplus]

Description

Fixes #30071

Important: This PR also includes changes to WordPress core. I will submit those here: https://core.trac.wordpress.org/ticket/52818

Cause of issue:

postboxes.js wp-admin script (this one), which is responsible for saving meta box order, is initialized by setAvailableMetaBoxesPerLocation here:

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

Lines 279 to 281 in 8aa446b

	if ( window.postboxes.page !== postType ) {
	window.postboxes.add_postbox_toggles( postType );
	}

However, that function is called from post.php, here:

https://github.com/WordPress/WordPress/blob/57da3e781320dd62e3016624b310c23447d9e6d1/wp-admin/includes/post.php#L2379-L2381

But at that point the editor is not yet fully initialized because setupEditor action didn't run yet. So postType is still null, postboxes becomes initialized incorrectly (with null instead of proper post type), and then when reordering happens the reordered metaboxes are saved incorrectly.

How has this been tested?

Manually. Try changing metabox order. Steps to reproduce are given in the linked issue.

Types of changes

Bug fix

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

I'm not sure if anything needs to be changed for React Native:

• I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

[peterwilsoncc]

Confirmed this fixes the bug with some manual testing. Also checked the postType is passed as a string on trunk with some well placed console.logs and that is the case too.

[peterwilsoncc]

Reading the coding standards and wondering if this should be returned with the __unstable prefix too.

What am I missing? This is mainly for my own education rather than an opinion either way :)

[ribaricplusplus]

I was following the style established elsewhere in the code where __unstable and __experimental is often removed, for example here:

gutenberg/packages/edit-post/src/components/layout/index.js

Lines 17 to 25 in 04c7c76

	import {
	BlockBreadcrumb,
	__experimentalLibrary as Library,
	} from '@wordpress/block-editor';
	import { Button, ScrollLock, Popover } from '@wordpress/components';
	import {
	useViewportMatch,
	__experimentalUseDialog as useDialog,
	} from '@wordpress/compose';

Also, if I'm not mistaken, it seems that the __unstable and __experimental prefix is there for people using Gutenberg packages in their projects, so it's important not to document these functions and tell people to use them and so forth. Here it's just used locally and not exported anywhere.

That's just my reasoning for renaming it, but I'll put the __unstable prefix back if you think that would be better.

[paaljoachim]

I believe this PR could use some additional eyes...
@mcsf @ockham @youknowriad

[youknowriad]

Hi there! thanks for the fix here, it seems to me that the current way this call is handled could be improved, and this is highlighted by the issue being fixed here.

In other words and to avoid any timing issues in the future, what about a solution like this:

1- Remove the promise from here https://github.com/WordPress/WordPress/blob/57da3e781320dd62e3016624b310c23447d9e6d1/wp-admin/includes/post.php#L2379-L2381 and just call setAvailableMetaBoxesPerLocation action synchronously.

2- Split the setAvailableMetaBoxesPerLocation action creator to two separate action steps:
a- one that sets the state in the store (saves the active meta boxes)
b- one that triggers the post box script and also enables the meta boxes saving: all this code

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

Lines 275 to 322 in 8aa446b

	const postType = yield controls.select(
	'core/editor',
	'getCurrentPostType'
	);
	if ( window.postboxes.page !== postType ) {
	window.postboxes.add_postbox_toggles( postType );
	}
	let wasSavingPost = yield controls.select( 'core/editor', 'isSavingPost' );
	let wasAutosavingPost = yield controls.select(
	'core/editor',
	'isAutosavingPost'
	);
	// Meta boxes are initialized once at page load. It is not necessary to
	// account for updates on each state change.
	//
	// See: https://github.com/WordPress/WordPress/blob/5.1.1/wp-admin/includes/post.php#L2307-L2309
	const hasActiveMetaBoxes = yield controls.select(
	editPostStore.name,
	'hasMetaBoxes'
	);
	// First remove any existing subscription in order to prevent multiple saves
	if ( !! saveMetaboxUnsubscribe ) {
	saveMetaboxUnsubscribe();
	}
	// Save metaboxes when performing a full save on the post.
	saveMetaboxUnsubscribe = subscribe( () => {
	const isSavingPost = select( 'core/editor' ).isSavingPost();
	const isAutosavingPost = select( 'core/editor' ).isAutosavingPost();
	// Save metaboxes on save completion, except for autosaves that are not a post preview.
	const shouldTriggerMetaboxesSave =
	hasActiveMetaBoxes &&
	wasSavingPost &&
	! isSavingPost &&
	! wasAutosavingPost;
	// Save current state for next inspection.
	wasSavingPost = isSavingPost;
	wasAutosavingPost = isAutosavingPost;
	if ( shouldTriggerMetaboxesSave ) {
	dispatch( editPostStore.name ).requestMetaBoxUpdates();
	}
	} );

c- when setting the metaboxes, only call the second part if the editor is ready

3- Add an effect somewhere in the MetaBoxes component that monitors __unstableIsEditorReady and calls the action defined in b above when the flag is set to true.

The main reason I'm suggesting this is to make the code less dependent on the timing of the actions being called. Does that sound like a better solution?

[paaljoachim]

Associated PR:
Allow moving metaboxes to previously empty area
#25187

[ribaricplusplus]

Thank you for the review @youknowriad.

Your suggestions sound good. However, I realized that I will not be able to invest time into this PR for at least a month. Just wanted to let you know. If it's still relevant when I can invest time in it, I'll do it.

[desrosj]

Hey @ribaricplusplus, just wanted to see if you had some time that freed up to revise this PR. I'd love to wrap this one up and get it shipped in a 5.8.x release of WordPress.

[rodrigo-arias]

Hi, I'm having a similar issue with ACF that I think has the same origin. After inserting a taxonomy I cannot open the meta boxes. The error doesn't happen in WP 5.6.x

I've seen a similar report here: https://core.trac.wordpress.org/ticket/52818

It would be great if you could have this fix for the next minor version as right now it is hard to use the block editor since WP 5.7 with this bug.

Thank you

[ribaricplusplus]

I haven't forgotten about this, but finding time is a struggle!

I'll update you in the next few days about when I'll be able to work on this.

[ribaricplusplus]

I'll try to do it mid September.

[ribaricplusplus]

@youknowriad Hey. I've made the changes as you proposed, the patch to WordPress core is here: https://core.trac.wordpress.org/ticket/52818

Please review this again and let me know if something still needs to be changed.

[youknowriad]

This is looking very good to me, I'd appreciate some testing from folks relying on meta boxes a bit.

I assume this works even without the Core patch right (as we're supposed to support existing and previous WP versions in the Gutenberg plugin)

[ribaricplusplus]

I assume this works even without the Core patch right

@youknowriad Yes.

I'd appreciate some testing from folks relying on meta boxes a bit.

@desrosj Can you mention anyone who could potentially do some testing?

[ribaricplusplus]

@youknowriad I just realized that my previous solution was actually not good because the MetaBoxes component is created multiple times for different values of the location parameter. So the effect that initializes meta boxes would also run multiple times.

Now I extracted the meta box initializing logic into a separate action on the edit-post store.

[youknowriad]

Can we maybe rename this one areMetaBoxesInitialized, hasInitializedMetaboxes or something like that to match the naming scheme of the other selectors?

[ribaricplusplus]

Done

[youknowriad]

Still looking good. Good point on the extraction to an action + isInitialized check.

[youknowriad]

Thanks for your work and persistence here :)

[dariaknl]

Tested on a last checkout from trunk: the re-ordering bug from the original issue is not reproduced. Re-ordering is kept after updating the post.