Improve performance of access checks and add localized sorting [#154469]

A follow-on to: http://drupal.org/node/151583 for issues not resolved by that patch.

Function menu_check_access() (and it's helper function) and _menu_tree_data() need to be refactored to make two improvements:

1) use db_rewrite_sql() to check 'view' access to node links, rather than calling node_access() on each one.

2) sort the menu siblings based on the dynamic (localized) link title, rather than using the same sort order for all users.

Comment	File	Size	Author
#12	fix_params_1.patch	569 bytes	pwolanin
#12
#8	menu_localized_sort_access_3.patch	11.45 KB	pwolanin
#8
#4	menu_localized_sort_access_2.patch	9.76 KB	pwolanin
#4
#1	menu_localized_sort_access_1.patch	6.22 KB	pwolanin
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

pwolanin CreditAttribution: pwolanin commented 5 July 2007 at 21:08

Assigned:	Unassigned	» pwolanin
Status:	Active	» Needs review

File	Size
menu_localized_sort_access_1.patch	6.22 KB

First go at this problem - special case access checks for node links and sort on the localized titles.

Comment #2

pwolanin CreditAttribution: pwolanin commented 5 July 2007 at 21:31

I can already see one way in which this patch is sub-optimal. It traverses the tree twice on every page view. The first traversal to discover the node links only needs to be done once if there is a way to cache the result together with the tree in a way that it can be re-used later.

Can I store a multi-dimensional array key as a string like $index = "34]['below']['45']['link'"?

It seems unlikely that PHP is smart enough to have references (e.g. $node_links[25] = &tree[45]['link']) persist when I serialize and then unserialize the arrays?

Comment #3

pwolanin CreditAttribution: pwolanin commented 6 July 2007 at 01:45

to answer my own question:

You can even serialize() arrays that contain references to itself. Circular references inside the array/object you are serialize()ing will also be stored. Any other reference will be lost.

http://us.php.net/manual/en/function.serialize.php

So this *can* work is everything is part of the same array.

Comment #4

pwolanin CreditAttribution: pwolanin commented 6 July 2007 at 02:50

File	Size
menu_localized_sort_access_2.patch	9.76 KB

Ok, here's an implementation using the insight above. The extra tree traversal only happens when building the data for the cache. Tested with PHP 4.4.4.

Comment #5

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 6 July 2007 at 15:45

Peter asked me to look at this issue. While I don't understand the fine details of the new menu system, sorting by the localized title only works runtime, since the page language is decided on runtime and we can only localize in PHP with the given localization function stored for a menu item.

Custom menu item localization is not built into Drupal 6, so I am not sure we should think about localizing menu items when an item is a node title (and that node has translations). Contributed modules will need to take care of this, so the important part is to allow them to do it.

Comment #6

pwolanin CreditAttribution: pwolanin commented 6 July 2007 at 16:34

@Gábor - one of the main changes in this patch is to determine the sorting after the titles are localized (as applicable). In this sense, it is a good step forward. I would appreciate you input on what (if any) additional mechanism should be used to enable localizing of link titles. Note that links to nodes may or may not use the node title as the link text, so I'm not sure what the best approach to this would be.

Comment #7

killes@www.drop.org CreditAttribution: killes@www.drop.org commented 7 July 2007 at 19:15

You shouldn't assume that translating by the localized version of a string is best usability by default.

For example, I admin both drupal sites in English and German and the "access" link in the admin navigation menu goes from top to bottom after translation. it is now less annoying than in 4.7 since the sub-menu is shorter, but it still is annoying.

Comment #8

pwolanin CreditAttribution: pwolanin commented 7 July 2007 at 21:28

File	Size
menu_localized_sort_access_3.patch	11.45 KB

minor code and comment cleanup per discussion with Karoly on IRC.

Comment #9

chx CreditAttribution: chx commented 7 July 2007 at 22:14

Status:

Needs review

» Reviewed & tested by the community

killes: People expect alphabetical sorting. If you do not want to have that, assign weights. That's what they are good for.

We presume that the visible menu tree will always be small and also, the node link collection happens before caching. So the performance harm if any, should be very small.