Deactivating custom post types/taxonomies breaks nav menus

[WraithKenny]

Deactivating a plugin that registers custom post types and taxonomies with are added to the nav menu breaks the nav menu admin screen and the site. Not sure if anything else gets borked from this process.

Steps to reproduce:

1. Add a plugin that does custom post types/taxonomies.
2. create an entry.
3. add taxonomy to menu.
4. deactivate plugin.

Turning on the plugin appears to fix the issue.

One of the symptoms:

Notice: Trying to get property of non-object in /home/cristi/svn/wp/wp-includes/nav-menu.php on line 565

[nacin]

The question is, how should we handle these menu items? We don't have a hook for unregistration we can attach to which we could then use to remove the post type.

Likewise, when we go to set them up, if the corresponding post type doesn't exist and we remove the menu item, we'd be severing any menu item children that menu item has.

Perhaps it would be best to convert them to custom links? I'm not even sure we store the URL though; we just summon that from the post type.

[filosofo]

handle-invalid-object-types.13958.diff is my suggestion on how to handle this situation, both for missing post types and missing taxonomies:

• If the menu item is associated with an invalid post or taxonomy type, the menu item:
  • Gets an error message for its title
  • Gets an empty URL
  • Receives the post_status property of "draft."
• Front-end Walker displays only those menu items that have "publish" as their post_status property.
• Edit menu Walker does not care whether menu items have "publish" as their post_status property.
• Upon saving a menu item that is associated with an invalid post or taxonomy type, wp_update_nav_menu_item returns an error object, effectively deleting the menu item on save.
• Note that the added error message 'Invalid post type.' is not a new string.

[WraithKenny]

Preventing the errors that break the site and the nav admin page is great. Is there anyway to show a warning on deactivating a plugin that registers types/taxonomies suggesting that they clean up their custom menus or would that be overkill?

[nacin]

Replying to WraithKenny:

Preventing the errors that break the site and the nav admin page is great. Is there anyway to show a warning on deactivating a plugin that registers types/taxonomies suggesting that they clean up their custom menus or would that be overkill?

Overkill, I'd say -- they're currently stateless.

[jaredwilli]

In my learning about this bug, I wasn't using a plugin for my taxonomies. I had coded it in my functions file, and then after I had set up the menu and stuff, I realized I needed to change the details of the taxonomy. That was how it broke.

I also found that remaking the original taxonomy I had didn't fix it either.
May just be me in that sense, but I tried for hours to do everything I could besides hacking the core files.

[westi]

Replying to filosofo:

handle-invalid-object-types.13958.diff is my suggestion on how to handle this situation, both for missing post types and missing taxonomies:

• If the menu item is associated with an invalid post or taxonomy type, the menu item:
  • Gets an error message for its title
  • Gets an empty URL
  • Receives the post_status property of "draft."
• Front-end Walker displays only those menu items that have "publish" as their post_status property.
• Edit menu Walker does not care whether menu items have "publish" as their post_status property.
• Upon saving a menu item that is associated with an invalid post or taxonomy type, wp_update_nav_menu_item returns an error object, effectively deleting the menu item on save.
• Note that the added error message 'Invalid post type.' is not a new string.

I am cool will all of this apart from:

Replying to filosofo:

• Upon saving a menu item that is associated with an invalid post or taxonomy type, wp_update_nav_menu_item returns an error object, effectively deleting the menu item on save.

I would rather than they stayed as draft and were visible on the backend with some message.

I would like us to have a sane behaviour if someone disables a plugin/theme to test something and then goes back.

Don't want all the menu items that would be affected to disappear in that use-case.

[jaredwilli]

That's exactly what I was trying to do when I ran into this problem.

The way I was thinking of with this is. Check if a post type or taxonomy exists, and if the terms being used are associated with an active and existing type or tax. Then use them as normal. But if no post type/tax used by terms is active or exists default display message no existing item, and remove them from the options.

I'm not sure if this sort of thinking is as easily said since I'm not too familiar with Walker and understand there's much more to be considered probably too.

[nacin]

Austin, has your take on how you would implement this changed in the last four months or so?

I think westi's take makes sense. Also allows them to rearrange hierarchy. No real issue with them remaining drafts, though IIRC the menus system is designed to simply discard drafts on save.

Makes sense to implement this possibly in a similar fashion (UI/feedback-wise) to #13822.

[garyc40]

mark menu items with invalid custom post type or taxonomy as "draft", but don't delete them

[garyc40]

I just attached another patch that's a bit different from how filosofo's patch handles this issue:

• If post type or taxonomy is invalid, post_status is draft.

• Walker class is not touched. Instead, draft items are filtered again within wp_get_nav_menu_items(). Otherwise, draft items would still be returned when calling wp_get_nav_menu_items($menu, array('post_status' => 'publish'), which is unexpected behavior.

• The invalid menu items are not touched as well, even when the menu is saved. The 'draft' post status is dubbed on the fly.

As a result, when a plugin with custom tax or post type is disabled, then enabled again, its menu items won't go away.

[garyc40]

fixed typo

[garyc40]

how it looks after 13958.2.diff is applied

[scribu]

Display raw post type / taxonomy

[scribu]

"INVALID POST TYPE" is not very helpful. I was thinking of just displaying the internal post type name. See informative.13958.diff​

[scribu]

Should also mark the menu item as invalid somehow. Maybe give it a red-ish background?

[scribu]

PS: I also agree with westi that we shouldn't automatically remove invalid menu items.

[scribu]

Something like this: invalid.png​

Obviously needs new graphics.

[johnjamesjacoby]

Tested the patches, and scribu's seems the best to me since it's not destructive in any way.

Attaching a refreshed patch and screenshot.

[johnjamesjacoby]

Screen shot

[johnjamesjacoby]

Refreshed patch

[scribu]

CSS Gradients rule :)

[kcssm]

I see this bug is still not resolved in 3.2 beta. Does anyone has a time frame when this will be fixed.

[sterlo]

It would seem that these things are related: #16956 ?

[nacin]

How does the most recent patch handle the front-end? Are _invalids and their children skipped?

[ocean90]

13958.2.patch:

• Patch from scribu refreshed
• _invalids are now skipped
• Patch also sets post_status to draft

[nacin]

This needs an owner.

[nacin]

I didn't think I'd ever see a simple patch here, but that is a mighty fine patch. I think it's time.

What happens to valid children? Do they become orphaned?

isset() should become !empty() (when adding classes), and !empty() implies truthiness, so no need for $item->_invalid == true for the array_filter callback.

[ryan]

Looks like valid children of invalid parents become root nodes. No matter how far down the tree they are, if they are invalid they move to root and take their valid children with them. For example, with invalid1 > valid1 > invalid2 > valid2 > valid3, valid1 and valid2 are moved to root and valid3 retains valid2 as its parent. invalid1 > valid1 > valid2 > valid3 has valid 1 moving to root and the rest retaining proper hierarchy. Seems good enough.

[nacin]

That seems fine to me.

[nacin]

The purpose, it seems, of setting post_status to draft is to end up with a "Pending" label. I've adjusted this to an "Invalid" label.

Thus, do we still need post_status = draft? IIRC, that had a special meaning for menu items, for non-JS mode. I'd rather just remove those lines setting it to draft if so.

Committing this for now for a soak, leaving it open for final decision on post_status.

[nacin]

In [18260]:

Mark menu items tied to invalid taxonomies and post types as invalid. props ocean90, see #13958.

[nacin]

I think I see now. We only filter out invalids when: ! in_array( $args['post_status'], array( 'draft', 'any' ) ). But I imagine we always ask for 'any' in the admin, so is there a reason for a 'draft' check? Or it could just be if $post_status != 'publish'...

[nacin]

Waiting for feedback from ocean90.

[ocean90]

13958.3.patch

• We don't need to set post_status, as nacin mentioned (Pending) is only for non-JS mode, also it's not pending it's invalid.

Side effect was also if you set post_status to draft that you will get the "Click Save Menu to make pending menu items public." message. So just remove the both lines.

• comment:35: I noticed that we can't do this check, because we are using ​''publish'' in the admin too. So the menu item couldn't be deleted. Solution is, too use ! is_admin().

• I added to the patch also a small message if $some_invalid_menu_items}} exists, same as {{{$some_pending_menu_items. String needs a review.

[nacin]

Looks good. Ryan?

[ryan]

In [18277]:

Improvements to invalid menu item handling. Props ocean90. fixes #13958