Enable Localization of Link Title Attribute and Fix Ugly Breadcrumbs
pwolanin - September 18, 2007 - 22:18
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | menu system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | pwolanin |
| Status: | closed |
Description
Go to node/x/edit - the breadcrumb is Home >> View which is not not nice - the node title should be there instead of View. We can fix this easily enough using a title callback.
The user, filter and taxonomy modules may have the same problem, so leaving this as CNW until I check them too.
Note - force a menu rebuild since this patch changes hook_menu.
| Attachment | Size |
|---|---|
| breadcrumb-titles-0.patch | 1.37 KB |
#1
oh dear - that's all wrong - if you make links to nodes this break in all sorts of bad ways (in part because we avoid loading the every node for performance reasons). So, maybe not readily fixable for nodes, except this patch changes the title to 'Content' from 'View' which looks a little better.
Currently we have a problem for user module - on the user edit page (for any user - not just the current) the BC is: Home >> My account. Attached patch fixes this, but the downside is that all links to user accounts (link 'user/2') consisting only of the user name. As a work-around you can link to 'ser/2/view'.
Also fixes the BC/titles for the filter pages.
#2
minor re-roll - webernet noticed a double period in one comment.
#3
re-roll to remove offset/fuzz
#4
If you don't like the "View" terminology (you suggested "Content" instead), I'd suggest either translating it yourself or using something like String Overrides. The other stuff in here is pretty good though...
#5
@Rob Loach - the issue is with what appears in the breadcrumb on the node edit screen. Does "View" really makes sense as the parent of "Edit"?
#6
What if we did the same thing you did for the users (title callback) for the nodes as well....
#7
@rob - I tried that approach earlier - it's bad for the reason that the title callback overrides whatever title you set if you make a link to the node in the a menu. This isn't as bad with users, but still maybe a reason to not do it.
#8
I see three things we're attempting to fix here:
#9
Also just found:
Think we should break these up into separate issues?
#10
the more I play with this the less happy I become about non-t() title callbacks being applied to link titles aside from BC and page title use.
thus, attached patch tries a somewhat new direction
#11
Found another:
#12
At first I got a bit concerned about security but then this is how title gets set in
drupal_get_title<?php$title = check_plain(menu_get_active_title());
?>
that's safe. As for the changes, they are fine.
#13
Some noted:
Menus:
- right the first line is a bad whitespace change
- why didn't we need the elseif($callback) check before?
- you removed(?) the special case for t() title callbacks which was supposed to be a performance improvement originally
Filter formats:
- this looks good
Nodes:
- we use verbs for tab titles where possible, so "Content" as a tab title does not look fitting into the concept
Users:
- that %user_current looked more and more suspicious through different patches... so why is it called the %user_current, if it matches any users?
#14
@Gabor: %user_current could be something else, but the reason it has that name is that it is one of the few items to use a _to_arg() function so that the link in the navigation menu points to the current user's account.
#15
@Gabor - also if we went with 'Content' as a title for the node page it does not show on the tab (that is still 'View') - 'Content' would only show in the breadcrumb. For example, on node/x/edit the typical BC would be Home >> Content rather than the current Home >> View
#16
Also - is it a feature or a bug that some page titles can be change by changing a link title in the Navigation menu?
#17
ok - I'm happier with this now - I think it makes sense. Now no user-specified link title will ever be overwritten by the title callback, when the link is rendered in the menu block. However, for the active trail, a non-t() title callback will still be applied.
#18
anyone taking a look at this? It's not critical, but I think it's a definite bug
#19
minor re-roll - webernet pointed out a stray space in front of function _menu_item_localize
#20
Nice work, everything seems to display properly. This attached patch also fixes the Node Previews that I mentioned earlier.
#21
Can we get a review and RTBC in here? /me runs away ...
#22
And perhaps someone like pwolanin can comment on related issue: http://drupal.org/node/170309
#23
Here are some steps that one can take to review this patch:
#24
Breadcrumbs before patch:
> Applied patch
> Rebuilt menus
Breadcrumbs after patch:
I set this to RTBC, as the patch apparently does exactly what is was supposed to do. Good job!
#25
OK, I looked over #20 and do not understand how/why the call to _menu_item_localize() gets removed, and replaced with some code which does less. It does not ensure that the menu item descriptions are localized for example. Can someone enlighten me? Also, nobody explained why didn't we need the elseif($callback) check before, although I asked this above.
#26
@Gabor - the elseif is extra insurance - I think there could be some items with empty callbacks.
_menu_item_localize() is removed at that point because it will overwrite a user-entered title for a link. Instead - only t() is called to actually localize the title if it matches the title of the router item. As far as I know, all other title callbacks are not actually localizing, but are rather generating the title based on the path or some such.
I don't think the description is ever localized - they are added to the options array. I suppose we could use the same sort of check as for the title - whether it matches the router item value.
#27
pwolanin: descriptions are written in English in code, and stored in English in the database. Look into what _menu_item_localize() does. It does localize the description too, so without using it, we basically loose menu item description localization at least.
#28
@Gabor - $item['description'] is only used, as far as I know, for a few admin pages. In this case, since we are displaying a link, the description is not used at all. It would be easy enough to add a line to localize it, just in case, but is there any reason to?
I think it is a separate bug that the title attribute will not get localized. This attribute is derived from the description when the link is saved to the DB
includes/menu.inc:1566:
'options' => empty($item['description']) ? array() : array('attributes' => array('title' => $item['description'])),#29
Hm, so it seems I unknowingly touched on another bug :) Anyway, all I was saying is that _menu_item_localize() does more then what you do here. One thing it does it it t()-es the item description. Whether the menu item description is used or not depends entirely on the theme, right? That Drupal core itself does not display it does not mean a theme would not.
#30
@Gabor - after investigating - apparently we weren't even loading the description onto the menu links.
Improved patch now does that and also localizes it and the title attribute (using the same test as for link title - localize it if it matches the router item)
#31
#32
Did some basic testing and all seems OK.
#33
Applied the patch and confirmed that it fixes the missing title/descriptions issue.
#34
#35
To me, this isn't 100% either. In case of editing a node, I'd expect the page is always the title of the node and not 'Edit $title'. We don't use 'Edit $username' either. I've made that change and committed the patch. Thanks all.
#36
The commit ( http://drupal.org/cvs?commit=92964 ) was incomplete -- only the node.module and node.pages.inc chunks were committed.
#37
I believe we should make a new patch reflecting the part(s) that were missing.
#38
Here's a re-rolled patch with the missing parts - It seems Dries intent was to have just the node title (absent the word "Edit"), so I fixed that line too. With the present HEAD there is no title at all on the edit page.
#39
Committed, thanks.
#40
Automatically closed -- issue fixed for two weeks with no activity.