forum module uses hook_link_alter instead of taxonomy_term_path hook
vito_swat - December 5, 2007 - 00:02
| Project: | Drupal |
| Version: | 5.x-dev |
| Component: | forum.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
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:
<?php
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:
<?php
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| forum.patch | 1.11 KB | Ignored | None | None |
#1
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.
#2
This will need to be re-rolled against HEAD.
#3
Re-rolled for HEAD and added comment.
#4
Drupal coding standards have changed for the placement of concatenation dot.
This looks quick and good. Dries?
#5
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
<?php$tid = str_replace('taxonomy/term/', '', $link['href']);
?>
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.
#6
Tested this on 5.x without issue, works nice. Good find.
#7
Still applies cleanly.
#8
This does not seem to break any forum tests. Committed to CVS HEAD. Thanks.
#9
Automatically closed -- issue fixed for two weeks with no activity.
#10
Any reason for not committing this to 6.x and 5.x?
#11
#12
Straight upport to D6.
#13
How this is invoked was not mentioned here, neither on the hook page, so I've looked that up:
<?php
/**
* 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."#14
backport for 5.x is an attachment to the issue post (#0). tested in #5 and #6 and by me.
#15
Committed to 5.x.
#16
Automatically closed -- issue fixed for 2 weeks with no activity.