search

unfulvio / wp-api-menus

:abcd: Menu routes for WordPress JSON REST API.
https://wordpress.org/plugins/wp-api-menus
GNU General Public License v2.0
140 stars 59 forks source link

Bug and Proposed Fix #22

Closed bponghneng closed 8 years ago

bponghneng commented 8 years ago

Hello.

Thanks for the the useful plugin. I started modifying it yesterday to add children to the output of the /menu-locations route, and I think I found a pretty big issue. After about the second sublevel, the code generates repeated nodes--basically walking back up the tree when it reaches a terminal child. I found this bug using "Testing Menu" from the WPtest data set (http://wptest.io/).

I came up with what I think is a good fix that simplifies the code. I'm a WordPress newbie, and my PHP is still stale from years of working only in JavaScript. So I could be wrong. But, here's what I found:

Looks like the output of wp_get_nav_menu_items() is sequenced correctly, so there's no need to do any sorting. Just reverse the array result, use a cache array to hold children until a parent comes along, and run a simple loop to build menu level arrays. Reverse generated arrays for completed menu levels, and send the response. Output looks correct in my tests.

Again, as I'm a newbie, please don't hesitate to send feedback so I can learn :)

Cheers!

unfulvio commented 8 years ago

sorry for the late reply, I just saw now this thread:

https://wordpress.org/support/topic/bug-and-proposed-fix

so there's two of you that could replicate the bug -- I'm sorry lately I don't have much time to follow this plugin. Your diff introduces quite a few changes. We need to make sure it doesn't break things for other users who did not report the issue.

bourpie commented 8 years ago

Thanks for this very usefull plugin.

I've also replicated the same bug. The fourth level node repeats itself on the third level.

image

bourpie commented 8 years ago

Tested the proposed fix. No problem so far. Thanks bpongvh.

unfulvio commented 8 years ago

thanks, I will push a release so folks will also get it via wp updates

bourpie commented 8 years ago

That's great! Thank you!