Fix SQL Issue with multiple orderBy('order')

[tjventurini]

Fixing SQL Issue: A column has been specified more than once in the order by list. Columns in the order by list must be unique.

Just had this issue on a MsSQL server. Whenever the menu items should be shown, I was facing the error from above. And I figured out, that there is indeed the orderBy('order') applied multiple times: Once in Menu::display() and the other time in the relationship itself, on MenuItem::children.

Since I think it makes somehow sense to sort the MenuItems per default, I decided to remove the orderBy in the Menu::display method. But this MR is just to share the awareness of the issue, so feel free to do something else to fix the issue 😉

Have a great day! 😁

[codecov]

Codecov Report

Merging #4985 into 1.4 will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##                1.4    #4985      +/-   ##
============================================
- Coverage     62.91%   62.90%   -0.01%     
  Complexity     1374     1374              
============================================
  Files           194      194              
  Lines          4010     4009       -1     
============================================
- Hits           2523     2522       -1     
  Misses         1487     1487              

Impacted Files	Coverage Δ	Complexity Δ
src/Models/MenuItem.php	84.31% <100.00%> (-0.31%)	20.00 <0.00> (ø)

---

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 80cc3aa...5ed67eb. Read the comment docs.

[MrCrayon]

Is ordering working as expected with this PR?

I see that order was introduced first in #1811 and then it was introduced in MenuItem::children with #4186.

P.S.
It looks like it should work because display is also calling children but I didn't test it.

[tjventurini]

Looking good for me 😃

The only difference is, that orderBy('order') is only applied once now.

[tjventurini]

Is this going to be merged or will it be fixed somewhere else?

[emptynick]

I think the order should be applied in display() not children().

[tjventurini]

Either way, It's probably gonna take some time until it's merged so feel free to update it meanwhile.

[emptynick]

Where is that second ordering tho?
Is it this one?
https://github.com/the-control-group/voyager/blob/8d60fe15d5b51393edf07117d347de16e0f0f4cd/src/Models/Menu.php#L74

Edit: Nevermind. In the model.

[tjventurini]

awesome, thx 👍