WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 2 years ago

#15264 closed defect (bug) (fixed)

Deleting a term shared across taxonomies deletes all associated nav menus.

Reported by: michelwppi Owned by: garyc40
Milestone: 3.7 Priority: normal
Severity: major Version: 3.0.1
Component: Taxonomy Keywords: has-patch 3.2-early commit
Focuses: Cc:

Description (last modified by scribu)

Using other taxonomies since WP 2.3 in plugin xili-dictionary , I just discover that now (since WP. 3.0) when deleting a term of his taxonomy, a menu item can disappear.

In which conditions ?

When the term (i.e. news) is shared by taxonomy category and the plugin taxonomy named dictionary ?

After deep tests (thanks to the night when time changes from summer to winter time), I can also reproduce it in current conditions : when a term is shared between taxonomies category and post_tag : If you delete the tag (i.e my test), if a category 'my test' exists and was active inside a navigation menu : the nav menu item disappear (unpleasant).

The cause of this unexpected erasing, was the action hook delete_term at end of wp_delete_term function and precisely the default filter _wp_delete_tax_menu_item.
The add_action in default-filter.php (line 233) don't pass the 3 parameters and the function (menu-nav.php line 700) don't verify if it is possible to delete the menu item (as well done for term in other taxonomies in wp_delete_term itself) by testing the params and the taxonomy of the menu item content.

Today workaround in plugins using new taxonomies : remove filter before deleting a term on concerned taxonomies and add it after.
Hope that explanations were explicit.

Best regards

Attachments (5)

garyc40-15264.patch (2.9 KB) - added by garyc40 5 years ago.
there's a patch for that
15264.diff (2.7 KB) - added by wonderboymusic 3 years ago.
15264.2.diff (2.4 KB) - added by SergeyBiryukov 3 years ago.
Same as 15264.diff, with a minor formatting fix
15264.3.diff (2.5 KB) - added by wonderboymusic 2 years ago.
15264.4.diff (3.7 KB) - added by wonderboymusic 2 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 @Utkarsh5 years ago

  • Cc admin@… added
  • Keywords needs-patch added; post_tag delete_term _wp_delete_tax_menu_item removed
  • Summary changed from if a term is shared: delete this term delete ever nav menu item ! to Deleting a term shared across taxonomies deletes all associated nav menus.

comment:2 @scribu5 years ago

  • Description modified (diff)

comment:3 @michelwppi5 years ago

After one week of tests and understanding complex data-model of menu-items in posts table, I suggest these light modifications :
1) in default-filters.php (around line 235) - params were omitted

//add_action( 'delete_term',                '_wp_delete_tax_menu_item'       );
add_action( 'delete_term',                '_wp_delete_tax_menu_item', 10, 3 );

2) in nav-menu.php : only menu item type taxonomy with the same content (currently category) can be deleted - so a post_tag deleted don't destroy a menu item based on category with same name of post_tag (or other taxonomies)

/**
 * Callback for handling a menu item when its original object is deleted.
 *
 * @since 3.0.0
 * @access private
 *
 * @param int $object_id The ID of the original object being trashed AND taxonomy passed form wp_delete_term
 *
 */
function _wp_delete_tax_menu_item( $object_id = 0, $tt_id, $taxonomy ) { //2 params added (see taxonomy wp_delete_term and default-filters)
	$object_id = (int) $object_id; 

	// $menu_item_ids = wp_get_associated_nav_menu_items( $object_id, 'taxonomy' );
	
	$menu_item_ids = array();

	$query = new WP_Query;
	$menu_items = $query->query(
		array(
			'meta_key' => '_menu_item_object_id',
			'meta_value' => $object_id,
			'post_status' => 'any',
			'post_type' => 'nav_menu_item',
			'showposts' => -1,
		)
	);
	foreach( (array) $menu_items as $menu_item ) {
		if ( isset( $menu_item->ID ) && is_nav_menu_item( $menu_item->ID ) ) {
			if ( get_post_meta( $menu_item->ID, '_menu_item_type', true ) != 'taxonomy' )
				continue;
			if ( get_post_meta( $menu_item->ID, '_menu_item_object', true ) != $taxonomy ) // only if corresponding #15264
				continue;
			$menu_item_ids[] = (int) $menu_item->ID;
		}
	}
	$menu_item_ids = array_unique( $menu_item_ids );
	
	foreach( (array) $menu_item_ids as $menu_item_id ) {
		wp_delete_post( $menu_item_id, true );
	}
}


Hope it is helpfull

comment:4 @nacin5 years ago

  • Cc filosofo added
  • Milestone changed from Awaiting Review to Future Release

comment:5 @garyc405 years ago

  • Owner set to garyc40
  • Status changed from new to assigned

@garyc405 years ago

there's a patch for that

comment:6 @garyc405 years ago

  • Keywords has-patch needs-testing 3.2-early added; needs-patch removed

Attached a patch that's more aligned with DRY, based on michelwppi's suggestion.

comment:7 @SergeyBiryukov5 years ago

It seems that $tt_id is never used in _wp_delete_tax_menu_item().

comment:8 @garyc405 years ago

It's not used. But we still have to pass it in order to get to $taxonomy.

Deleting $tt_id or reordering the arguments in action hook 'delete_term' is not a good idea because that would break backward compat.

comment:9 @SergeyBiryukov5 years ago

I didn't pay enough attention to the add_action() part. Got it, thanks.

@wonderboymusic3 years ago

comment:10 @wonderboymusic3 years ago

  • Keywords needs-testing removed
  • Milestone changed from Future Release to 3.6

This is still a bonafide problem, I refreshed the patch so it will apply cleanly - bugs like this make me cringe, especially since it was fixed for 3.2-early

@SergeyBiryukov3 years ago

Same as 15264.diff, with a minor formatting fix

comment:11 @ryan2 years ago

  • Milestone changed from 3.6 to Future Release

@wonderboymusic2 years ago

comment:12 @wonderboymusic2 years ago

  • Milestone changed from Future Release to 3.7

15264.3.diff removes the fuzzes

@wonderboymusic2 years ago

comment:13 @wonderboymusic2 years ago

  • Keywords commit added

.4.diff adds unit tests

comment:15 @wonderboymusic2 years ago

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

In 25163:

Pass the taxonomy around to relevant nav menu actions to avoid arbitrarily deleting all items with menu-item-type of taxonomy. Adds unit test for wp_get_associated_nav_menu_items().

Props garyc40, SergeyBiryukov.
Fixes #15264.

Note: See TracTickets for help on using tickets.