Is there any particular reason for forum module not to implement taxonomy_term_path hook?

instead of very complicated and time consuming (if for each link) code like current:

function forum_link_alter(&$node, &$links) {
  foreach ($links as $module => $link) {
    if (strstr($module, 'taxonomy_term')) {
      // Link back to the forum and not the taxonomy term page. We'll only
      // do this if the taxonomy term in question belongs to forums.
      $tid = str_replace('taxonomy/term/', '', $link['href']);
      $term = taxonomy_get_term($tid);
      if ($term->vid == _forum_get_vid()) {
        $links[$module]['href'] = str_replace('taxonomy/term', 'forum', $link['href']);
      }
    }
  }
}

there could be only this:

function forum_term_path($term) {
  return 'forum/'. $term->tid;
}

Changing those lines of code provides the same functionality with much less and faster code and gives chance to contributed modules to properly handle or imitate forum taxonomy links

This patch should be also rerolled versus D6.

CommentFileSizeAuthor
#12 197864-forum-term-path-D6.patch1.12 KBDave Reid
#4 forum_hook_term_path.patch1.11 KBGurpartap Singh
#3 forum_hook_term_path.patch1.11 KBalpritt
forum.patch1.11 KBvito_swat
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Version: 5.3 » 6.x-dev

This makes sense to me. vito_swat - what do you think about tackling this in D6 first? I think Drumm prefers to have bugs fixed in HEAD and then backported.

Liam McDermott’s picture

Liam McDermott CreditAttribution: Liam McDermott commented
Version: 6.x-dev » 7.x-dev
Status: Needs review » Needs work

This will need to be re-rolled against HEAD.

alpritt’s picture

alpritt CreditAttribution: alpritt commented
Status: Needs work » Needs review
FileSize
forum_hook_term_path.patch1.11 KB

Re-rolled for HEAD and added comment.

Gurpartap Singh’s picture

Gurpartap Singh CreditAttribution: Gurpartap Singh commented
FileSize
forum_hook_term_path.patch1.11 KB

Drupal coding standards have changed for the placement of concatenation dot.

This looks quick and good. Dries?

Murz’s picture

Murz
Primary language English
Location Yerevan, Armenia
CreditAttribution: Murz commented

On my Drupal 5.7 works good and smalls query list by many queries!

And I have found a bug in function forum_link_alter(&$node, &$links) from 5.x versions:

String $tid = str_replace('taxonomy/term/', '', $link['href']); does not replace 'taxonomy' always, because taxonomy link may be not only like 'taxonomy/term/%', but much more, for example - 'nodeorder/term/%'. In this case function forum_link_alter generates bad function call like this: term = taxonomy_get_term('nodeorder/term/6');.

I hope that in 7.x this bug will be solved, because in my 5.x and 6.x i needs to solve it with hands for perfomance reasons.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community

Tested this on 5.x without issue, works nice. Good find.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Still applies cleanly.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

This does not seem to break any forum tests. Committed to CVS HEAD. Thanks.

Anonymous’s picture

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

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

vito_swat’s picture

vito_swat CreditAttribution: vito_swat commented
Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Reviewed & tested by the community

Any reason for not committing this to 6.x and 5.x?

StevenPatz’s picture

StevenPatz
he/him
Primary language English
Location Alexandria VA
CreditAttribution: StevenPatz commented
Status: Reviewed & tested by the community » Active
Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Active » Reviewed & tested by the community
FileSize
197864-forum-term-path-D6.patch1.12 KB

Straight upport to D6.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Version: 6.x-dev » 5.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

How this is invoked was not mentioned here, neither on the hook page, so I've looked that up:

/**
 * For vocabularies not maintained by taxonomy.module, give the maintaining
 * module a chance to provide a path for terms in that vocabulary.
 *
 * @param $term
 *   A term object.
 * @return
 *   An internal Drupal path.
 */

function taxonomy_term_path($term) {
  $vocabulary = taxonomy_vocabulary_load($term->vid);
  if ($vocabulary->module != 'taxonomy' && $path = module_invoke($vocabulary->module, 'term_path', $term)) 
{
    return $path;
  }
  return 'taxonomy/term/'. $term->tid;
}

From this, and taxonomy module's use of taxonomy_term_path(), it looks like this is a very good change indeed. Thanks, committed.
This should also be backported to 5.x. (I did need to modify Dave's reroll above for spacing code style for Drupal 6). This was my commit message:

cvs commit -m "#197864 by vito_swat, alpritt, Murz, catch: Use hook_term_path() in forum module instead of hook_link_alter(); simplfies code, improves performance and compatibility."

vito_swat’s picture

vito_swat CreditAttribution: vito_swat commented
Status: Patch (to be ported) » Reviewed & tested by the community

backport for 5.x is an attachment to the issue post (#0). tested in #5 and #6 and by me.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Reviewed & tested by the community » Fixed

Committed to 5.x.

Status: Fixed » Closed (fixed)

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