-
-
Notifications
You must be signed in to change notification settings - Fork 511
Eliminate Recursion in Menu / MenuItem #2083
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Travis tests are still failing (though they pass on my local); please hold off on any review/merge until I can resolve |
|
Looks like those failing tests are now resolved (good thing they failed, helped me to identify a bug that this created related to a |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
This commit consists of patches automatically generated for this project on https://scrutinizer-ci.com
Scrutinizer Auto-Fixes
1 similar comment
gchtr
left a comment
There was a problem hiding this 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?
|
@gchtr thanks for your feedback, back to you! Just a few tweaks were needed to take care of those items |
|
@jarednova Thanks for the changes!
I’m assuming you don’t want to catch direct calls to |
|
@gchtr actually, it turns out we get that already w/o any modification to 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 |
|
@jarednova Oh, I see! Very well then. Let’s merge this in! |
Ticket: #2071
Issue
There's recursion in a Menu's MenuItem's Menus
Solution
Convert the public
MenuItem::$menuproperty into a public methodImpact
This will break any PHP-based calls to
MenuItem::$menuhowever, 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:
Considerations
Would be great to make calls to the
$menuproperty backwards compatibleTesting
New test to validate no recursion; might need additional test to cover new method