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.

CommentFileSizeAuthor
#12 fix_params_1.patch569 bytespwolanin
#8 menu_localized_sort_access_3.patch11.45 KBpwolanin
#4 menu_localized_sort_access_2.patch9.76 KBpwolanin
#1 menu_localized_sort_access_1.patch6.22 KBpwolanin

Comments

pwolanin’s picture

pwolanin commented
Assigned: Unassigned » pwolanin
Status: Active » Needs review
StatusFileSize
new menu_localized_sort_access_1.patch6.22 KB

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

pwolanin’s picture

pwolanin commented

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?

pwolanin’s picture

pwolanin commented

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.

pwolanin’s picture

pwolanin commented
StatusFileSize
new menu_localized_sort_access_2.patch9.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.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

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.

pwolanin’s picture

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.

killes@www.drop.org’s picture

killes@www.drop.org commented

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.

pwolanin’s picture

pwolanin commented
StatusFileSize
new menu_localized_sort_access_3.patch11.45 KB

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

chx’s picture

chx commented
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.

pwolanin’s picture

pwolanin commented

patch still applies with minor offset.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

pwolanin’s picture

pwolanin commented
Status: Fixed » Reviewed & tested by the community
StatusFileSize
new fix_params_1.patch569 bytes

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.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Thanks, committed!

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)