WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 28 hours ago

#39800 new defect (bug)

current_item_{parent|ancestor} Not set for Post Type Archive Items

Reported by: mrwweb Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7.2
Component: Menus Keywords: has-patch needs-unit-tests
Focuses: Cc:

Description

To reproduce:

Add a Post Type Archive link as a submenu item.

Expected Result:

Parent $item object in menu should have $item->current_item_parent as true.

Current Result:

var_dump of the parent menu's $item in the Walker_Nav_Menu->start_el() shows current_item_parent as false.

Interestingly, $item->menu_item_parent of the current menu item (the post type archive) is set to the correct menu item object ID.


Note, in the past there have been quite a few reports of the Post Type Archive link and it's parents not receiving the correct CSS classes. This has been fixed, but I wonder if the fix was only a bandaid and missed the underlying issue. I'll keep investigating and report back if I can figure out a potential solution.

Attachments (5)

39800.patch (737 bytes) - added by mrwweb 4 months ago.
first attempt at patch
39800.2.patch (672 bytes) - added by mrwweb 4 months ago.
updated patch, removes $active_parent_object_ids array from first patch
39800.3.patch (904 bytes) - added by ajoah 5 weeks ago.
New patch witch works for the two classes : current-menu-ancestor and current-menu-parent
39800.4.patch (892 bytes) - added by mrwweb 3 weeks ago.
add a final line break after changes to match formatting in rest of file
39800.5.diff (897 bytes) - added by mrwweb 28 hours ago.
generated from repo root, sets $strict for in_array

Download all attachments as: .zip

Change History (18)

@mrwweb
4 months ago

first attempt at patch

#1 @mrwweb
4 months ago

I think I tracked this down. For some reason, the menu_item_parent wasn't added to the active_parent_item_ids array, so it wouldn't get noticed later on.

Realizing just now that active_parent_object_ids array doesn't make much sense in relation to post type archives, so maybe that shouldn't be in the patch.

This is my first patch so all constructive feedback is greatly appreciated.

@mrwweb
4 months ago

updated patch, removes $active_parent_object_ids array from first patch

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


4 months ago

#3 @swissspidy
4 months ago

  • Keywords has-patch added

#4 @SergeyBiryukov
4 months ago

  • Milestone changed from Awaiting Review to 4.8

#5 @benedicteraae
5 weeks ago

I am also very new to contributing here.

I copied the patch and tried testing it. It seems it fixes the current_item_parent issue but does not fix the current_item_ancestor issue.

@ajoah
5 weeks ago

New patch witch works for the two classes : current-menu-ancestor and current-menu-parent

#6 @mrwweb
3 weeks ago

Tested @ajoah's patch and it looks good! It fixes both the current_item_parent and current_item_ancestor properties of each menu item object. The menu classes also look good, though I believe they were already working.

Attaching a hopefully-final patch that adds a final line break before the comment for the next if clause to match formatting in rest of file.

@mrwweb
3 weeks ago

add a final line break after changes to match formatting in rest of file

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


11 days ago

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


4 days ago

#9 @welcher
4 days ago

  • Keywords needs-unit-tests added

@mrwweb thanks for pushing this along!

Can you regenerate the patch from the root of the repo instead of /src? It applies fine but if we end up adding unit tests - spoiler alert: we should ;) - we'll need a second patch.

The only other change I'd make is to change the in_array method to use the strict parameter to avoid unexpected results.

i.e in_array( $_anc_id, $active_ancestor_item_ids , true )

I might just be not getting it, but I was not able to get this setup locally to test it so I didn't make that change - can you share more detailed instructions?

Either way, don't let me hold this up, it looks like others have tested it so if you make those changes we are probably good and should get some unit tests on this. The current tests all passed when I ran them.

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


3 days ago

#11 @obenland
3 days ago

  • Milestone changed from 4.8 to Future Release

#12 @mrwweb
3 days ago

Was rushing to get a patch and then saw this just got punted :( Maybe if we hurry it can still get in?

@welcher I'm happy to redo this patch, but have a quick question I need an answer on first.

change the in_array method to use the strict parameter to avoid unexpected results.

This was copied from elsewhere in that file that also doesn't use strict mode. Should it get added in both places even though the second instance is for custom links, not post type archives?

can you share more detailed instructions?

I think this is maybe what you want? Here's a little testing plugin I was using. (Suffice it to say I don't know how to write unit tests!)

<?php
/**
 * MU Plugin for Testing Ticket #39800
 * https://core.trac.wordpress.org/ticket/39800
 *
 * Setup:
 * 
 * 1. Dump this code somewhere like an mu-plugin
 * 2. Create at least one Book post and one Page
 * 3. Create a menu with the following structures
 * 
 *     - Page
 * 			- Page
 * 		 	- Post Type Archive
 *     - Page
 *     		- Page
 *     			- Page
 *     			- Post Type Archive
 *
 * 4. View the menu/var_dump on the front end.
 * 
 * Expected Results:
 * 		- Without the patch: The current_item_parent and current_item_ancestor values for the parent/ancestor menu items should be incorrect when on the Post Type Archive page (compare to results on sibling Page's).
 * 		- With the patch: current_item_parent and current_item_ancestor are correct.
 * 		
 */
/* Register a custom post type with archive for testing */
function _39800_register_a_cpt() {
	register_post_type( 'books', array( 'label' => 'Books', 'public' => true, 'has_archive'=> true ) );
}
add_action('init','_39800_register_a_cpt');

/* var_dump current_item_parent and current_item_ancestor properties for each menu item to observe bug and patch results. */
function _39800_dump_my_vars( $items, $args ) {
	
	$dump = array();
	foreach( $items as $item ) {

		$dump[$item->title] = array('current_item_parent'=>$item->current_item_parent,'current_item_ancestor'=>$item->current_item_ancestor);

	}
	echo '<pre style="position:relative">'; var_dump($dump); echo '</pre>';

	return $items;

}
add_filter( 'wp_nav_menu_objects', '_39800_dump_my_vars', 10, 2 );

I've got a patch ready to go, just need an answer on whether to patch both instances of in_array or just the one relevant to this ticket?

@mrwweb
28 hours ago

generated from repo root, sets $strict for in_array

#13 @mrwweb
28 hours ago

New patch just updated. Generated from the root and setting $strict to true.

I realized there are a ton of in_array()s without $strict set to true in this file, so that seems like a different ticket.

Is this too late for 4.8?

Note: See TracTickets for help on using tickets.