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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cburschka’s picture

Version: 5.1 » 6.x-dev
Status: Active » Needs review
FileSize
690 bytes

The 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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

The 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.

salvis’s picture

I 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.

Gábor Hojtsy’s picture

t() 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.

salvis’s picture

What 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?

Liam McDermott’s picture

Version: 6.x-dev » 7.x-dev

Think this is still valid so bumping version.

LoveCharge’s picture

subscribing

toemaz’s picture

Workaround: in forums.tpl.php , simple set drupal_set_title(t('Forums'));

andypost’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D6, +Quick fix
FileSize
687 bytes

Why 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

andypost’s picture

Same patch for d6

andypost’s picture

Added comment

VladSavitsky’s picture

Why 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.

VladSavitsky’s picture

Status: Needs review » Reviewed & tested by the community

Works fine

andypost’s picture

Priority: Normal » Major

I think this major because drupal is multilingual and translation of vocabulary name should not have meaning on page title

Dries’s picture

Status: Reviewed & tested by the community » Needs work

andy, can you explain in the code comment _why_ we set the title conditionally? That would be great.

andypost’s picture

Status: Needs work » Needs review
FileSize
1.39 KB

A bit reorganized code with comment. Suppose this approach is much cleaner.

andypost’s picture

I 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.

andypost’s picture

Slightly 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.

Dries’s picture

This looks like a nice clean-up.

+++ modules/forum/forum.pages.inc	11 Aug 2010 08:51:10 -0000
@@ -15,6 +15,37 @@ function forum_page($forum_term = NULL) 
+  // The page title for forum list and breadcrumbs start should be inherited

Something is wrong with this sentence. The word 'start' doesn't seem to belong here.

andypost’s picture

Sure, this was a typo, re-roll

andypost’s picture

For D6 we can't modify template_preprocess_forums() because a lot of themes could relay on it. So Just a simple patch backport #17

Gábor Hojtsy’s picture

Looks 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.

andypost’s picture

Issue tags: -Quick fix +beta blocker

Changing this in beta would be bad

Gábor Hojtsy’s picture

I 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?

Gábor Hojtsy’s picture

Status: Needs review » Needs work
FileSize
4.47 KB

Indeed, looks like forum_node_view() has the same issue:

/**
 * Implements hook_node_view().
 */
function forum_node_view($node, $view_mode) {
  $vid = variable_get('forum_nav_vocabulary', 0);
  $vocabulary = taxonomy_vocabulary_load($vid);
  if (_forum_node_check_node_type($node)) {
    if (node_is_page($node)) {
      // Breadcrumb navigation
      $breadcrumb[] = l(t('Home'), NULL);
      $breadcrumb[] = l($vocabulary->name, 'forum');
      if ($parents = taxonomy_get_parents_all($node->forum_tid)) {
        $parents = array_reverse($parents);
        foreach ($parents as $parent) {
          $breadcrumb[] = l($parent->name, 'forum/' . $parent->tid);
        }
      }
      drupal_set_breadcrumb($breadcrumb);

    }
  }
}

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.

andypost’s picture

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?

forum_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

how are breadcrumbs on forum nodes made consistent with this new approach?

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

tloudon’s picture

andypost, 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

andypost’s picture

@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_page

Also 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

Gábor Hojtsy’s picture

@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.

tloudon’s picture

ok, 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 :)

andypost’s picture

Status: Needs work » Needs review
FileSize
5.31 KB

New 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

philbar’s picture

Someone with proper permissions, add this too:

http://drupal.org/community-initiatives/drupal-core

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@andypost: ok, then its confusing to call it forum_term in the caller.

andypost’s picture

Status: Needs work » Needs review
FileSize
5.68 KB

Discussed 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()

Status: Needs review » Needs work

The last submitted patch, 148145-forum-page-title-d7.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
5.68 KB

Missed to rename forum => forum_term

andypost’s picture

Issue tags: +API change, +D7 API clean-up

This 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"

philbar’s picture

Priority: Major » Critical

Beta blockers = Critical

webchick’s picture

Priority: Critical » Normal
Issue tags: -beta blocker

Why on earth would a bug that's been there since Drupal 6 be a critical, beta-blocking bug?

andypost’s picture

@webchick because this patch changes API (theme function params)

webchick’s picture

Then we should probably fix it a different way? I assume you want this back-ported to D6.

andypost’s picture

@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 layer

webchick’s picture

What'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.

Gábor Hojtsy’s picture

As 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.

andypost’s picture

Patch 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

pwolanin’s picture

Should probably be postponed until #576290: Breadcrumbs don't work for dynamic paths & local tasks is done.

sun’s picture

Status: Needs review » Needs work

I'm not sure whether this is really related to the Breadcrumbs patch. It also seems like the title of this issue is outdated.

+++ modules/forum/forum.module	1 Sep 2010 14:11:47 -0000
@@ -96,6 +96,8 @@ function forum_menu() {
   $items['forum/%forum_forum'] = array(
...
+    'title callback' => 'forum_page_title',
+    'title arguments' => array(1),

The callback should be forum_forum_page_title - this is the page title for a single forum, not for the path 'forum' itself.

+++ modules/forum/forum.module	1 Sep 2010 14:11:47 -0000
@@ -274,13 +283,12 @@ function _forum_node_check_node_type($no
 function forum_node_view($node, $view_mode) {
...
-      $breadcrumb[] = l($vocabulary->name, 'forum');
+      $menu_item = menu_get_item('forum');
+      $breadcrumb[] = l($menu_item['title'], 'forum');

+++ modules/forum/forum.pages.inc	1 Sep 2010 14:11:47 -0000
@@ -15,6 +15,34 @@ function forum_page($forum_term = NULL) 
+  if ($forum_term->tid) {
+    // Breadcrumbs should be inherited from menu link title to follow the menu
+    // item title changes.
+    $router_item = menu_get_item('forum');
+    $breadcrumb[] = l($router_item['title'], 'forum');
+  }

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.

+++ modules/forum/forum.module	1 Sep 2010 14:11:47 -0000
@@ -954,31 +961,6 @@ function forum_get_topics($tid, $sortby,
       $variables['forums'] = theme('forum_list', $variables);

@@ -989,7 +971,6 @@ function template_preprocess_forums(&$va
       $variables['topics'] = theme('forum_topic_list', $variables);

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.

+++ modules/forum/forum.pages.inc	1 Sep 2010 14:11:47 -0000
@@ -15,6 +15,34 @@ function forum_page($forum_term = NULL) 
+  if ($forum_term->tid) {
+    // Add RSS feed.
+    drupal_add_feed('taxonomy/term/' . $forum_term->tid . '/0/feed', 'RSS - ' . $forum_term->name);
+  }
+  elseif (empty($forum_term->forums) && empty($forum_term->parents)) {
+    drupal_set_title(t('No forums defined'));
+  }

Not sure whether the elseif is correct. Dropping the else would probably be safer.

Powered by Dreditor.

andypost’s picture

Status: Needs work » Needs review

The 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.

cosmicdreams’s picture

How 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.

sun’s picture

Also 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)

webchick’s picture

Status: Needs review » Closed (won't fix)

Ok.

andypost’s picture

Status: Closed (won't fix) » Needs review
Issue tags: -API change
FileSize
4.63 KB

Ok, 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

andypost’s picture

-d7 suffix no longer works for bot

Status: Needs review » Needs work

The last submitted patch, 148145-forum-page-title-cleanup.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
4.63 KB

copy/paste error

andypost’s picture

sun’s picture

Status: Needs review » Needs work
+++ modules/forum/forum.module	30 Nov 2010 14:33:18 -0000
@@ -955,31 +964,6 @@ function forum_get_topics($tid, $sortby,
-      if ($p->tid == $variables['tid']) {
-        $title = $p->name;
-      }
...
-  drupal_set_title($title);

This part is missing.

Powered by Dreditor.

andypost’s picture

Status: Needs work » Needs review

This part is not needed because of title callback

EDIT: also title callback makes forum title an easy to override from contrib

andypost’s picture

Version: 7.x-dev » 8.x-dev
FileSize
3.95 KB

Status: Needs review » Needs work
Issue tags: -Needs backport to D6, -D7 API clean-up

The last submitted patch, 138145-forum-cleanup.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review

#59: 138145-forum-cleanup.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs backport to D6, +D7 API clean-up

The last submitted patch, 138145-forum-cleanup.patch, failed testing.

alexweber’s picture

Status: Needs work » Needs review
FileSize
3.99 KB

Patch from #59 re-rolled for new Drupal 8 directory structure

Status: Needs review » Needs work

The last submitted patch, 138145-forum-cleanup.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
4.37 KB

Removed 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()

andypost’s picture

Status: Needs review » Needs work
Issue tags: +D8MI, +Twig

This preprocess should be fixed but probably not in this issue

andypost’s picture

Marked as duplicate #72871: Renaming the Forum

andypost’s picture

Status: Needs work » Needs review
FileSize
5.62 KB

Re-roll for current HEAD

Another duplicate #1866900: Forum pages are adjusting breadcrumb too late

andypost’s picture

sun’s picture

Looks great :)

Only a minimal remark:

+++ b/core/modules/forum/forum.module
@@ -100,6 +100,8 @@ function forum_menu() {
   $items['forum/%forum_forum'] = array(
...
+    'title callback' => 'forum_page_title',

@@ -164,6 +166,13 @@ function forum_menu() {
+function forum_page_title($forum_term) {
+  return $forum_term->name;
+}

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 proper entity_create('taxonomy_term', array(...)).

andypost’s picture

@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

sun’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs backport to D6, -D7 API clean-up

Looks 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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

This 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!

Status: Fixed » Closed (fixed)
Issue tags: -D8MI, -Twig

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.