WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 years ago

#38072 new enhancement

Eliminate placeholder nav menu items in favor of auto-drafts in Customizer

Reported by: westonruter Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: needs-patch
Focuses: Cc:

Description (last modified by westonruter)

When nav menus were added to the customizer in 4.3, newly-created nav menu items were assigned a random negative integer to represent the ID for that nav_menu_item post. (This was also true of nav_menu terms for newly-created nav menus.) Upon saving the customized state, any such nav_menu_item[...] settings with negative IDs would then get inserted and assigned actual IDs which would then get sent back in the customize_save_response and the UI then replaces the placeholder nav menu item's control with the newly-inserted nav menu item's control. The key motivation here was to ensure that changes made in the customizer would not have an impact any part of WP until hitting Save. (What happens in the customizer stays in the customizer… until you hit Save.)

Now, aside from a momentary flicker of placeholder nav menu item controls that get replaced with actual nav menu item controls, there is a key disadvantage to using such placeholder nav menu items (with negative post IDs): it is not possible to relate postmeta to them. There is a long-standing ticket #18584 for allowing nav menu items to be extensible to add custom fields in the customizer (and in the admin screen). In the call to get_metadata it has a behavior whereby it passes the supplied post ID through absint so if any postmeta are attempted to be related to placeholder nav_menu_item posts, they will fail to be accessed when calling get_post_meta().

Now, in #34923 there was the introduction of being able to create stubs for posts and pages inside the customizer so that they can have nav menu items created for them. The stubs created here are auto-draft posts which ensures that they do not affect other parts of WordPress and they will be automatically garbage-collected if never published. Now, the original Menu Customizer plugin did make an Ajax request for each created new nav menu items but they were nav_menu_item posts that were not related to a nav_menu (they were orphaned) rather than being auto-draft. We could consider going back to making an Ajax request to create each nav_menu_item (now an auto-draft) in the same way that is being done for post/page stubs.

A downside of going to using Ajax to create new nav menu items (to reserve an auto-incremented post ID) is that adding a new nav menu item would no longer be instant. However, it would mean that upon save there wouldn't be any rebuild of nav menu item controls replacing placeholders with actuals, and as such it could mean a lot of code could be removed. But the most important benefit of switching to use auto-draft posts for nav menu items is that postmeta could then be created in the customizer and related to actual post IDs which could then be properly targeted in get_post_metadatafilters.

Alternatively, get_post_meta could just replace the absint with a call to intval and then ensure that the get_post_metadata filters apply with that negative ID, but then short-circuit if the filter doesn't return with null (since it wouldn't be able to find entries with negative IDs in the database).

See feature plugin for adding custom fields to the customizer: https://github.com/xwp/wp-customize-nav-menu-item-custom-fields
Note how the plugin has to postpone the presentation of custom fields until a newly-added nav menu item is saved the first time: https://github.com/xwp/wp-customize-nav-menu-item-custom-fields/blob/2ad056634441a74ba91982ca88b089297f630971/customize-nav-menu-item-custom-fields.js#L279-L284

Dependency for: #18584

Change History (10)

#1 @celloexpressions
3 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

+1. I would probably prefer to wait for #38015 so that we could also take a similar approach for new menu terms at the same time. As far as I can remember from the original solution with orphaned drafts, they essentially work the same as auto drafts for nav menu item posts. However, the approach outlined here would be good for parity with the way new pages are managed in menus and eventually for post editing. Now that there isn't an Ajax call to add items, on the user side we'd need to come up with a way to keep that from slowing things down; perhaps we could update all items that were added with a single Ajax call when the add-items panel is closed. Essentially, using a combination of auto drafts and placeholders.

The intval is probably a good idea for now, as this will require quite a bit of refactoring.

#2 @westonruter
3 years ago

@celloexpressions I'm not sure I understand the dependency on #38015 or terms. Since nav menu items are just posts, then we can use straight auto-draft as well.

I'm not sure about using hybrid placeholders, other than perhaps a placeholder control could be added which could show some loading indicator and then get replaced with the actual control once the Ajax request completes. This would be similar to how Facebook reduces the sense of latency by showing stories with placeholder content while the page loads.

#3 @westonruter
3 years ago

I've opened #38082 specifically for allowing negative IDs in calls to get_metadata.

#4 @celloexpressions
3 years ago

  • Version changed from 3.4 to 4.3

The term dependency would be for the menu term that groups menu items into a menu. I might be remembering incorrectly but I though we used placeholder ids for both the menu item posts and the menu terms currently. If not, we could switch to auto draft for the posts now, although I'm a bit wary of making a big structural change like this without there being clear benefits to the end user yet.

#5 @westonruter
3 years ago

The user benefit would be allowing for postmeta to be attached to pre-saved nav menu items (#18584).

#6 @westonruter
3 years ago

  • Description modified (diff)
  • Milestone changed from Future Release to 4.8

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


3 years ago

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


2 years ago

#9 @westonruter
2 years ago

  • Milestone changed from 4.8 to Future Release

#10 @westonruter
2 years ago

See #32183 for doing the same for widgets.

Note: See TracTickets for help on using tickets.