Fixes #3035. Reversing menu before sorting to display menu items in the ... #212

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@sembrestels
Member

sembrestels commented May 5, 2012

...registered order in a last instance.

Sem
Fixes #3035. Reversing menu before sorting to display menu items in …
…the registered order in a last instance.
@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow May 6, 2012

Member

I'm not convinced this is going to work correctly for all cases. We might need some tests here to prove correctness.

Member

ewinslow commented May 6, 2012

I'm not convinced this is going to work correctly for all cases. We might need some tests here to prove correctness.

@sembrestels

This comment has been minimized.

Show comment
Hide comment
@sembrestels

sembrestels May 7, 2012

Member

I undersand your worry. It doesn't seem to go working at all, but if you review the code a little more:

  1. array_reverse() works with any kind of array, and $section is always an array.
  2. After the reverse we have a sort function. If items must be sorted, a previous reverse doesn't matter.
  3. The reverse only corrects what usort do. Usort only reverses the array if it find all elements equivalent, and we need a reverse before to undo this.

I didn't find any flaw in the code.

Member

sembrestels commented May 7, 2012

I undersand your worry. It doesn't seem to go working at all, but if you review the code a little more:

  1. array_reverse() works with any kind of array, and $section is always an array.
  2. After the reverse we have a sort function. If items must be sorted, a previous reverse doesn't matter.
  3. The reverse only corrects what usort do. Usort only reverses the array if it find all elements equivalent, and we need a reverse before to undo this.

I didn't find any flaw in the code.

@brettp brettp closed this May 10, 2012

@brettp

This comment has been minimized.

Show comment
Hide comment
@brettp

brettp May 10, 2012

Member

Let's hold off on this until we have some unit tests. I'm going to close this request because it's on master. @sembrestels Please open PRs against the branch the ticket is filed under (1.8, in this case). More discussion at the ticket: http://trac.elgg.org/ticket/3035

Member

brettp commented May 10, 2012

Let's hold off on this until we have some unit tests. I'm going to close this request because it's on master. @sembrestels Please open PRs against the branch the ticket is filed under (1.8, in this case). More discussion at the ticket: http://trac.elgg.org/ticket/3035

@sembrestels

This comment has been minimized.

Show comment
Hide comment
@sembrestels

sembrestels May 10, 2012

Member

@brettp Default version in trac is 1.7, and lots of tickets are under this version. Anyway, if I'm doing a 1.8.x ticket, I should send it to 1.8 branch, do you?

Member

sembrestels commented May 10, 2012

@brettp Default version in trac is 1.7, and lots of tickets are under this version. Anyway, if I'm doing a 1.8.x ticket, I should send it to 1.8 branch, do you?

@brettp

This comment has been minimized.

Show comment
Hide comment
@brettp

brettp May 11, 2012

Member

I meant make the pull request against the branch that matches the milestone of the ticket. If a ticket is on milestone 1.8.4 the PR goes against the 1.8 branch, if it's 1.7.15 it's against the 1.7 branch. Does that make sense?

Member

brettp commented May 11, 2012

I meant make the pull request against the branch that matches the milestone of the ticket. If a ticket is on milestone 1.8.4 the PR goes against the 1.8 branch, if it's 1.7.15 it's against the 1.7 branch. Does that make sense?

@sembrestels

This comment has been minimized.

Show comment
Hide comment
@sembrestels

sembrestels May 11, 2012

Member

Understood. Of course it make sense.

Member

sembrestels commented May 11, 2012

Understood. Of course it make sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment