CRM-21821 respect nav item weight

[michaelmcandrew]

Note: In rebasing, I inadvertently closed the previous PR #11744 which has some previous chat in it. I don't think I can reopen it.

Overview

You can now pass a weight attribute to items in hook_civicrm_navigationMenu and they will be taken into account when rendering the menu.

This gives extension authors more control over where their menu items appear.

Before

Items were added to the end of the menu

After

Items are placed appropriately based on their weight. Items with no weight are placed at the end as before (nothing should break)

Technical Details

buildNavigationTree ordered navigation items by weight when pulling them from the DB. However, it did not retrieve the weight.

It now also retrieves the weight which makes it trivial to slot new menu items in to the menu.

An alternative approach to ordering would be to expect hook implementers to 'splice' their new menu items into the array. I did look into this approach but:

self::fixNavigationMenu() seemed to override any changes in ordering that I made
I think that the weight approach is fairly sound. It allows you to say things like "ensure that this menu appears just after / before the Mailing menu / Admin menu" etc.

---

• CRM-21821: navigationMenu hook should take account of weight attributes

[michaelmcandrew]

@colemanw said: Trying to review this I do have one concern - it looks like it is reordering not just hooks' additions to the menu but also core menu items. Is this going to result in any inconsistencies with existing menu items?

I don't think so. It is using the same method to do the re-ordering that was used to do the original ordering. The process is as follows:

1. The original ordering is done with this ORDER BY in the query:

https://github.com/michaelmcandrew/civicrm-core/blob/9f51bd16e3c7ac4e6e80d04ef6a8f570394fe30b/CRM/Core/BAO/Navigation.php#L175

2. More items are then added to the end of the array via hooks.

3. We re-order the arrays using the weigh where present. Since we are using the same method / field to do the ordering, the original items should remain in the same order. New items will be slotted in to the right place.

[colemanw]

@michaelmcandrew does that mean there's no point to the sql ORDER BY clause anymore and it can be removed?

[michaelmcandrew]

it could be removed. I am guessing, given the size of the nav menu, that the effect on performance of including / removing it would be negligible, which is why I didn't remove it. It would make the code a bit simpler.

[michaelmcandrew]

do you think we should remove it?

[colemanw]

If it makes the code simpler, then +1.

[michaelmcandrew]

done

[michaelmcandrew]

hey @colemanw - just following up on this one since I implemented your requested change. would you like to do another review and merge if you approve?

[colemanw]

I tested it out and found the menu items are all in the same order even without SQL-based ordering, so I think we're good to go.

[michaelmcandrew]

ok great - thanks