Here is forum_access_menu_get_item_alter():
/**
* Implements hook_menu_get_item_alter().
*
* Saves the tid on the forum-specific pages.
*/
function forum_access_menu_get_item_alter(&$router_item, $path, $original_map) {
switch ($original_map[0]) {
case 'forum':
if (empty($original_map[1])) {
forum_access_current_tid(0);
}
elseif (is_numeric($original_map[1])) {
forum_access_current_tid($original_map[1]);
}
break;
case 'node':
if (isset($original_map[1]) && is_numeric($nid = $original_map[1]) && ($node = node_load($nid)) && ($tid = _forum_access_get_tid($node))) {
forum_access_current_tid($tid);
}
break;
}
}
The call to forum_access_current_tid($tid) is setting a static variable so that it is available in future calls to the function. Unfortunately, if another module calls menu_get_item($path), the menu_alter hook above will be run again, and it will reset the $tid depending on the $path supplied in the menu_get_item call. I think we should modify the code for hook_menu_get_item_alter as follows:
/**
* Implements hook_menu_get_item_alter().
*
* Saves the tid on the forum-specific pages.
*/
function forum_access_menu_get_item_alter(&$router_item, $path, $original_map) {
if(forum_access_current_tid() === 0)
{
switch ($original_map[0]) {
case 'forum':
if (isset($original_map[1]) && is_numeric($original_map[1])) {
forum_access_current_tid($original_map[1]);
}
break;
case 'node':
if (isset($original_map[1]) && is_numeric($nid = $original_map[1]) && ($node = node_load($nid)) && ($tid = _forum_access_get_tid($node))) {
forum_access_current_tid($tid);
}
break;
}
}
}
This will only set the $tid if it hasn't already been set before. I also removed the redundant if block, since $tid will always default to zero when it isn't set anyway.
Repodution steps:
- Enable Forum Access module
- In another module, implement a hook such as hook_preprocess_link(&$link) and code it as follows:
function hook_preprocess_link(&$link) { $router_item = menu_get_item('forum'); }
- In forum_access_node_access(), add some code to dump the contents of line 563 to screen. This will be the tid of the forum we are currently viewing.
- Browse to a forum, and view the output of the $tid we dumped in the previous step. It will always read 0 because the menu_get_item('forum') call we made in the preprocess link hook is overwriting the $tid static variable.
Comments
Comment #1
salvisI can see how this could cause problems, thank you for the analysis.
Please post a patch against the -dev version and set the status to 'needs review', so that the testbot can do its thing.
Comment #2
EAnushan CreditAttribution: EAnushan commentedAttached the patch.
Comment #3
salvisPlease use 2 spaces rather than tabs for indenting, according to the http://drupal.org/coding-standards
Comment #4
EAnushan CreditAttribution: EAnushan commentedReplaced tabs with double spaces.
Comment #5
EAnushan CreditAttribution: EAnushan commentedComment #6
salvisSorry about nit-picking and bothering you with formatting issues. It's a kind of initiation rite that all new contributors have to go through. Once you've fixed your editor settings and adjusted your coding style to the http://drupal.org/coding-standards, everything will go much smoother, and it makes sense to do this sooner rather than later.
Add space after 'if'. Are you sure that the typed comparison (=== rather than ==) is needed?
Opening brace always at the end of the preceding brace.
Remove trailing space on empty line.
Remove trailing space on empty line.
Indenting: closing brace must line up with the 'switch'.
Comment #7
EAnushan CreditAttribution: EAnushan commentedSorry it took me so long. I only just found some time to update the patch.
Comment #8
salvisPushed to the -dev version (give it up to 12h to be repackaged). Thanks!
Comment #9.0
(not verified) CreditAttribution: commentedForgot an isset check.