Make WordPress Core

Opened 9 years ago

Closed 8 years ago

Last modified 7 years ago

#33742 closed defect (bug) (fixed)

Menu Customizer: Adding draft items?

Reported by: pavelevap's profile pavelevap Owned by: westonruter's profile 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 8 years ago.
draft-nav-menu-items.png (67.0 KB) - added by westonruter 8 years ago.
33742.2.diff (1.1 KB) - added by westonruter 8 years ago.
33742-tests.diff (1.6 KB) - added by welcher 8 years ago.

Download all attachments as: .zip

Change History (27)

#1 @westonruter
9 years ago

  • Description modified (diff)

#2 @westonruter
9 years ago

  • Version changed from trunk to 4.3

#3 @welcher
8 years 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
8 years ago

#4 @welcher
8 years ago

  • Keywords dev-feedback added

#5 @westonruter
8 years 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
8 years 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
8 years 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.

@westonruter
8 years ago

#8 @westonruter
8 years 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
8 years 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
8 years 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
8 years 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
8 years 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
8 years 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
8 years ago

  • Keywords needs-unit-tests added

#15 @welcher
8 years ago

  • Keywords dev-feedback 2nd-opinion added

@welcher
8 years ago

#16 @welcher
8 years 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.


8 years ago

#18 @celloexpressions
8 years 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
8 years 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
8 years ago

In 38588:

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

See #33742.
See #20714.

#21 @afercia
8 years 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
8 years 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
7 years 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 7 years ago by indranis789 (previous) (diff)
Note: See TracTickets for help on using tickets.