WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#27113 closed defect (bug) (fixed)

Explicitly assign menu term relationship in wp_update_nav_menu_item() instead of using "tax_input" argument

Reported by: danielbachhuber Owned by: nacin
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.0
Component: Menus Keywords: has-patch commit
Focuses: Cc:
PR Number:

Description

wp_update_nav_menu_item() depends on the tax_input to assign a menu item to a menu term. However, when wp_update_nav_menu_item() is called from a user-less context, this association isn't created and the menu item is orphaned.

For me, this user-less context is a WP-CLI command where I haven't explicitly set a user.

Related #19373

Attachments (3)

27113.1.diff (1003 bytes) - added by danielbachhuber 6 years ago.
Explicitly set the menu term when creating a menu item, rather than depending on tax_input
27113.2.diff (1.2 KB) - added by danielbachhuber 6 years ago.
Update 27113.1.diff to support setting menu id when the menu item is an orphan
27113.3.diff (2.5 KB) - added by danielbachhuber 6 years ago.
Patch with tests

Download all attachments as: .zip

Change History (11)

#1 @nacin
6 years ago

  • Keywords needs-patch added; dev-feedback removed

Yeah, I agree. tax_input is shaky stuff.

@danielbachhuber
6 years ago

Explicitly set the menu term when creating a menu item, rather than depending on tax_input

#2 @danielbachhuber
6 years ago

  • Keywords has-patch added; needs-patch removed

27113.1.diff does the trick. The change is covered by existing unit tests.

For the record, the code didn't and doesn't support updating the associated menu_id of a menu item.

#3 @nacin
6 years ago

You removed a comment there, "Only set the menu term if it isn't set to avoid unnecessary wp_get_object_terms()".

Basically, that is_object_in_term() call was added for the purposes of reducing queries (which nav menus use a ton of). is_object_in_term() uses the term cache, while wp_set_object_terms() does not, by way of wp_get_object_terms().

the code didn't and doesn't support updating the associated menu_id of a menu item.

I believe you, but it's not obvious why this is the case. Could you explain?

@danielbachhuber
6 years ago

Update 27113.1.diff to support setting menu id when the menu item is an orphan

#4 follow-up: @danielbachhuber
6 years ago

I believe you, but it's not obvious why this is the case. Could you explain?

Good thing you asked :) I misread the conditional. It supported updating before if the previous item was an orphan. I've updated the patch, and added a test case.

#5 @danielbachhuber
6 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 3.9

#6 in reply to: ↑ 4 @nacin
6 years ago

Replying to danielbachhuber:

I've updated the patch, and added a test case.

I don't see the test case here. Patch looks good, though.

@danielbachhuber
6 years ago

Patch with tests

#7 @danielbachhuber
6 years ago

I don't see the test case here. Patch looks good, though.

Dang, them was some good tests. I've written new ones.

#8 @nacin
6 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 27556:

Explicitly assign menu term relationship rather than piggybacking on wp_insert_post() with the tax_input argument.

That argument currently depends on user context (see #19373).

Adds unit test for properly updating orphaned menu items.

props danielbachhuber.
fixes #27113.

Note: See TracTickets for help on using tickets.