Make WordPress Core

Opened 13 months ago

Closed 13 months ago

Last modified 13 months ago

#56946 closed defect (bug) (fixed)

WP 6.1-RC6: `menu-item-has-children` class is not being applied correctly

Reported by: webmandesign's profile webmandesign Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.1.1 Priority: normal
Severity: normal Version: 6.1
Component: Menus Keywords: has-patch has-unit-tests commit fixed-major
Focuses: Cc:

Description

When a theme has `wp_nav_menu()` depth parameter set higher than 0, WordPress 6.1-RC6 stops applying menu-item-has-children class for sub-menu items correctly.

This only happens when menu contains certain number of menu and sub-menu items - haven't found out the exact logic here though. But, for example, the issue is triggered when a theme has depth=4 and menu has this structure:

Level 1
- Level 2
- - Level 3
- - - Level 4
- - - - Level 5
- - - - - Level 6

The issue is not present if depth parameter is not set or is set to 0.

For testing check out my Michelle theme, for example, and use theme unit test with "All pages" menu set for "Primary" menu location.

This worked perfectly fine with previous versions of WordPress, so I think it is a WP6.1 bug.

For more info please visit https://wordpress.org/support/topic/wp-6-1-rc3-menu-item-has-children-class-is-not-being-applied-correctly/ thread where I've possibly found a bug in the code and also proposed a solution.

Attachments (3)

56946.patch (1.1 KB) - added by webmandesign 13 months ago.
before-patch.png (296.1 KB) - added by petitphp 13 months ago.
after-patch.png (329.4 KB) - added by petitphp 13 months ago.

Download all attachments as: .zip

Change History (37)

This ticket was mentioned in PR #3547 on WordPress/wordpress-develop by @webmandesign.


13 months ago
#1

  • Keywords has-patch added

Fixes incorrect menu-item-has-children class application by fixing foreach loop nesting.

Trac ticket: https://core.trac.wordpress.org/ticket/56946

#2 @SergeyBiryukov
13 months ago

  • Component changed from General to Menus
  • Milestone changed from Awaiting Review to 6.1.1

Hi there, thanks for the ticket!

Introduced in [54478] / #28620.

Moving to 6.1.1 for investigation.

#3 @SergeyBiryukov
13 months ago

  • Keywords needs-unit-tests added

#5 @sippis
13 months ago

Just run into the same problem, missing menu-item-has-children class, which caused menus to break on some of our production sites.

If it's helpful, one menu structure where this happened is not so deeply nested as the original example. Depth is set to 4.

- Level 1
  - Level 2
    - Level 3
    - Level 3
  - Level 2
    - Level 3
    - Level 3

#6 @petitphp
13 months ago

The patch works fine on my side and fix the issue (see before-patch and after-patch screenshot).

Tested with Twentytwenty theme and setting a custom depth of 4 for the primary menu.

#7 @ocean90
13 months ago

#56973 was marked as a duplicate.

#8 @hellofromTonya
13 months ago

#56986 was marked as a duplicate.

#9 @mukesh27
13 months ago

Thanks @webmandesign for the ticket and PR. Left one nit-pick feedback other then it PR LGTM. Can you please add unit tests?

#10 @webmandesign
13 months ago

@mukesh27 I'm sorry, I don't know how to add unit tests for that code. Can somebody else take over from here?

#11 follow-up: @ocean90
13 months ago

#56997 was marked as a duplicate.

#12 in reply to: ↑ 11 @fpodhorsky
13 months ago

Replying to ocean90:

#56997 was marked as a duplicate.

@ocean90 Thank you for redirecting me here. So I tested the patch and it works, but how do I apply this on my web? Should I wait until the new version of WordPress (6.1.1) comes out?

Last edited 13 months ago by fpodhorsky (previous) (diff)

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


13 months ago

This ticket was mentioned in PR #3568 on WordPress/wordpress-develop by @LeonidasMilossis.


13 months ago
#14

  • Keywords has-unit-tests added; needs-unit-tests removed

#15 @LeonidasMilossis
13 months ago

I've added my PR that enhances @webmandesign 's one with unit tests that verify the bugfix.

Considering it's my first contribution, apologies if I shouldn't have created a new PR.

#16 @davidvongries
13 months ago

We're making use of that class extensively with our theme and are currently receiving tons of reports. Will there be a hotfix for that? Or, do you guys any idea how we can fix this temporarily? It's potentially breaking 20k+ websites for us right now.

#17 follow-up: @webmandesign
13 months ago

@davidvongries I also receive support issues regarding this bug. I'm providing this quick temporary workaround for my theme users. It only prevents triggering the issue, not fixing it: https://gist.github.com/webmandesign/0f56c1a9a219ab3ce251a5844d33924b Check it out, maybe it will help you too. You can even put it into a plugin.

#18 @webmandesign
13 months ago

@davidvongries Also, it seems this will be fixed in WP6.1.1 which is due next week.

#19 follow-up: @nuvoPoint
13 months ago

We are still having issues with line 223:

( $args->depth <= 0
$menu_items_with_children[ $menu_item->ID ] < $args->depth )

This extra check was added in v.6.1 but is this necessary?

#20 in reply to: ↑ 19 @larsmqller
13 months ago

Replying to nuvoPoint:

We are still having issues with line 223:

( $args->depth <= 0
$menu_items_with_children[ $menu_item->ID ] < $args->depth )

This extra check was added in v.6.1 but is this necessary?

At least for my case when debugging $menu_items_with_children[ $menu_item->ID ] gives menu items id's to connect(expected with that name), and those would close to all of the time be bigger than $args->depth.

In my case wp_nav_menu is called with $args->depth = 0.

The extra check seems not to be necessary in my opinion also.
I did not take time to understand why this check could be needed based on the related code.

Just wanted to share.

#21 in reply to: ↑ 17 @davidvongries
13 months ago

@webmandesign, Thanks for sharing! Here's what I did: https://github.com/MapSteps/Page-Builder-Framework/commit/99d0c4785d65b888aaaf0f3e90d24f668c8d8aab

It's not great as it allows people to add more depth than they should but it's better than having a broken menu altogether, at least until the official fix is released.

#22 @petitphp
13 months ago

#57045 was marked as a duplicate.

#23 @petitphp
13 months ago

I redid my testing with the PR from @LeonidasMilossis and everything fine.

The tests break with the faulty code, as expected, and they are green with the fixed code.

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


13 months ago

#25 @mattkeys
13 months ago

We are experiencing issues due to this bug. I can confirm that after manually applying the patch in this thread that it has resolved the issue.

#26 @peterwilsoncc
13 months ago

  • Keywords commit added

Marking commit ready with the second linked pull request.

This includes the code changes from the first PR with the addition of some tests to ensure the class is added as expected.

#27 @peterwilsoncc
13 months ago

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

In 54801:

Menus: Apply menu-item-has-children class in sub-menus.

Ensure the menu-item-has-children class is added to sub-menu items when wp_nav_menu() is called with the depth parameter specified to a non-zero value.

Follow up to [54478].

Props davidvongries, fpodhorsky, hellofromTonya, innovext, larsmqller, LeonidasMilossis, mattkeys, mukesh27, nuvoPoint, ocean90, outrankjames, petitphp, SergeyBiryukov, sippis, webmandesign.
Fixes #56946.
See #28620.

#28 @peterwilsoncc
13 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for merging to the 6.1 branch.

#31 @desrosj
13 months ago

It looks like [54741] is not in the 6.1 branch and is causing conflicts when trying to backport [54801]. I'm going to backport both commits for consistency and a conflict free merge.

#32 @desrosj
13 months ago

In 54808:

Docs: Update comments in wp_nav_menu() tests per the documentation standards.

Includes:

  • Fixing a few typos.
  • Using the correct format for multi-line comments.
  • Removing some comments that duplicate the assertion messages without providing any additional context.

Follow-up to [54478].

Props SergeyBiryukov.
Merges [54741] to the 6.1 branch.
See #56792, #56946.

#33 @desrosj
13 months ago

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

In 54809:

Menus: Apply menu-item-has-children class in sub-menus.

Ensure the menu-item-has-children class is added to sub-menu items when wp_nav_menu() is called with the depth parameter specified to a non-zero value.

Follow up to [54478].

Props davidvongries, fpodhorsky, hellofromTonya, innovext, larsmqller, LeonidasMilossis, mattkeys, mukesh27, nuvoPoint, ocean90, outrankjames, petitphp, SergeyBiryukov, sippis, webmandesign, peterwilsoncc.
Merges [54801] to the 6.1 branch.
Fixes #56946.
See #28620.

This ticket was mentioned in Slack in #hosting-community by annezazu. View the logs.


13 months ago

Note: See TracTickets for help on using tickets.