Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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 |
#8 | menu_localized_sort_access_3.patch | 11.45 KB | pwolanin |
#4 | menu_localized_sort_access_2.patch | 9.76 KB | pwolanin |
#1 | menu_localized_sort_access_1.patch | 6.22 KB | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedFirst go at this problem - special case access checks for node links and sort on the localized titles.
Comment #2
pwolanin CreditAttribution: pwolanin commentedI 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 commentedto 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.
Comment #4
pwolanin CreditAttribution: pwolanin commentedOk, 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 HojtsyPeter 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@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 commentedYou 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 commentedminor code and comment cleanup per discussion with Karoly on IRC.
Comment #9
chx CreditAttribution: chx commentedkilles: 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.
Comment #10
pwolanin CreditAttribution: pwolanin commentedpatch still applies with minor offset.
Comment #11
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #12
pwolanin CreditAttribution: pwolanin commentedAnd 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.
Comment #13
Gábor HojtsyThanks, committed!
Comment #14
(not verified) CreditAttribution: commented