Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Metaboxes registered after initial initialisation are not saved #44716

Closed
lgladdy opened this issue Oct 5, 2022 · 7 comments · Fixed by #45145
Closed

Metaboxes registered after initial initialisation are not saved #44716

lgladdy opened this issue Oct 5, 2022 · 7 comments · Fixed by #45145
Assignees
Labels
[Feature] Meta Boxes A draggable box shown on the post editing screen [Package] Edit Post /packages/edit-post

Comments

@lgladdy
Copy link

lgladdy commented Oct 5, 2022

Description

Somewhere around WordPress 5.8, Gutenberg stopped saving Advanced Custom Fields data for metaboxes which are added to pages after the page has been initialised.

This is because the hasMetaBoxes constant is only checked on init:

const hasMetaBoxes = select.hasMetaBoxes();

This means for any metaboxes added to the page after init (in ACF's case, this happens if a field group is only assigned to a specific category, and the user changes that page to that category), the check for shouldTriggerMetaboxesSave is always false:

const shouldTriggerMetaboxesSave =

ACF does dispatch setAvailableMetaBoxesPerLocation when it adds a new metabox after init, but that doesn't trigger the update for hasMetaBoxes to allow them to save.

We don't think this is an issue we can solve our side, short of registering a "hidden" meta box that is always present to trigger the save - we're also happy to raise a PR to help fix this in Gutenberg too, but would love some advice and direction on the best place to add the logic to fix this.

Thanks!

Step-by-step reproduction instructions

  1. Add a metabox to a Gutenberg page after it's been initialised.
  2. Save the page - because hasMetaBoxes was empty on init, the shouldTriggerMetaboxesSave check will return false when it should be aware new metaboxes have been added and save.

I threw together a minimum viable demo of the issue here: https://github.com/lgladdy/gutenberg-issue-44716-demo/blob/main/after-init.js

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

No

@colorful-tones colorful-tones added the [Package] Edit Post /packages/edit-post label Oct 5, 2022
@Mamaduka
Copy link
Member

Mamaduka commented Oct 5, 2022

Hi, @lgladdy

Can you provide a minimal code example to reproduce the issue without ACF?

@Mamaduka Mamaduka added the [Feature] Meta Boxes A draggable box shown on the post editing screen label Oct 5, 2022
@lgladdy
Copy link
Author

lgladdy commented Oct 5, 2022

@Mamaduka Sure! I've thrown together a quick plugin that shows the issue via a simple JS loaded into the block editor, https://github.com/lgladdy/gutenberg-issue-44716-demo

If you toggle between the window.setTimeout which adds the meta box after 2 seconds vs calling addMetabox() immediately on .ready() you can see the issue.

Updating a post where the meta box was added immediately, the editor will save metaboxes with a call to post.php after the main post update. For the meta box added after 2 seconds, that call to post.php never fires, because of the code I pointed to in the original issue post.

@lgladdy
Copy link
Author

lgladdy commented Oct 5, 2022

Oops, sorry - repo is now public.

The main code that needs to run to demo this is here: https://github.com/lgladdy/gutenberg-issue-44716-demo/blob/main/after-init.js - if you'd rather run it in some other way than a plugin!

@Mamaduka
Copy link
Member

Thanks for the reproduction code, @lgladdy!

I think the original behavior changed after the changes from #30617 landed in WP 5.8.x.

@jsnajdr, I know you're working on an improved flow for saving meta boxes as part of #44971. Would it be possible to account for lazy loaded meta boxes in that flow?

@jsnajdr
Copy link
Member

jsnajdr commented Oct 20, 2022

I think the original behavior changed after the changes from #30617 landed in WP 5.8.x.

Yes, the buggy behavior is a combination of #15041, which moved the hasMetaBoxes() check out of the subscribe listener, and of #30617 which stopped re-creating the subscription on every setAvailableMetaBoxesPerLocation call, together with a fresh hasMetaBoxes() call.

ACF does dispatch setAvailableMetaBoxesPerLocation when it adds a new metabox after init

@lgladdy This is probably not 100% reliable because setAvailableMetaBoxesPerLocation overwrites the data that were set by the previous call. If any metaboxes were created during page initialization, possibly by another plugin, calling setAvailableMetaBoxesPerLocation just with the one new dynamically added metabox will overwrite them.

Maybe we could change setAvailableMetaBoxesPerLocation to merge the new data with the old? The data structure is very amenable to merging, because all items have unique IDs.

I'm fixing the hasMetaBoxes() call in #45145, could you please verify that it indeed fixes the ACF issue?

@lgladdy
Copy link
Author

lgladdy commented Oct 20, 2022

Just to add my +1 that this resolves the issue in ACF. Thanks @jsnajdr! 🥳

@jsnajdr
Copy link
Member

jsnajdr commented Oct 20, 2022

@lgladdy I'm improving the setAvailableMetaBoxesPerLocation action in #45156, so that, as described above, existing metaboxes are not lost when new ones are registered at runtime. Please try it out!

@priethor priethor removed the [Status] In Progress Tracking issues with work in progress label Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Meta Boxes A draggable box shown on the post editing screen [Package] Edit Post /packages/edit-post
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants