Menu items that get unpublished still appear

[nacin]

We need to properly account for menu items linked to unpublished/pending post type objects.

My thought is they should probably be hidden from the frontend with an indication on the backend (like "(Pending)") that they are are unpublished/pending.

We need to properly handle private posts too.

[wpmuguru]

A patch to kick around

[wpmuguru]

That patch takes the menu item out on both the front end and in the menu editor. I didn't test, but I expect if the user saves the menu, the menu item would be removed. That's not ideal but is better than putting out a link that will 404.

[nacin]

We don't want to remove pending/draft posts from the admin, I don't think. When I discussed this with Jane, appending a label like "(Draft)" (which could probably cover both) in the admin would be the best solution.

Then we can just skip over non-published posts in the front end.

Do we want to ignore posts that become private? I say skip right over them, assuming they have to be publicly published to show up in the admin.

This is somewhat edge, as it only 404's the link and I don't think we allow non-published posts to be added in the first place, so its status will have to change after the fact. However, it does reveal some information about the post.

Considered a blocker for RC3.

[filosofo]

Got an idea for implementing the "(Draft)" label, and working on a patch.

[filosofo]

sync-menu-item-status-to-orig-object.13822.diff does the following:

• Renames _wp_auto_add_pages_to_menu to _wp_menu_changing_status_observer and uses it to catch parent object status changes in addition to new top-level page publishing.
• Makes what we've been calling "pending" menu items actually have "pending" status instead of "draft" (duh?)
• If an associated post object becomes "draft" from "publish," the menu item gets "draft" status.
• If an associated post object becomes "publish" from "draft" (re-published), the menu item is re-published too.
• Pending menu items behave the same as before.
• Draft menu items' statuses are tied to their associated post objects; in other words, unlike pending menu items, saving the menu does not change a menu item's status from "draft" to "publish."

[wpmuguru]

(In [15219]) hide unpublished items on frontend nav menus, props filosofo, fixes #13822

[nacin]

I'm seeing some issues with [15219].

Listening for status changes is not very effective, as you can't easily account for all options.

The commit does not account for pending, private, or scheduled posts, only draft posts. Additionally, the menu item is only triggered back to publish again when it goes straight from draft to publish. It might very well pass through pending or scheduled on the way to publish.

I liked wpmuguru's take, though not the patch, and had tried to patch this in wp_setup_nav_menu_item() because we pull the post object and could easily check if the post status is not "publish," and then exclude it from the menu. Unfortunately, the way we call wp_setup_nav_menu_item() (through array_map) means we can't simply unset the item, and since we map arrays through wp_setup_nav_menu_item like a dozen places, we'd have to make a lot of changes.

We could pair array_map() calls with array_filter(), and return null from wp_setup_nav_menu_item(), but that's not very DRY. I think at this point that's our best case scenario however, as it is a minimal code change, and we would revert much of [15219].

[nacin]

koopersmith and I are talking about this now, and we have two potential solutions we're both working on.

One is a variation of wpmuguru's patch, and the other is a variation of my initial approach. Both have the advantage of piggybacking on an existing get_post/get_posts call, and both would only be triggered when we want it (i.e. not in the admin).

[nacin]

Would revert [15219].

[nacin]

Patch from koopersmith and me reverts [15219] and makes some important changes to properly fix this ticket:

• wp_nav_menu() now passes an argument to wp_get_nav_menu_items(), telling that function to use a wrapper for wp_setup_nav_menu_item(), which would then filter out unpublished posts, only on the frontend, and during menu display, not on any hooks.

• Menu items that are associated with a post that is unpublished now gets a status after it, such as "(Draft)" and "(Pending)", using the post status object label.

• "(Pending)" in non-JS becomes "(Unsaved)" because "Pending" is an existing status we also need to display. i18n-change, we're adding "%s (Unsaved)". Also, "Click Save Menu to make pending menu items public." becomes "Click Save Menu to make unsaved menu items public." (Redundant/obvious, but a yellow warning never hurt.)

• Removes the trash/untrash post hooks and other special trash handling. They didn't work properly, as they should have returned the menu item to the end of the menu on untrash. Now, we handle it like other stati, i.e. "(Trash)". i18n-change, we're adding "Original: %1$s (%2$s)", that way we reflect the status here too.

This patch is scary -- there's a lot being changed, or so it seems. I think we should do this in pieces, at least two commits if not more. First, revert [15219], then fix frontend, labels, and trash. Once [15219] is out it'll look much more sane.

[markjaquith]

This patch is scary -- there's a lot being changed, or so it seems. I think we should do this in pieces, at least two commits if not more. First, revert [15219], then fix frontend, labels, and trash. Once [15219] is out it'll look much more sane.

Agreed. Go ahead and do that.

[filosofo]

Replying to nacin:

Listening for status changes is not very effective, as you can't easily account for all options.

The commit does not account for pending, private, or scheduled posts, only draft posts.

True, but it seems like the proposal changes a single-point logic failure into an extensive code rewrite. Fix the logic at the point of failure, instead.

Among my concerns:

• This proposal rewrites lots of code to deal with a niche situation that has, at worst, a dead link as its consequence.
• The problem can be better solved at object-change time (as it currently does):
  • When the associated object changes is the right time to change its associated menu item.
  • This proposal puts additional burdens on the public generation of menus, rather than handling the issue one time when it happens.
• We should not be putting query logic into wp_setup_nav_menu_item, as this proposal does. wp_setup_nav_menu_item simply decorates an object to give it the requisite standard menu item properties.
  • This proposal takes wp_setup_nav_menu_item, a function that does one thing, and one thing well, and gives it additional things to do, which means the bug possibilities open up proportionally.
  • By having wp_setup_nav_menu_item handle additional arguments, it introduces the possibility of returning null having passed in an object, which doesn't make sense for something that's only supposed to add properties.
• We should not be putting view logic into wp_get_nav_menu_items
  • It makes wp_get_nav_menu_items more complicated: instead of getting menu items and passing them to wp_setup_nav_menu_item to decorate them, the proposal has it determining view state ('public_consumption'). This is the wrong place for that logic.
• r15219 also had logic simplification for auto-page-add. Those should not be reverted.

To re-iterate:

• I think this can be handled more simply.
• The potential problems of the status quo (in other words, dead menu links) do not warrant wholesale code change at this point.
• This proposal involves a number of questionable design decisions with long-term implications.

Therefore I strongly object to these proposed changes.

[filosofo]

As as starting point, what are the current problems not solved by generalize-object-change-logic.1.diff?

[nacin]

Dead link is not the only consequence, it is exposure of data that should be hidden.

I have ​attached a diff that shows my changes against r15218, which shows the limited changes of the diff (a lot of it is removing the specialized trash handling).

The patch you posted to get us going is missing the exclusion of $old_status == $new_status and also would probably duplicate efforts with trashing and untrashing of posts. The current implementation is not ideal UX, it overlapped with the no-JS pending, and triggered a bunch of bugs related to unpublished handling, starting with menu items associated with trashed posts. On the other hand, 13822-reverts-r15219.3.diff is ready for commit. I thought [15219] was way too many changes so deep into an RC phase, and additional bandaids on it now is probably the wrong direction.

Handling this on generation is exactly how wp_list_pages/wp_page_menu functions, in that its get_pages() call only grabs published pages. I fail to see any simplicity in generalize-object-change-logic.1.diff that is not present in 13822-on-r15218.diff, and it's hardly a code rewrite. It's more code changes and more points for failure than what's currently in core.

This proposal puts additional burdens on the public generation of menus, rather than handling the issue one time when it happens.

Absolutely not. It's extraordinarily light code.

wp_setup_nav_menu_item() decorates a menu object based on the properties of post object it is associated with. If the properties of the post object invalidate the menu item for public display, that is clearly within the definition of setting up the menu item. It opens up no additional "bug possibilities".

I think most of the concerns outlined here are exaggerated. Your suggestions of where logic needs to go is largesly based on opinion, and I largely disagree with the assumptions made here. Calling it "questionable design decisions with long-term implications" is laughable and reactionary, and I think we both know that.

Two tactics here: focusing on synchronizing object stati with menu items, and handling this on generation. I frankly don't care which we go with, though I feel handling this on public generation would be far more versatile.

[filosofo]

Replying to nacin:

The patch you posted to get us going is missing the exclusion of $old_status == $new_status

How would that affect the end result?

and also would probably duplicate efforts with trashing and untrashing of posts.

OK, so add a check for when the new status is "trash." No big deal.

The current implementation is not ideal UX, it overlapped with the no-JS pending, and triggered a bunch of bugs related to unpublished handling, starting with menu items associated with trashed posts.

Please list the bugs. I still haven't seen any specifics. Again, I fail to see why we can't handle trashed posts with a couple lines in the observer.

I thought [15219] was way too many changes so deep into an RC phase, and additional bandaids on it now is probably the wrong direction.

That seems like a non-sequitur: there were too many changes, so in response we should make even more substantial ones?

Handling this on generation is exactly how wp_list_pages/wp_page_menu functions, in that its get_pages() call only grabs published pages.

The point is that it's not necessary in the first place: no matter how minimal, we're doing logic checks for a niche situation on every menu generation, logic checks that don't need to be done there at all.

And other functions like wp_list_pages are not a helpful analogy, because here we're querying objects (menu items) that are not the same as the objects causing the problem (posts, pages, etc.).

I fail to see any simplicity in generalize-object-change-logic.1.diff that is not present in 13822-on-r15218.diff

In generalize-object-change-logic.1.diff I see about 3 lines changed, as opposed to the dozens of new lines, several significant API changes and logic branches added in 13822-on-r15218.diff.

This proposal puts additional burdens on the public generation of menus, rather than handling the issue one time when it happens.

Absolutely not. It's extraordinarily light code.

It may be feather-light, but it's a feather that doesn't need to be there in the first place.

wp_setup_nav_menu_item() decorates a menu object based on the properties of post object it is associated with. If the properties of the post object invalidate the menu item for public display, that is clearly within the definition of setting up the menu item.

Actually, that's mixing up "view" with "model." The proposed changes attempt to change an object's data to respond to an issue of its presentation.

It opens up no additional "bug possibilities".

Instead of passing one object as the argument, now the proposal wants to pass an $args variable that is meant to branch on presentation logic. Imagine trying to create unit tests each for the current and the proposed, and you'll get what I mean.

I think most of the concerns outlined here are exaggerated. Your suggestions of where logic needs to go is largesly based on opinion, and I largely disagree with the assumptions made here.

Although what's considered good programming practice has a large element of the subjective (perhaps judgment and experience) mixed in it, that doesn't mean that it's simply a matter of opinion. I'm not clever enough to have come up with MVC, Uncle Bob's "Single Responsibility Principle," or Unix's "do one thing and do it well."

Calling it "questionable design decisions with long-term implications" is laughable and reactionary, and I think we both know that.

I've already raised questions against the design decisions; the long-term implications are that we'll be stuck with more-complicated, logically-convoluted code.

I am reacting to what seems like a disproportionately large change, in RC3, for a trivial problem.

Nothing about this makes me want to laugh.

[filosofo]

No one's going to read the previous comment, so let me boil it down to this:

Assuming we add a check for trash status, what are the bugs that still occur against generalize-object-change-logic.1.diff applied?

[filosofo]

Try generalize-object-change-logic.2.diff

[koopersmith]

13822.merged.diff merges filosofo's approach with the UX changes in nacin/my earlier patch. The following describes the UX changes and the reasoning behind their implementation.

Menu item titles now reflect their object's status.

Instead of reading "My Post (Draft)" for all non-published objects, titles now include their object's status, such as "My Post (Private)".

This improves UX by informing the user why their post will not appear in the menu (and as a result, what they can change to have the menu item appear). This is especially useful for scheduled posts, as it reflects the fact that the menu will dynamically update.

Trash behavior changes: On trash, make the menu item 'draft' instead of 'pending'.

The existing behavior is as follows:

• Add post A to the menu. Save. Trash post A.
• The menu item disappears from the frontend menu (as it should).
• If post A is untrashed and the menu has not been edited, the menu item will be automatically restored to its original position.

However, we run into trouble when we edit the menu:

• Post A appears as "Post A (Pending)". You can edit this menu item like you would any other menu item. There's even a yellow box that reads "Click Save Menu to make pending menu items public."
• When you save the menu, you are simultaneously presented with a warning that "Post A" referred to a trashed item and could not be saved, and a notice that the menu has been updated. The trashed item has been removed.

There is no indication that the trashed item will be removed and no way for the user to recover the menu item. The fact that the item is labelled as "pending" only adds to the confusion.

A menu item linking to a trashed post is no different than a menu item linking to a draft post or a pending post. If a menu item is untrashed it should reappear in the frontend menu (much like a menu item linking to a private post would if made public).

In the backend, the item will now appear as "Post A (Trash)". If a user wants to delete the item, they will. If the user wants to rearrange the menu and untrash the post, they can do that too. We should not be making the decision to delete the menu item for them; we should be providing the information necessary for them to make a competent decision.

(The implementation involved removing the trash/untrash listeners as well as the clause that deleted any menu items with trashed objects when the menu was saved.)

Improve "Original: Link to post" box.

Finally, the "Original: Link to post" has been updated to reflect any non-published post status (such as "Original: Link to post (Draft)"). Linking to a trashed post would lead to a 404—instead, we link to the appropriate trash page.

[filosofo]

Replying to koopersmith:

13822.merged.diff

+1

I think you nailed it.

[koopersmith]

Divided the patch along the three features described in my ​previous comment.

[nacin]

I'm quite happy to see this plan of attack worked out. Nice job.

Looks like we have one removed string, two changed strings, and two new strings. All are small changes it appears.

Appears good to commit. I want a sign-off from Jane on the "({post_status})" and "(Unsaved)" labels. We all appear to be in agreement that this is the best course of action.

[nacin]

(In [15249]) Treat trash/untrash of posts associated with media items the same as other stati changes. props koopersmith, see #13822.

[nacin]

(In [15250]) Add admin UI labels for menu items of unpublished posts. props koopersmith, fixes #13822. #stringchange

[jane]

I thought we were done for 3.0. We're in RC3 with a planned launch for today. Why exactly are we suggesting changing something untested on the day of release that *no one considered a blocker as of last Thursday in the official dev chat when the dates were decided*?

[markjaquith]

I told Nacin to put the new patch in because there was already a fix in ( [15219] ) that wasn't quite cutting it. We had to move forward (fix it with a new patch) or backwards (yank the patch out and live with it as a known issue). They were eager to fix, so I wanted to give them a shot. I expected that it likely would have to be reverted. I wanted to see if they could come up with a relatively simple fix that we could ship for 3.0. What they came up with is good, but not simple. It doesn't seem like a simple fix is even possible. I'm chatting with Nacin now to talk about the "known issue" implications of backing away to make sure there are no gotchas.

[filosofo]

r15249 fixes the non-UX issues mentioned in this ticket, with no string changes, so it should stay at least.

[nacin]

Reverts everything, removes trash handlers, revisit in 3.0.1.

[nacin]

​http://core.trac.wordpress.org/attachment/ticket/13822/13822.reverts.diff reverts [15219] and [15250]. Keeps part of [15249], as in the trash handlers are now gone.

[koopersmith]

If we can keep r15249 (logic/trash), the bug should be fixed.
Reverting r15250 will remove the descriptive labels and cause the "Original:" link to objects in trash to 404.

If we revert to auto-add and nix the trash handlers (i.e. 13822.reverts.diff), users will just see some weird behavior with trash and won't know when a menu-item refers to a non-published item. And the frontend should just do its thing.

[markjaquith]

Yep. I think we can live with that for 3.0, and revisit your comprehensive solution for 3.0.1. It's the most conservative option we have available.

[filosofo]

Why not keep all of r15249?

[nacin]

Replying to filosofo:

Why not keep all of r15249?

Because we're also removing r15219. The last part of r15249 makes a change to code introduced in r15219. The parts that are "kept" in r15249 are the giant blocks of red removing the trash/untrash handlers.

[filosofo]

Replying to nacin:

Because we're also removing r15219.

Yes, but why? That was the question.

[nacin]

Too much too close to final release. We either need all of it in, or all of it out, so we're removing the whole bit and going to revisit in 3.0.1.

[nacin]

(In [15254]) Revert [15219], [15250], some of [15249] for 3.0, revisit in 3.0.1. see #13822.

[koopersmith]

Replying to markjaquith:

Yep. I think we can live with that for 3.0, and revisit your comprehensive solution for 3.0.1. It's the most conservative option we have available.

I agree. After testing the patch, it seems like the most lightweight option we have. Removing the trash handlers reduces numerous odd behaviors into one bug—if you give a menu an object, it will show it regardless of the object's status. This is much easier to understand from a user's perspective and is likely the best possible interim solution.

[nacin]

Closed #14054 as a duplicate.

[filosofo]

I think my patch on ​13958 is another way of handling this same issue.

[nacin]

This fixed with #13958?

[marcus1060]

This is still an issue in 3.4.1.
Really think that non-published articles should not show up even if in a menu.

[wonderboymusic]

Doing some housekeeping here. Some of the code from 13822.merged.diff​ already made it into core. The pieces I could translate, I added in 13822.3.diff​. There is a piece in wp-includes/nav-menu.php from 13822.merged.diff​ that belongs to a function that no longer exists. If anyone wants to do the archaeology there, please do.

[helen]

#23835 was marked as a duplicate.

[wonderboymusic]

this bowl of spaghetti still applies with some fuzz, but I have no idea what I did when I made the patch

[wonderboymusic]

Needs a look by someone who knows the origin story here

[nacin]

Back to the Future.

[SergeyBiryukov]

#33987 was marked as a duplicate.

[dd32]

#37164 was marked as a duplicate.