.menu-item-home class not applying to static front pages

[RachieVee]

The wp_nav_menu is only applying the .menu-item-home class to menu items when the home url is created with a custom link on Appearance > Menus. The Codex and Code Reference mention this class should be added to menu items "that correspond to the site front page."

​https://developer.wordpress.org/reference/functions/wp_nav_menu/

When there is a static page set as a front page from Settings > Reading Settings, and then added as a link in Appearance > Menus from the "Pages" metabox, the class is not applied to the menu item. So either the descriptions in documentation can be adjusted or the conditionals/if statements in wp-includes/nav-menu-template.php can be adjusted so this class applies to static page links without having to be custom links.

[RachieVee]

Screenshot of missing menu-item-home class from static page menu item

[christophherr]

Adds the class .menu-item-home to a page set as front page when it is added to a navigation menu through the "Pages" metabox in Apperance - Menus.

[christophherr]

Original patch didn´t apply. Added a refresh.

[welcher]

Looks good to me. Not sure what else we'd need to do on this.

[pento]

In 38882:

Menus: Add the menu-item-home class to the static front page.

When a site is using a static front page, and that page is in a menu, it isn't given the CSS class menu-item-home, contrary to the developer documentation.

Props christophherr.
Fixes #35272.

[mdgl]

Pardon me if I'm being stupid, but how is this patch supposed to work? The value being tested in the if statement looks just like a constant string to me. When I apply the patch, I get the class menu-item-home added on current taxonomy menu items and current but non-home pages as well as on the front page. I think a new patch and some unit tests are needed, see #32367.

[adamsilverstein]

Replying to mdgl:

Pardon me if I'm being stupid, but how is this patch supposed to work? The value being tested in the if statement looks just like a constant string to me.

@mdgl I think you are right, this conditional wasn’t doing anything. When I tested it actually seemed to work, I think the reason the patch mostly worked is that by the time logic reached this point only the home page was left?

In 35272.2.diff​ I added an actual check for the object id set as the 'front page'. When the ids match, the calss is added. Some unit tests here to verify behavior would be helpful.

[christophherr]

I did not see the class added to other current pages / taxonomies but that only proves your point about unit tests @mdgl.

@adamsilverstein's patch looks much better and more robust.
Following the format of the other checks, may I suggest to cast the get_option check to (int)?

$front_page_id  = (int) get_option( 'page_on_front' );

I'd need some pointers to get started on unit tests for menu classes / items.
(Seems rather daunting after reading 32367)

[adamsilverstein]

Replying to christophherr:

Following the format of the other checks, may I suggest to cast the get_option check to (int)?

Yes, I agree we should cast as int here to match the rest of code and for safety. I had originally cast as int and removed when I discovered strict equality worked without it.

I'd need some pointers to get started on unit tests for menu classes / items.
(Seems rather daunting after reading 32367)

I have never tried writing these, might be worth asking @johnbillion how far he got on building out tests on #32367 or looking at existing core unit tests for menus.

[adamsilverstein]

35272.3.diff​:

• Cast page_on_front option as int

[pento]

Ugh, that was my bad, I don't know how I committed that without noticing it.

Until #32367 happens, unit tests can probably live in post/nav-menu.php.

[mdgl]

The new patch looks better to me and from my limited testing it appears to work correctly. Are we sure that there are no additional conditions required to ensure that the comparison with front_page_id remains valid in all circumstances? I would guess so but can't be completely confident.

In general, I still find the overall logic in function _wp_menu_item_classes_by_context() very confusing and wouldn't be surprised if there were a few more bugs lurking in this area :-)

[welcher]

First, sorry for adding the commit keyword on this. I tested it, but clearly didn't review the code.

To (hopefully) redeem myself, I have added some unit tests and a small fix to @adamsilverstein's patch that came out of the tests.

[welcher]

Adds unit tests and small fix

[adamsilverstein]

Nice work, thanks @welcher!

[christophherr]

Thank you everybody for cleaning up my mess.
I apologize for trying a shortcut that seemed to work.

The unit test also passes with my patch which seems rather strange.
Would that be an indication that there is more to discover with greater unit test coverage for the nav menus in general?

[mdgl]

Yes! I suspect the unit tests should also include at least one negative case: a menu entry referring to a page that is not marked "on front" should not contain the menu-item-home class.

I think this is why John Blackbourn got a bit fed up with #32367 as to create good coverage here the number of test cases required is actually quite large when you include all possible classes, object types and hierarchic combinations. He did say, however that he was "nearly there" over a year ago!

[welcher]

adding a no class applied unit test.

[pento]

In 38940:

Menus: Add the menu-item-home class to the static front page item.

When a site is using a static front page, and that page is in a menu, it isn't given the CSS class menu-item-home, contrary to the developer documentation.

An incorrect solution was originally added in [35272], and is now gone. Let us never speak of it again.

Props mdgl, adamsilverstein, welcher, pento.
Fixes #35272.

[pento]

In 38945:

Tests: Fix some query typos introduced in [38940].

Props piewp for the catch.
See #35272.

[mdgl]

I think this is fine now and I don't want to reopen the ticket, but for completeness (and future work on #32367) I just wanted to raise a question about the unit tests, which I am not sure are strong enough to distinguish between the original (incorrect) patch and the final version.

I'm afraid I don't know how to run the unit tests, so this is just based on inspection.

The classes added by function _wp_menu_item_classes_by_context() are dependent on the value of the current query (i.e. the page being displayed), but this does not appear to be catered for by the tests.

For example, if I create a menu with two page items: "Front" and "Back" and set my home page to "Front", I see the following behaviour with the original patch:

	Front Menu Item	Back Menu Item
Front Page Displayed	menu-item-home	-
Back Page Displayed	-	menu-item-home

That is, the class menu-item-home is always and incorrectly added to the menu item corresponding to the currently displayed page, even if it is not actually the home page. With the final version of the patch, I see the correct behaviour as follows:

	Front Menu Item	Back Menu Item
Front Page Displayed	menu-item-home	-
Back Page Displayed	menu-item-home	-

Could somebody tell me whether the unit tests are able to pick up the difference between these two solutions? To my eyes, there doesn't appear to be quite enough code to do that.

[welcher]

Replying to mdgl:

I'm afraid I don't know how to run the unit tests, so this is just based on inspection.

Have a look at ​https://make.wordpress.org/core/handbook/testing/automated-testing/phpunit/ or ​http://ryanwelcher.com/2015/07/setting-up-unit-testing-for-wordpress-core/ to help you get setup.

Could somebody tell me whether the unit tests are able to pick up the difference between these two solutions? To my eyes, there doesn't appear to be quite enough code to do that.

The specific test of two menu items and one is set to the front page is not in place. The current tests do not check for that. However, there is a test that tests for a page that is not assigned to the home page and another that tests for one that is. It would not be a large lift to add a third to cover off your scenario