Skip to content

Conversation

@jarednova
Copy link
Member

Ticket: #2071

Issue

There's recursion in a Menu's MenuItem's Menus

Solution

Convert the public MenuItem::$menu property into a public method

Impact

This will break any PHP-based calls to MenuItem::$menu however, anything in Twig should still work (since it will pass the call of {{ menuitem.menu }} off to the method)

Usage Changes

In PHP, to get a MenuItem's menu, the dev should use:

$menu = $menu_item->menu();

Considerations

Would be great to make calls to the $menu property backwards compatible

Testing

New test to validate no recursion; might need additional test to cover new method

@jarednova
Copy link
Member Author

Travis tests are still failing (though they pass on my local); please hold off on any review/merge until I can resolve

@jarednova jarednova self-assigned this Oct 2, 2019
@jarednova jarednova changed the title Create failing test for #2071 Eliminate Recursion in Menu / MenuItem Oct 2, 2019
@jarednova
Copy link
Member Author

Looks like those failing tests are now resolved (good thing they failed, helped me to identify a bug that this created related to a MenuItem's depth!). Would love another set of 👀 on this ...

@jarednova jarednova added the Ready for Review Ready for a contrib to take a look at and review/merge label Oct 2, 2019
@codecov
Copy link

codecov bot commented Oct 2, 2019

Codecov Report

Merging #2083 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2083      +/-   ##
============================================
+ Coverage     95.05%   95.06%   +<.01%     
- Complexity     1579     1580       +1     
============================================
  Files            50       50              
  Lines          3744     3746       +2     
============================================
+ Hits           3559     3561       +2     
  Misses          185      185
Impacted Files Coverage Δ Complexity Δ
lib/MenuItem.php 88.54% <100%> (+0.24%) 47 <1> (+1) ⬆️

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 99e5e87...cbaa1b9. Read the comment docs.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 93.614% when pulling 90cac55 on 2071/menu_recursion into ca46f19 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 93.614% when pulling 90cac55 on 2071/menu_recursion into ca46f19 on master.

@coveralls
Copy link

coveralls commented Oct 4, 2019

Coverage Status

Coverage increased (+0.06%) to 93.888% when pulling 90cac55 on 2071/menu_recursion into ca46f19 on master.

Copy link
Member

@gchtr gchtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we keep the variable name $menu? A variable starting with an underscore, like $_menu, will not work with the WordPress Coding Standards that we will apply for 2.x.

Instead, could we add a deprecation call to a __get() method, that checks if the protected $menu property is accessed?

@jarednova
Copy link
Member Author

@gchtr thanks for your feedback, back to you! Just a few tweaks were needed to take care of those items

@jarednova jarednova assigned gchtr and unassigned jarednova Oct 12, 2019
@gchtr
Copy link
Member

gchtr commented Oct 16, 2019

@jarednova Thanks for the changes!

Instead, could we add a deprecation call to a __get() method, that checks if the protected $menu property is accessed?

I’m assuming you don’t want to catch direct calls to MenuItem::$menu then?

@jarednova
Copy link
Member Author

@gchtr actually, it turns out we get that already w/o any modification to __get() ....

I wrote this test to validate (which passes):

function testMenuItemMenuProperty() {
		$pg_1 = $this->factory->post->create( array( 'post_type' => 'page', 'post_title' => 'Foo Page', 'menu_order' => 10 ) );
		$pg_2 = $this->factory->post->create( array( 'post_type' => 'page', 'post_title' => 'Bar Page', 'menu_order' => 1 ) );
		$page_menu = new Timber\Menu();
		$items = $page_menu->get_items();
		$menu = $items[0]->menu;
		$this->assertEquals('Timber\Menu', get_class($menu));
	}

... the logic in __get() looks for methods with that name and already uses them instead of properties.

@gchtr
Copy link
Member

gchtr commented Oct 17, 2019

@jarednova Oh, I see! Very well then. Let’s merge this in!

@jarednova jarednova merged commit 6c4a2cb into master Oct 17, 2019
@jarednova jarednova deleted the 2071/menu_recursion branch October 17, 2019 14:03
acobster pushed a commit that referenced this pull request Aug 15, 2020
acobster pushed a commit that referenced this pull request Aug 17, 2020
acobster pushed a commit that referenced this pull request Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready for Review Ready for a contrib to take a look at and review/merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants