Improve performance of access checks and add localized sorting
pwolanin - June 25, 2007 - 13:35
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | menu system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | pwolanin |
| Status: | closed |
Description
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.
#1
First go at this problem - special case access checks for node links and sort on the localized titles.
#2
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?#3
to answer my own question:
http://us.php.net/manual/en/function.serialize.php
So this *can* work is everything is part of the same array.
#4
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.
#5
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.
#6
@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.
#7
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.
#8
minor code and comment cleanup per discussion with Karoly on IRC.
#9
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.
#10
patch still applies with minor offset.
#11
Committed to CVS HEAD. Thanks.
#12
And of course, as soon as I start working on the book patch, I found a bug in this code that only manifests with deeper node trees.
A foolish mis-ordering of parameters.
#13
Thanks, committed!
#14