WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#33187 closed defect (bug) (fixed)

Menu becomes empty after saving in WordPress 4.2.3

Reported by: jcexygy Owned by: boonebgorges
Milestone: 4.3 Priority: high
Severity: major Version: 4.2
Component: Taxonomy Keywords: has-patch commit
Focuses: administration Cc:

Description

After upgrading to WordPress 4.2.2 (and the same happens with 4.2.3), I tried to add an item to a menu (in Appearance > Menus), and upon clicking Save, the menu becomes blank.

This bug seems to be related to the new Taxonomy Term Splitting functionality. The last commenter here seems to have had the same issue:
https://make.wordpress.org/core/2015/02/16/taxonomy-term-splitting-in-4-2-a-developer-guide

When this happened, I checked the database to see what was happening. It appears a new term was created in the wp_terms table. The original term was 'Apps' and had a term_id of 6. There was also a category using the term 'Apps'. I also checked the wp_term_relationships table, and all of the rows with a term_taxonomy_id of 6 have been deleted.

I did a diff against the previous version of the database in order to find out what happened - here's a screenshot of part of that diff: https://www.dropbox.com/s/u76zynmz4jz5scx/Screenshot%202015-07-29%2014.43.16.png?dl=0

Also, in the wp_term_taxonomy table, the related row changed - you can see by this diff that the term_id changed to 199 (which is the term_id of the newly created 'Apps' term), and the count went from 66 to 0: https://www.dropbox.com/s/oejbedp5vgur240/Screenshot%202015-07-29%2014.45.56.png?dl=0

I've been able to replicate this several times on a dev site and live site. I deactivated all plugins and switched to the TwentyFourteen theme, and the issue still occurs. Note that once the "split" happens, I can add items back to the menu, and the problem doesn't recur. In order to replicate, I had to revert to a previous version of the database.

I realize this is an edge case (having a menu name that's the same as a category name - or other taxonomy term), and that once it happens, the problem doesn't recur, but it can be frustrating and disconcerting to lose an entire menu structure, especially when that menu contains a lot of items and has to be re-created.

Attachments (2)

33187.diff (6.2 KB) - added by boonebgorges 6 years ago.
33187.2.diff (10.1 KB) - added by dd32 6 years ago.

Download all attachments as: .zip

Change History (15)

#1 @ocean90
6 years ago

#33334 was marked as a duplicate.

#2 @ocean90
6 years ago

  • Milestone changed from Awaiting Review to 4.3
  • Owner set to boonebgorges
  • Status changed from new to reviewing
  • Version changed from 4.2.3 to 4.2

@boonebgorges Can you take a look at this?

Given we're bulk splitting terms in 4.3, it seems like this could affect some installs.

@boonebgorges
6 years ago

#3 @boonebgorges
6 years ago

  • Keywords has-patch needs-testing added
  • Severity changed from normal to major

jcexygy - Thanks very much for the report.

When we built the term-splitting logic in 4.2, we accounted for terms associated with specific nav items, but we totally missed the 'nav_menu' terms themselves.

Two fixes are required to make it work. See 33187.diff and associated tests:

  1. wp_update_nav_menu_object() returns the ID of the menu term, and that ID is used to perform other updates (like updating the associated nav items). Previously, the ID passed to the function was being blindly returned, regardless of what was returned by the internal call to wp_update_term(). 33187.diff fixes this, and modifies wp-admin/nav-menus.php so that it redirects to a correct URL (?menu=123) if the nav_menu ID changes.
  2. When a nav_menu term is split, the corresponding entries in 'nav_menu_locations' must be changed. See _wp_check_split_nav_menu_terms().

This ticket was mentioned in Slack in #core by boone. View the logs.


6 years ago

#5 @ocean90
6 years ago

  • Priority changed from normal to high

Tested 33187.diff and the patch works for me.

My test case:

  • Create a menu and category with the same name in 4.0
  • Update to 4.2
  • Apply patch
  • Update the menu
  • The term gets splitted and the redirect kicks in

This ticket was mentioned in Slack in #core by obenland. View the logs.


6 years ago

#7 @pento
6 years ago

  • Keywords commit added; needs-testing removed

+1 on committing 33187.diff.

This ticket was mentioned in Slack in #core by netweb. View the logs.


6 years ago

#9 @dd32
6 years ago

What about when $_nav_menu_selected_id is a WP_Error instance? Other than that, the patch looks good to me.

@dd32
6 years ago

#10 follow-up: @dd32
6 years ago

I think 33187.2.diff works around that issue, and includes calling exit() after wp_redirect() which was missing in the earlier patch.

One thing worth noting is that this all causes the $messages to be ignored, which are probably just Menu X updated, but could also include error messages or something I think.

#11 in reply to: ↑ 10 @boonebgorges
6 years ago

Replying to dd32:

I think 33187.2.diff works around that issue, and includes calling exit() after wp_redirect() which was missing in the earlier patch.

One thing worth noting is that this all causes the $messages to be ignored, which are probably just Menu X updated, but could also include error messages or something I think.

Thanks, dd32. Yes, $messages will be lost. And it's possible that $messages will contain error messages, though it looks like it'll only happen when data is corrupt (like, a nav item has been deleted manually from wp_posts during a race condition). Given that the redirect is going to be tripped in such a small number of installs (those with a shared menu term), and on those installs it'll happen just one time and never again, I'm OK with this limitation.

This ticket was mentioned in Slack in #core by boone. View the logs.


6 years ago

#13 @boonebgorges
6 years ago

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

In 33611:

When splitting a shared 'nav_menu' term, ensure that nav items and theme locations are retained.

Props boonebgorges, dd32.
Fixes #33187.

Note: See TracTickets for help on using tickets.