The function
function theme_forum_display($forums, $topics, $parents, $tid, $sortby, $forum_per_page) {
global $user;
// forum list, topics list, topic browser and 'add new topic' link
$vocabulary = taxonomy_get_vocabulary(variable_get('forum_nav_vocabulary', ''));
$title = $vocabulary->name;
...
uses the name of the forum vocabulary (by default "Forums") as the title of the forum page, and it does not wrap it in a call to t()
.
I know I could change the name of the forum vocabulary, but this does not help for a multi-language site, and it's odd that the name of the forum vocabulary should determine the forum page title. But even if it does, why should the page title be exempt from translation? The German translation de.po quite rightly translates "Forums" into "Foren" just about everywhere else, but the title of the forum page won't budge.
Related issues
#1776826: Cleanup of template_preprocess_forum_topic_list and template_preprocess_forum_list
Comment | File | Size | Author |
---|---|---|---|
#71 | 148145-interdiff-71.txt | 6.23 KB | andypost |
#71 | 148145-forum-page-71.patch | 9.24 KB | andypost |
#69 | 148145-forum-page-69.patch | 4.71 KB | andypost |
#68 | 148145-forum-page-68.patch | 5.62 KB | andypost |
#65 | 138145-forum-cleanup.patch | 4.37 KB | andypost |
Comments
Comment #1
cburschkaThe question is whether to change this in creating the forum vocabulary or displaying it. The former fails to take into account a site that has users with different locales; the latter doesn't take into account that the forum vocabulary may be manually renamed.
However, I'd tend toward the second, as you suggested; if the vocabulary is renamed manually to an unknown word, it just won't be localized anymore which is not a big problem.
Comment #2
Gábor HojtsyThe forum vocabulary name is a user defined string. It could be localized to *one* language when entering the text. Like the default content type names and descriptions are localized once on installation. After a user submitted setting is in the database, it should not be localized, as t() is not designed to translate user defined stuff.
Comment #3
salvisI understand that t() shouldn't be used to translate email messages and such stuff, but what's the harm done by passing the forum vocabulary name through t() to allow translating it for displaying it as a page title?
Using the same string for two unrelated tasks is unfortunate and unexpected; having a separate customizable string for the title would be cleaner, but even then the title appears in a context where users expect it to be localized.
It's not an issue on single-language non-English sites, but it is a problem on multi-language sites.
Comment #4
Gábor Hojtsyt() is used with static strings, and it is not designed by its nature to be used with user defined strings, such as vocabulary names, or taxonomy terms. It is not scalable to that direction, neither its import/export/cleanup procedures are implemented with user defined strings in mind.
Comment #5
salvisWhat is the worst thing that could happen if t() were used with the forum vocabulary name to display the forum page title?
The only problem that I can think of is that someone would create a .po file with a translation for a changed forum vocabulary name, which is not in core. But aren't the core .po files controlled by the drupal.pot file, which is firmly in your hands?
Comment #6
Liam McDermott CreditAttribution: Liam McDermott commentedThink this is still valid so bumping version.
Comment #7
LoveCharge CreditAttribution: LoveCharge commentedsubscribing
Comment #8
toemaz CreditAttribution: toemaz commentedWorkaround: in forums.tpl.php , simple set
drupal_set_title(t('Forums'));
Comment #9
andypostWhy we use vocabulary name as title for Forums?
Just let it be overriden only if we are in some forum (tid > 0) in this case menu title will be inherited
Not sure about breadcrumbs... suppose we should get translated menu item title
Comment #10
andypostSame patch for d6
Comment #11
andypostAdded comment
Comment #12
VladSavitsky CreditAttribution: VladSavitsky commentedWhy wait?
Me and my clients need this feature now!
All testes was passed. Can you please commit this to D6 too?
Thanks a lot for your work.
Comment #13
VladSavitsky CreditAttribution: VladSavitsky commentedWorks fine
Comment #14
andypostI think this major because drupal is multilingual and translation of vocabulary name should not have meaning on page title
Comment #15
Dries CreditAttribution: Dries commentedandy, can you explain in the code comment _why_ we set the title conditionally? That would be great.
Comment #16
andypostA bit reorganized code with comment. Suppose this approach is much cleaner.
Comment #17
andypostI think this a more saner way: page title and breadcrumbs start piece are following from menu title.
By this way we could rename menu to "Discussions" and this will change all: page title, breadcrumbs and RSS feed.
PS Previous patch was wrong because $title is also used in RSS feed title so no way.
Comment #18
andypostSlightly re-worked logic
Breadcrumbs and page title should NOT be set in preprocess function this makes this function none-reusable!
So I moved code for breadcrumbs and page title into menu callback handler.
template_preprocess_forums() the ONLY function where page title is set in preprocess function
Changed a parameters for theme_forums now whole $forum_term passed.
This allows theme-defined preprocess functions use a forum name and properties of forum or container.
Also "tid, forums, parents" are preserved for themes that already using them.
RSS feed still added within preprocess because if page have forum listing so page should contain feed link.
That change could not be backported to D6 because changing preprocess function so #17 is a best candidature for backporting.
Comment #19
Dries CreditAttribution: Dries commentedThis looks like a nice clean-up.
Something is wrong with this sentence. The word 'start' doesn't seem to belong here.
Comment #20
andypostSure, this was a typo, re-roll
Comment #21
andypostFor D6 we can't modify template_preprocess_forums() because a lot of themes could relay on it. So Just a simple patch backport #17
Comment #22
Gábor HojtsyLooks like this changes logic of how the title is set which might cause problems on existing sites. While you might see this as a bug, others got used to it and will consider changing it a bug. Also, you remove a check_plain() without reasoning.
Comment #23
andypostChanging this in beta would be bad
Comment #24
Gábor HojtsyI understand this is marked beta blocker because of the API change on the theme function. I believe this can be done without the API change however if you omit that part of the cleanup.
Suggestions / questions:
- the first item was renamed to 'forum' from 'forums' and it is in fact a 'forum_term' as used elsewhere; maybe use that name here too?
- how are breadcrumbs on forum nodes made consistent with this new approach?
Comment #25
Gábor HojtsyIndeed, looks like forum_node_view() has the same issue:
Here is the forum -> forum_term rename patch at least. I think we should devise a helper function for the breadcrumb so we can use it consistently.
Comment #26
andypostforum_term means only term related data but this object is more then term it has whole forum's data inside
So I think forum is a better name
This actually need some work...
... I'll try to unify breadcrumbs for nodes/forums
EDIT: Suppose we could commit theme's functions change and continue with breadcrumbs in follow-ups
Comment #27
tloudon CreditAttribution: tloudon commentedandypost, Gabor: does this summarize correctly?
INCITING ISSUE: "Forums" ($title) was not localized although other usages of the word "forums" on the same page were
SOLUTION:
1) $title is now retrieved from menu_get_active_trail() rather than from the taxonomy_get_variable()->name, sidestepping the usage of t()
2) breadcrumbs and title were set in the forum_theme function but have been moved to forums.page.inc to make the functionality reusable
3) RSS line was changed as a side-effect of moving the page title and bread crumbs functionality
TODO:
1) look at removing the API change forum_page() introduces
2) update forum_node_view() to also use menu_get_active_trail() for the breadcrumbs
Comment #28
andypost@t_l mostly right except 1st TODO
API change in
theme_forums()
- this change is required because it makes it reusable and fixes bug of setting page title in preprocess function which is not related to *_preprocess_pageAlso forum_node_view() should be fixed about breadcrumbs
It seems reasonable to have some _forum_breadcrumbs() function to set both breadcrumbs - for forum and for nodes.
Also there's confusion - should be set breadcrumbs ONLY for 'forum topic' node type because any node could have "Forums" vocabulary field
Comment #29
Gábor Hojtsy@t_l: see @andypost's comment. The theme API change makes this a beta blocker. We can get that in quickly and then work on the breadcrumb / page title issue. If this drags on for long, the API change has no chance to get in. The RSS title change looks good in fact, since it now includes the subtopic title. Maybe including the 'Forums' equivalent title there could also be useful.
Comment #30
tloudon CreditAttribution: tloudon commentedok, what do we need to do as far as testing the API change then? I grep'd theme_forums, theme('forums, and theme("forums with no results. so it looks to me like there is no effected client code.
wrt making the breadcrumbs a separate on-going issue; do you want me to reroll the patch w/ the breadcrumb section still in the preprocess function? that seems counter-productive to me. i think we should just use the patch from # 20.
so if you are both on board, i think we can call this issue RTBC. i verified that the translations work as advertised w/ Belarusian. i have some screen caps if anyone is interested :)
Comment #31
andypostNew patch with fixed forum_node_view() breadcrumbs, I just menu_get_item('forum') which already returned as translated.
Also I prefer 'forum' against 'forum_term' argument for theme_forums() as it holds more forum-related data other then term-related.
PS: @t_l I'm testing with Russian language
Comment #32
philbar CreditAttribution: philbar commentedSomeone with proper permissions, add this too:
http://drupal.org/community-initiatives/drupal-core
Comment #33
Gábor Hojtsy@andypost: ok, then its confusing to call it forum_term in the caller.
Comment #34
andypostDiscussed with Gáborat IRC, forum_term has more sense because it's result of forum_forum_load() which name is used also in forum_menu_local_tasks_alter()
forum_term could be a forum or forum container so abstract forum_term is better
Patch also fixes doc-block for template_preprocess_forums()
Comment #36
andypostMissed to rename forum => forum_term
Comment #37
andypostThis is a API change so better commit this before beta
EDIT: menu forum/% was added at #520736: SA-CORE-2009-007 forum module XSS
I think we should change this back to original implementation because users can edit/change/translate only one menu item - "/forum"
Comment #38
philbar CreditAttribution: philbar commentedBeta blockers = Critical
Comment #39
webchickWhy on earth would a bug that's been there since Drupal 6 be a critical, beta-blocking bug?
Comment #40
andypost@webchick because this patch changes API (theme function params)
Comment #41
webchickThen we should probably fix it a different way? I assume you want this back-ported to D6.
Comment #42
andypost@webchick this can be fixed in D6 as Gábor Hojtsy pointed in #22
Leaving this as is (not changing arguments for preprocess) we leave no chance for contrib to have access to forum title in any theme function.
This looks like simple bug but causes a lot of troubles with forum theming and useless queries... but nobody cares about it so #881916: Should forums be in core or not?
PS: suggested "title callback" is not a solution because we have no
$forum->title
in theme layerComment #43
webchickWhat's unclear to me is why the D6 and D7 patches aren't exactly the same, given where we are in the release cycle. Themes have already started porting, and if this doesn't fix a critical bug, I'm not supportive of futzing with template variables at this stage. Polish phase was last year. We need to get D7 out the door.
Comment #44
Gábor HojtsyAs I've explained above, the only part that makes this an API breaker is the change in how the theming function is invoked, which is not at all required to fix the bug as far as I see.
Comment #45
andypostPatch without changing preprocess() signature
- drupal_add_feed() moved into forum_page() because this functionality is for page callback and not for theme layer, this is a ONLY place in core where drupal_add_feed() used in theme()
- Added title callback to set page title with menu.
- Added tests that ensures that page title does not inherits from vocabulary name
Breadcrumbs still uses menu router name (Forums), this should be fixed in follow-ups after landing of #576290: Breadcrumbs don't work for dynamic paths & local tasks
EDIT:
- I think breadcrumb generation should be moved in separate internal function because used 2 places
- menu_get_item() returns a router item but we need menu link title to set breadcrumbs so waiting for #576290
Comment #46
pwolanin CreditAttribution: pwolanin commentedShould probably be postponed until #576290: Breadcrumbs don't work for dynamic paths & local tasks is done.
Comment #47
sunI'm not sure whether this is really related to the Breadcrumbs patch. It also seems like the title of this issue is outdated.
The callback should be forum_forum_page_title - this is the page title for a single forum, not for the path 'forum' itself.
This makes the forum breadcrumb start with the title of the menu link of the path 'forum', if there happens to be one.
If there is no menu link to 'forum', then it is going to use t('Forums'), i.e., the internal 'title' of the path 'forum' defined in forum_menu().
I'm not sure whether that is intentional and an improvement -- while using a potentially existing link title is correct, the fallback to the internal menu router item title does not sound correct to me. It would have to fall back to the vocabulary name -- which may be translated through a contributed module.
Separate issue: I'm pretty sure that it's wrong to call those theme functions in this template preprocessor - instead, the variables should contain renderable arrays.
Not sure whether the elseif is correct. Dropping the else would probably be safer.
Powered by Dreditor.
Comment #48
andypostThe main problem that
$router_item = menu_get_item('forum');
always returns router not menu link!Fallback to vocabulary name probably is preferred but in D7 we also have field name (Forums) so it's could be a alternative
drupal_set_title(t('No forums defined'));
was always there so not sure that we should drop this.Comment #49
cosmicdreams CreditAttribution: cosmicdreams commentedHow can we move forward on this bug? I'd like to help, but in reading the history on this bug, I'm unclear what I should be spending me time doing.
Comment #50
sunAlso read the entire issue now. The problem outlined in the OP should no longer be an issue in D7. While work on the D7 version of http://drupal.org/project/translatable didn't officially start yet, taxonomy term names have been made translatable via #593746: Prepare Drupal core for dynamic data translation
Thus, I guess this issue won't fix. (Translatable select queries cannot be done in D6)
Comment #51
webchickOk.
Comment #52
andypostOk, let's make a bit of cleanup:
this is a ONLY API change
- page title uses unified forum_page_title() callback same as node_page_title() and user_page_title() (@sun not a forum_forum_page_title name)
- title and breadcrumbs should not set within preprocess function.
- same for RSS feed
- code comments a bit extended
Suppose this is enought for cleanup so no api changes.
PS: Latter in release circle we can move out theme() functions from preprocess
Comment #53
andypost-d7 suffix no longer works for bot
Comment #55
andypostcopy/paste error
Comment #56
andypostComment #57
sunThis part is missing.
Powered by Dreditor.
Comment #58
andypostThis part is not needed because of title callback
EDIT: also title callback makes forum title an easy to override from contrib
Comment #59
andypostRe-roll for D8 in flwor commited #1041906: Taxonomy term menu link title overrides term page title
Comment #61
andypost#59: 138145-forum-cleanup.patch queued for re-testing.
Comment #63
alexweber CreditAttribution: alexweber commentedPatch from #59 re-rolled for new Drupal 8 directory structure
Comment #65
andypostRemoved
drupal_add_feed('taxonomy/term/' . $variables['tid'] . '/feed', 'RSS - ' . $title);
from template_preprocess_forums() which caused this exceptions. This one should stay only in forum_page()Comment #66
andypostThis preprocess should be fixed but probably not in this issue
Comment #67
andypostMarked as duplicate #72871: Renaming the Forum
Comment #68
andypostRe-roll for current HEAD
Another duplicate #1866900: Forum pages are adjusting breadcrumb too late
Comment #69
andypostRight patch that does not touch notice from #1796292: Use of undeclared variable : $original_title in core/modules/forum/forum.module on line 1232
Comment #70
sunLooks great :)
Only a minimal remark:
I think we can use
entity_page_label()
here.Will merely require a trivial change in
forum_forum_load()
, replacing the(object) array(...)
with a properentity_create('taxonomy_term', array(...))
.Comment #71
andypost@sun thanx a lot for idea
Here's a patch:
- page title uses entity_page_label()
- no more forum object - used stub term entity.
- added test for title change
- additional cleanu-up for admin tests
Comment #72
sunLooks good to me. Please wait for green light from the bot though.
Also cleaning out some tags. I don't think this can be backported in any way, since theme functions may be overridden, by design.
Comment #73
webchickThis all looks like very nice clean-up, but yes alas, not for D6 in its current form, anyway.
Committed and pushed to 8.x. Thanks!
Comment #74.0
(not verified) CreditAttribution: commentedUpdated issue summary.