WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 years ago

#13958 closed defect (bug)

Deactivating custom post types/taxonomies breaks nav menus — at Version 16

Reported by: WraithKenny Owned by: filosofo
Milestone: 3.2 Priority: normal
Severity: normal Version: 3.0
Component: Menus Keywords: has-patch
Focuses: Cc:

Description (last modified by scribu)

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

Change History (21)

comment:1 WraithKenny4 years ago

  • Cc Ken@… added
  • Keywords post_type taxonomy added

comment:2 nacin4 years ago

  • Keywords 2nd-opinion added; post_type taxonomy removed

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.

comment:3 follow-up: filosofo4 years ago

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.

comment:4 filosofo4 years ago

  • Keywords has-patch added

comment:5 filosofo4 years ago

  • Owner set to filosofo
  • Status changed from new to accepted

comment:6 follow-up: WraithKenny4 years ago

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?

comment:7 in reply to: ↑ 6 nacin4 years ago

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.

comment:8 jaredwilli4 years ago

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.

comment:9 in reply to: ↑ 3 ; follow-up: westi4 years ago

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.

comment:10 in reply to: ↑ 9 jaredwilli4 years ago

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.

Last edited 3 years ago by scribu (previous) (diff)

comment:11 nacin4 years ago

  • Milestone changed from 3.0.1 to 3.0.2

comment:12 nacin3 years ago

  • Keywords needs-patch added; 2nd-opinion has-patch removed

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.

comment:13 nacin3 years ago

  • Milestone changed from 3.0.3 to 3.1

comment:14 nacin3 years ago

  • Keywords 3.2-early added
  • Milestone changed from 3.1 to Future Release

garyc403 years ago

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

comment:15 garyc403 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

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.

garyc403 years ago

fixed typo

garyc403 years ago

how it looks after 13958.2.diff is applied

comment:16 scribu3 years ago

  • Description modified (diff)

scribu3 years ago

Display raw post type / taxonomy

Note: See TracTickets for help on using tickets.