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:

  1. Enable Forum Access module
  2. 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');
      }
      
  3. 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.
  4. 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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

salvis’s picture

Version: 7.x-1.2 » 7.x-1.x-dev

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

EAnushan’s picture

Status: Active » Needs review
FileSize
1.3 KB

Attached the patch.

salvis’s picture

Status: Needs review » Needs work
+++ b/forum_access.module
@@ -456,23 +456,23 @@ function forum_access_custom_theme() {
+	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;
+  		}
+	}

Please use 2 spaces rather than tabs for indenting, according to the http://drupal.org/coding-standards

EAnushan’s picture

Replaced tabs with double spaces.

EAnushan’s picture

Status: Needs work » Needs review
salvis’s picture

Status: Needs review » Needs work

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

+++ b/forum_access.module
@@ -456,23 +456,23 @@ function forum_access_custom_theme() {
+  if(forum_access_current_tid() === 0)

Add space after 'if'. Are you sure that the typed comparison (=== rather than ==) is needed?

+++ b/forum_access.module
@@ -456,23 +456,23 @@ function forum_access_custom_theme() {
+  if(forum_access_current_tid() === 0)
+  {

Opening brace always at the end of the preceding brace.

+++ b/forum_access.module
@@ -456,23 +456,23 @@ function forum_access_custom_theme() {
+  ¶
+      case 'forum':

Remove trailing space on empty line.

+++ b/forum_access.module
@@ -456,23 +456,23 @@ function forum_access_custom_theme() {
+  ¶
+      case 'node':

Remove trailing space on empty line.

+++ b/forum_access.module
@@ -456,23 +456,23 @@ function forum_access_custom_theme() {
+        break;
+      }
+  }

Indenting: closing brace must line up with the 'switch'.

EAnushan’s picture

Status: Needs work » Needs review
FileSize
1.3 KB

Sorry it took me so long. I only just found some time to update the patch.

salvis’s picture

Status: Needs review » Fixed

Pushed to the -dev version (give it up to 12h to be repackaged). Thanks!

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

Forgot an isset check.