WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 15 months ago

Last modified 3 months ago

#33742 closed defect (bug) (fixed)

Menu Customizer: Adding draft items?

Reported by: pavelevap Owned by: westonruter
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.3
Component: Menus Keywords: has-unit-tests needs-patch
Focuses: administration Cc:

Description (last modified by westonruter)

I can add draft items into menu on classic menu page, but it is not possible with Customizer. I am not sure if it should be removed from admin page (items are not visually marked and mistake possible) or added to Customizer, but it should be consistent.

Originally mentioned in #33665.

Attachments (4)

33742.diff (479 bytes) - added by welcher 16 months ago.
draft-nav-menu-items.png (67.0 KB) - added by westonruter 16 months ago.
33742.2.diff (1.1 KB) - added by westonruter 16 months ago.
33742-tests.diff (1.6 KB) - added by welcher 15 months ago.

Download all attachments as: .zip

Change History (27)

#1 @westonruter
2 years ago

  • Description modified (diff)

#2 @westonruter
2 years ago

  • Version changed from trunk to 4.3

#3 @welcher
16 months ago

  • Focuses administration added
  • Keywords has-patch added

You can't add draft items to menus via the customizer. Attached is a patch to restrict the quick search to 'post_status' => true in the classic menu page.

@welcher
16 months ago

#4 @welcher
16 months ago

  • Keywords dev-feedback added

#5 @westonruter
16 months ago

  • Milestone changed from Awaiting Review to 4.7

@welcher thanks for raising this. Interesting. It does seem like a glitch (hack?) that nav menu items for draft posts/pages can be added to a nav menu, and remain in the nav menu even when an unauthenticated user is viewing. I would have thought such nav menu items would have been removed on the frontend in the same way as _is_valid_nav_menu_item is filtering ones that are _invalid. I'm not sure if this is a defect or a feature. If a defect, then I think your patch is correct. If a feature, then draft posts should be discoverable in the customizer as well. If a feature, this is closely related to #34923 since it involves adding nav menu items for auto-draft posts. I'm adding this ticket to the 4.7 milestone for this reason.

(Something else to note about nav menu items: the wp_update_nav_menu_item() function actually allows the nav_menu_item post itself to be made a draft (if you supply 'menu-item-status' => 'draft'). See draft-nav-menu-items.png for how the nav menu admin page looks when there is a nav menu item with a draft status (the blue notice and the word “(Pending)” after the nav menu item label). I've never ever seen this UI before today, although I have seen the reference to it in the code which I had to manually call to get this UI to appear. I don't think there is any UI to create draft nav menu items.)

@celloexpressions What are your thoughts on this? Is it a hidden feature to be able to add non-published posts to a nav menu? Or is it a defect?

#6 @welcher
16 months ago

@westonruter It honestly feels like a bug to me. The query that is pulling the search results doesn't have 'post_status' => 'any' set. In fact, it had no post_status parameter set at all - which (unless I'm missing something ) should mean that only published posts are being queried.

I haven't investigated but could there be a pre_get_posts happening that would change this or does having the s param disregard the post_status?

If we determine that this is a bug, I'm happy to remove my patch and mark it as a Good First Bug - feeling a little guilty about stealing a one-liner :)

#7 @jorbin
16 months ago

@welcher would you be willing to investigate and look into what is going on here? This definitely feels like a bug to me at first glance.

#8 @westonruter
16 months ago

  • Keywords dev-feedback removed

I'm inclined to agree with @welcher that it is a bug to be able to add non-publish posts to a nav menu, and so 33742.diff is the way to go. But actually, the underlying issue here is that the following logic isn't getting applied to searches like it is for when wp_nav_menu_item_post_type_meta_box runs:

<?php
if ( isset( $box['args']->_default_query ) )
                $args = array_merge($args, (array) $box['args']->_default_query );

So the fix I think is to make sure the same _default_query is merged with the query vars when a search as done, just as when the list generated. See 33742.2.diff which does this.

#9 @pavelevap
16 months ago

Interesting, but I do not think that it is a bug, but expected feature? See for example string pendingTitleTpl with comment /* translators: %s: title of menu item in draft status */ This string will be no longer needed in this case? And what about users using this feature? It can be used for scheduled posts added to menu before publication or it does not work this way?

#10 @westonruter
16 months ago

@pavelevap that is speaking of the nav_menu_item post itself, not the status of the underlying object that is being referenced. See my comment above about “draft nav menu items”. There is no UI to create such draft nav menu items, but there is a UI to show them if they are in a draft (pending) status.

#11 @pavelevap
16 months ago

Thank you, I did not realize that! So the only way to add draft item is publish some post, add it to menu and then unpublish it? It is only callback for unpublished items which were part of menu?

#12 @westonruter
16 months ago

@pavelevap no, if you add a nav menu which points to a published post, and then you unpublish that post, the nav menu item status does not change. The nav menu will continue to include that item, even though it points to a draft post and unauthorized users would not be able to see it. That's probably a separate (yet related) issue to what we're discussing here: limiting non-published posts from being available to be added to a nav menu vs filtering out nav menu items from a nav menu when they point to a non-published post.

#13 @welcher
16 months ago

I did some digging on this and it seems that when WP_Query is run without a post_status parameter in the admin, all post types that are available to the admin are queried:

class-wp-query ~ line 2345

if ( $this->is_admin ) {
    // Add protected states that should show in the admin all list.
    $admin_all_states = get_post_stati( array('protected' => true, 'show_in_admin_all_list' => true) );
    foreach ( (array) $admin_all_states as $state ) {
        $where .= " OR {$this->db->posts}.post_status = '$state'";
    }
}

The above generates SQL like the following:

AND (wp_posts.post_status = 'publish' OR wp_posts.post_status = 'future' OR wp_posts.post_status = 'draft' OR wp_posts.post_status = 'pending' OR wp_posts.post_status = 'private')

As such , the query is exhibiting expected behaviour - just the wrong behaviour for this use case.

At the end of the day, both patches are adding 'post_status' => 'publish' to the query but @westonruter 's is a more standardized approach. I'd go with it.

I do wonder now however if this needs a filter to allow this behaviour? I can imagine someone out in the wild is using this bug as a feature.

At any rate we should get some unit tests on this too.

#14 @welcher
16 months ago

  • Keywords needs-unit-tests added

#15 @welcher
15 months ago

  • Keywords dev-feedback 2nd-opinion added

#16 @welcher
15 months ago

  • Keywords has-unit-tests added; needs-unit-tests dev-feedback removed

I've added a unit test to demonstrate that the current functionality in _wp_ajax_menu_quick_search is indeed a bug. In 33742-tests.diff ] there are 4 posts created, 3 of which have post_statuses that should keep them out of standard queries. In theory, only the publish post status should be returned however all of them are.

@westonruter 's 33742.2.diff fixes the issue and the test passes. You can test by first applying 33742-tests.diff and then running the Core tests and then applying 33742.2.diff and re-running.

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


15 months ago

#18 @celloexpressions
15 months ago

  • Keywords 2nd-opinion removed

Sorry, just saw this. I think the ability to add drafts to menus is a bug; it probably initially made sense but can cause major UX issues, especially when getting to the front end site. Updating the admin to match the customizer result seems like the best action here.

#19 @westonruter
15 months ago

  • Owner set to westonruter
  • Resolution set to fixed
  • Status changed from new to closed

In 38584:

Menus: Prevent non-published posts/pages from being returned in search results for adding as nav menu items.

Re-use the same query vars in searching as when listing posts. Aligns with behavior of nav menus in customizer.

Fixes #33742.
Props welcher, westonruter.

#20 @westonruter
15 months ago

In 38588:

Docs: Fix phpdoc and jsdoc typos introduced in [38584] and [38587], respectively.

See #33742.
See #20714.

#21 @afercia
15 months ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm afraid [38584] broke the quick search in the Menus screen (nav-menus.php), where the search result should return a list with checkboxes and now returns instead a list of links:

trunk:

https://cldup.com/pXIHcdZPOX.png

4.6

https://cldup.com/oT_4Z-t8DD.png

#22 @westonruter
15 months ago

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

In 38702:

Menus: Restore checkboxes in post type search which were lost in [38584].

The $args variable was being overridden instead of extended, therefore causing the walker to be dropped.

Fixes #33742.

#23 @indranis789
3 months ago

I am new to wordpress, currently using online wordpress web face issue to insert a draft menu in Customize theme.

I had used create menu tab , then save menu but not worked for me.
Is this is still a issue in wordPress web?

Last edited 3 months ago by indranis789 (previous) (diff)
Note: See TracTickets for help on using tickets.