WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#13958 closed defect (bug) (fixed)

Deactivating custom post types/taxonomies breaks nav menus

Reported by: WraithKenny Owned by: ocean90
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

Attachments (11)

handle-invalid-object-types.13958.diff (4.7 KB) - added by filosofo 6 years ago.
13958.diff (5.3 KB) - added by garyc40 6 years ago.
mark menu items with invalid custom post type or taxonomy as "draft", but don't delete them
13958.2.diff (5.3 KB) - added by garyc40 6 years ago.
fixed typo
Screen shot 2011-02-23 at 2.51.54 PM.png (43.5 KB) - added by garyc40 6 years ago.
how it looks after 13958.2.diff is applied
informative.13958.diff (1.4 KB) - added by scribu 6 years ago.
Display raw post type / taxonomy
informative.13958.2.diff (2.5 KB) - added by scribu 6 years ago.
invalid.png (65.1 KB) - added by scribu 6 years ago.
Screen shot 2011-05-11 at 9.13.55 PM.png (36.0 KB) - added by johnjamesjacoby 6 years ago.
Screen shot
13958.patch (2.5 KB) - added by johnjamesjacoby 6 years ago.
Refreshed patch
13958.2.patch (3.9 KB) - added by ocean90 6 years ago.
13958.3.patch (2.6 KB) - added by ocean90 6 years ago.

Download all attachments as: .zip

Change History (50)

#1 @WraithKenny
6 years ago

  • Cc Ken@… added
  • Keywords post_type taxonomy added

#2 @nacin
6 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.

#3 follow-up: @filosofo
6 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.

#4 @filosofo
6 years ago

  • Keywords has-patch added

#5 @filosofo
6 years ago

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

#6 follow-up: @WraithKenny
6 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?

#7 in reply to: ↑ 6 @nacin
6 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.

#8 @jaredwilli
6 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.

#9 in reply to: ↑ 3 ; follow-up: @westi
6 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.

#10 in reply to: ↑ 9 @jaredwilli
6 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 6 years ago by scribu (previous) (diff)

#11 @nacin
6 years ago

  • Milestone changed from 3.0.1 to 3.0.2

#12 @nacin
6 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.

#13 @nacin
6 years ago

  • Milestone changed from 3.0.3 to 3.1

#14 @nacin
6 years ago

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

@garyc40
6 years ago

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

#15 @garyc40
6 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.

@garyc40
6 years ago

fixed typo

@garyc40
6 years ago

how it looks after 13958.2.diff is applied

#16 @scribu
6 years ago

  • Description modified (diff)

@scribu
6 years ago

Display raw post type / taxonomy

#17 @scribu
6 years ago

  • Keywords 3.2-early removed
  • Milestone changed from Future Release to 3.2

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

#18 @scribu
6 years ago

  • Keywords ui-feedback added; dev-feedback removed

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

#19 @scribu
6 years ago

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

#20 @scribu
6 years ago

  • Keywords needs-ui added; ui-feedback removed

Something like this: invalid.png

Obviously needs new graphics.

@scribu
6 years ago

#21 @johnjamesjacoby
6 years ago

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
6 years ago

Refreshed patch

#22 @scribu
6 years ago

  • Keywords needs-ui removed

CSS Gradients rule :)

#23 @kcssm
6 years ago

  • Version changed from 3.0 to 3.2

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

#24 @toscho
6 years ago

  • Cc info@… added

#25 @jamescollins
6 years ago

  • Version changed from 3.2 to 3.0

#26 @sterlo
6 years ago

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

#27 @nacin
6 years ago

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

@ocean90
6 years ago

#28 @ocean90
6 years ago

13958.2.patch:

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

#29 @nacin
6 years ago

  • Owner filosofo deleted
  • Status changed from accepted to assigned

This needs an owner.

#30 @nacin
6 years ago

  • Keywords commit added

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.

#31 @ryan
6 years ago

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.

Last edited 6 years ago by ryan (previous) (diff)

#32 @nacin
6 years ago

That seems fine to me.

#33 @nacin
6 years ago

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.

#34 @nacin
6 years ago

In [18260]:

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

#35 @nacin
6 years ago

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'...

#36 @nacin
6 years ago

  • Owner set to ocean90
  • Status changed from assigned to reviewing

Waiting for feedback from ocean90.

@ocean90
6 years ago

#37 @ocean90
6 years ago

  • Keywords commit removed

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.

  • 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.
Last edited 6 years ago by ocean90 (previous) (diff)

#38 @nacin
6 years ago

Looks good. Ryan?

#39 @ryan
6 years ago

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

In [18277]:

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

Note: See TracTickets for help on using tickets.