Menu optimizations

[gchtr]

Ticket: #1959

Issue

See #1959.

Solution

This pull request:

• Adds a theme_location property to the Timber\Menu class that is set when the menu is registered with a theme location.
• Adds a menu property to the Timber\MenuItem so it’s possible the parent menu object from a menu item. For this, the parent menu object is passed to the constructor of Timber\MenuItem.
• Adds an options property to the Timber\Menu class that holds an array of default options that can be adapted through Timber\Menu’s constructor. It also remove the Timber\Menu::set_options() method and moves its content back to the constructor. I thought there’s no need to have a separate function for this.
• Passes down the new options property as an object for the $args parameter in the nav_menu_css_class filter.
• Updates the default for depth from -1 to 0 to match WordPress’s defaults in wp_nav_menu().

Impact

Better parity with WordPress menu filters and defaults.

Usage Changes

It’s possible to access the theme location in various places, like in the nav_menu_css_class filter:

function my_secondary_menu_classes( $classes, $item, $args ) {
    if ( 'secondary' === $item->menu->theme_location ) {
        $classes[] = 'theme-location-secondary';
    }

    return $classes;
}

add_filter( 'nav_menu_css_class', 'my_secondary_menu_classes', 10, 3 ); 

And the $args parameter in that filter contains the options passed to Timber\Menu.

Considerations

Should we add the options for the args parameter in the nav_menu_css_class filter or is it obsolete, because the same options could be accessed through $item->menu->options?

I didn’t add theme_location to Timber\Menu::$options, because when theme_location is used as an argument for wp_nav_menu(), it’s a query to select only the menu from that location. But in Timber, you could pass this in the first argument of Timber\Menu::__construct(). So you would never use it as an option.

In Timber\Menu::init(), I added $locations = get_nav_menu_locations(); to get theme the locations in order to set the theme_location property. We already use get_nav_menu_locations() in the Timber\Menu::__construct(), but it didn’t feel right to pass it down as a parameter in Timber\Menu::init(). The locations should be cached by WordPress’s internal option handling, so I hope we’re fine here.

Testing

Yes.

[codecov]

Codecov Report

Merging #2018 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master   #2018      +/-   ##
===========================================
+ Coverage     94.99%     95%   +0.01%     
  Complexity     1567    1567              
===========================================
  Files            49      49              
  Lines          3694    3702       +8     
===========================================
+ Hits           3509    3517       +8     
  Misses          185     185

Impacted Files	Coverage Δ	Complexity Δ
lib/MenuItem.php	87.91% <100%> (+0.7%)	45 <2> (ø)	⬇️
lib/Menu.php	92% <100%> (+0.19%)	51 <0> (ø)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e0418c...fff2953. Read the comment docs.

[codecov]

Codecov Report

Merging #2018 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master   #2018      +/-   ##
===========================================
+ Coverage     94.99%     95%   +0.01%     
  Complexity     1567    1567              
===========================================
  Files            49      49              
  Lines          3694    3702       +8     
===========================================
+ Hits           3509    3517       +8     
  Misses          185     185

Impacted Files	Coverage Δ	Complexity Δ
lib/MenuItem.php	87.91% <100%> (+0.7%)	45 <2> (ø)	⬇️
lib/Menu.php	92% <100%> (+0.19%)	51 <0> (ø)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e0418c...fff2953. Read the comment docs.

[codecov]

Codecov Report

Merging #2018 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master   #2018      +/-   ##
===========================================
+ Coverage     94.99%     95%   +0.01%     
  Complexity     1567    1567              
===========================================
  Files            49      49              
  Lines          3694    3702       +8     
===========================================
+ Hits           3509    3517       +8     
  Misses          185     185

Impacted Files	Coverage Δ	Complexity Δ
lib/MenuItem.php	87.91% <100%> (+0.7%)	45 <2> (ø)	⬇️
lib/Menu.php	92% <100%> (+0.19%)	51 <0> (ø)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa64d72...10bede7. Read the comment docs.

[jarednova]

Should we add the options for the args parameter in the nav_menu_css_class filter or is it obsolete, because the same options could be accessed through $item->menu->options?
If I understand this correctly, the args should be sent as well (as the code you have currently has it).

Even if it's duplicative, I think we want to play nicely with how the filter normally works and where a dev is expecting to find that info

I didn’t add theme_location to Timber\Menu::$options, because when theme_location is used as an argument for wp_nav_menu(), it’s a query to select only the menu from that location. But in Timber, you could pass this in the first argument of Timber\Menu::__construct(). So you would never use it as an option.

👍

In Timber\Menu::init(), I added $locations = get_nav_menu_locations(); to get theme the locations in order to set the theme_location property. We already use get_nav_menu_locations() in the Timber\Menu::__construct(), but it didn’t feel right to pass it down as a parameter in Timber\Menu::init(). The locations should be cached by WordPress’s internal option handling, so I hope we’re fine here.

My experience is that those WP functions are always cached. I think relying on that vs. a crazy chain of argument passing is better

[jarednova]

found a little typo, and then responded to your considerations (tldr; I think the code as you have it is the right answers to those)