Opened 15 years ago
Closed 14 years ago
#13958 closed defect (bug) (fixed)
Deactivating custom post types/taxonomies breaks nav menus
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 3.2 | Priority: | normal |
Severity: | normal | Version: | 3.0 |
Component: | Menus | Keywords: | has-patch |
Focuses: | Cc: |
Description (last modified by )
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:
- Add a plugin that does custom post types/taxonomies.
- create an entry.
- add taxonomy to menu.
- 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)
Change History (50)
#3
follow-up:
↓ 9
@
15 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.
#6
follow-up:
↓ 7
@
15 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
@
15 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
@
15 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:
↓ 10
@
15 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
@
15 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.
#12
@
14 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.
@
14 years ago
mark menu items with invalid custom post type or taxonomy as "draft", but don't delete them
#15
@
14 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 callingwp_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.
#17
@
14 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
@
14 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
@
14 years ago
PS: I also agree with westi that we shouldn't automatically remove invalid menu items.
#20
@
14 years ago
- Keywords needs-ui added; ui-feedback removed
Something like this: invalid.png
Obviously needs new graphics.
#21
@
14 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.
#23
@
14 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.
#27
@
14 years ago
How does the most recent patch handle the front-end? Are _invalids and their children skipped?
#28
@
14 years ago
13958.2.patch:
- Patch from scribu refreshed
- _invalids are now skipped
- Patch also sets post_status to draft
#29
@
14 years ago
- Owner filosofo deleted
- Status changed from accepted to assigned
This needs an owner.
#30
@
14 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
@
14 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.
#33
@
14 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.
#35
@
14 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
@
14 years ago
- Owner set to ocean90
- Status changed from assigned to reviewing
Waiting for feedback from ocean90.
#37
@
14 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.
- 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.
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.