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

[mrwweb]

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.

[mrwweb]

first attempt at patch

[mrwweb]

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]

updated patch, removes $active_parent_object_ids array from first patch

[benedicteraae]

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]

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

[mrwweb]

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]

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

[welcher]

@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.

[mrwweb]

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]

generated from repo root, sets $strict for in_array

[mrwweb]

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?

[mrwweb]

@welcher et al, given that this bug at least seems small in scope, I'm really hoping to get the fix into 4.9 (I know at least I have sites this impacts). It sounds like that can't happen until there are unit tests written. What does it take to make that happen?

[welcher]

Adds Unit Test

[welcher]

@mrwweb thanks for your patience on this one. I wrote up some unit tests for this and uploaded a new patch. The tests pass with your patch and fail without. The filenames are a bit off but the 39800.2.diff file is the one I added.

[welcher]

File cleanup and remove @group reference

[SergeyBiryukov]

Replying to welcher:

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 )

The same check is already present in two other places in the function, so I think the new one should be non-strict as well for consistency. We could probably make them all strict, but let's do that in a new ticket :)

Looks good to me otherwise.

[SergeyBiryukov]

In 41008:

Menus: Make sure current-menu-parent and current-menu-ancestor classes are properly set for parent items of post type archive submenu items.

Props mrwweb, ajoah, welcher.
Fixes #39800.

[mdgl]

This was a duplicate of #34839. As shown in the patch attached to that ticket, there is an opportunity to simplify/refactor the code for setting these attributes, eliminating much duplication.