Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years 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 2 years ago.
before-patch.png (296.1 KB) - added by petitphp 2 years ago.
after-patch.png (329.4 KB) - added by petitphp 2 years ago.

Download all attachments as: .zip

Change History (37)

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


2 years 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

@webmandesign
2 years ago

#2 @SergeyBiryukov
2 years 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
2 years ago

  • Keywords needs-unit-tests added

#5 @sippis
2 years 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

@petitphp
2 years ago

@petitphp
2 years ago

#6 @petitphp
2 years 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
2 years ago

#56973 was marked as a duplicate.

#8 @hellofromTonya
2 years ago

#56986 was marked as a duplicate.

#9 @mukesh27
2 years 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
2 years 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
2 years ago

#56997 was marked as a duplicate.

#12 in reply to: ↑ 11 @fpodhorsky
2 years 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 2 years ago by fpodhorsky (previous) (diff)

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


2 years ago

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


2 years ago
#14

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

#15 @LeonidasMilossis
2 years 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
2 years 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
2 years 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
2 years ago

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

#19 follow-up: @nuvoPoint
2 years 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
2 years 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
2 years 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
2 years ago

#57045 was marked as a duplicate.

#23 @petitphp
2 years 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.


2 years ago

#25 @mattkeys
2 years 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
2 years 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
2 years 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
2 years ago

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

Reopening for merging to the 6.1 branch.

#31 @desrosj
2 years 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
2 years 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
2 years 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.


2 years ago

Note: See TracTickets for help on using tickets.