WordPress.org

Make WordPress Core

Opened 10 months ago

Closed 5 months ago

Last modified 5 months ago

#39800 closed defect (bug) (fixed)

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

Reported by: mrwweb Owned by: welcher
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.7.2
Component: Menus Keywords: has-patch has-unit-tests commit
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 (7)

39800.patch (737 bytes) - added by mrwweb 10 months ago.
first attempt at patch
39800.2.patch (672 bytes) - added by mrwweb 9 months ago.
updated patch, removes $active_parent_object_ids array from first patch
39800.3.patch (904 bytes) - added by ajoah 7 months ago.
New patch witch works for the two classes : current-menu-ancestor and current-menu-parent
39800.4.patch (892 bytes) - added by mrwweb 6 months ago.
add a final line break after changes to match formatting in rest of file
39800.5.diff (897 bytes) - added by mrwweb 6 months ago.
generated from repo root, sets $strict for in_array
39800.2.diff (3.7 KB) - added by welcher 5 months ago.
Adds Unit Test
39800.diff (3.7 KB) - added by welcher 5 months ago.
File cleanup and remove @group reference

Download all attachments as: .zip

Change History (27)

@mrwweb
10 months ago

first attempt at patch

#1 @mrwweb
10 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
9 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.


9 months ago

#3 @swissspidy
9 months ago

  • Keywords has-patch added

#4 @SergeyBiryukov
9 months ago

  • Milestone changed from Awaiting Review to 4.8

#5 @benedicteraae
7 months 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
7 months ago

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

#6 @mrwweb
6 months 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
6 months 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.


6 months ago

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


6 months ago

#9 follow-up: @welcher
6 months 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.


6 months ago

#11 @obenland
6 months ago

  • Milestone changed from 4.8 to Future Release

#12 @mrwweb
6 months 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
6 months ago

generated from repo root, sets $strict for in_array

#13 @mrwweb
6 months 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?

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


5 months ago

#15 @mrwweb
5 months ago

@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
5 months ago

Adds Unit Test

#16 @welcher
5 months ago

  • Keywords has-unit-tests commit added; needs-unit-tests removed
  • Milestone changed from Future Release to 4.9
  • Owner set to welcher
  • Status changed from new to accepted

@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
5 months ago

File cleanup and remove @group reference

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


5 months ago

#18 in reply to: ↑ 9 @SergeyBiryukov
5 months ago

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.

#19 @SergeyBiryukov
5 months ago

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

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.

#20 @mdgl
5 months ago

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.

Note: See TracTickets for help on using tickets.