WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 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 4 years ago.
13958.diff (5.3 KB) - added by garyc40 3 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 3 years ago.
fixed typo
Screen shot 2011-02-23 at 2.51.54 PM.png (43.5 KB) - added by garyc40 3 years ago.
how it looks after 13958.2.diff is applied
informative.13958.diff (1.4 KB) - added by scribu 3 years ago.
Display raw post type / taxonomy
informative.13958.2.diff (2.5 KB) - added by scribu 3 years ago.
invalid.png (65.1 KB) - added by scribu 3 years ago.
Screen shot 2011-05-11 at 9.13.55 PM.png (36.0 KB) - added by johnjamesjacoby 3 years ago.
Screen shot
13958.patch (2.5 KB) - added by johnjamesjacoby 3 years ago.
Refreshed patch
13958.2.patch (3.9 KB) - added by ocean90 3 years ago.
13958.3.patch (2.6 KB) - added by ocean90 3 years ago.

Download all attachments as: .zip

Change History (50)

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

Replying to 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.

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.

Version 0, edited 4 years ago by jaredwilli (next)

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

comment:17 scribu3 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

comment:18 scribu3 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?

comment:19 scribu3 years ago

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

scribu3 years ago

comment:20 scribu3 years ago

  • Keywords needs-ui added; ui-feedback removed

Something like this: invalid.png

Obviously needs new graphics.

scribu3 years ago

comment:21 johnjamesjacoby3 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.

johnjamesjacoby3 years ago

Refreshed patch

comment:22 scribu3 years ago

  • Keywords needs-ui removed

CSS Gradients rule :)

comment:23 kcssm3 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.

comment:24 toscho3 years ago

  • Cc info@… added

comment:25 jamescollins3 years ago

  • Version changed from 3.2 to 3.0

comment:26 sterlo3 years ago

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

comment:27 nacin3 years ago

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

ocean903 years ago

comment:28 ocean903 years ago

13958.2.patch:

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

comment:29 nacin3 years ago

  • Owner filosofo deleted
  • Status changed from accepted to assigned

This needs an owner.

comment:30 nacin3 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.

comment:31 ryan3 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 3 years ago by ryan (previous) (diff)

comment:32 nacin3 years ago

That seems fine to me.

comment:33 nacin3 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.

comment:34 nacin3 years ago

In [18260]:

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

comment:35 nacin3 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'...

comment:36 nacin3 years ago

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

Waiting for feedback from ocean90.

ocean903 years ago

comment:37 ocean903 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 3 years ago by ocean90 (previous) (diff)

comment:38 nacin3 years ago

Looks good. Ryan?

comment:39 ryan3 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.