Replacing Menu Item Visibility module with custom "in code" solution

18 Mar · by Tim Kamanin · 3 min read

I'm a big fan of fighting with Drupal's inefficiencies and bottlenecks. Most of these come from contrib modules. Everytime we install a contrib module we should be ready for surprises which come on board with the module.

One of the latest examples is Menu item visibility (https://drupal.org/project/menu_item_visibility) that turned out to be a big trouble maker on one of my client 's sites. Menu item visibility is a simple module that let 's you define link visibility based on a user's role. Simple and innocent... until you look under the hood.

The thing is Menu item visibility stores it 's data in database and does a query per every menu item on the page. In my case it produced around 30 queries per page and 600 queries on menu/cache rebuild (which normally equals to the number of menu items you have in your system).

The functionality that this module gives to an end user is good and useful (according to drupal.org: 6,181 sites currently report using this module) but as you see, storing these settings in db can become a huge bottleneck for your site. I looked at the Menu item visibility source and came to this "in code" solutions that fully replicates the module functionality but stores data in code.

Step 1.

Create a custom module and call it like Better menu item visibility. , machine name: _better_menu_item_visibility_.

Step 2.

Let's add the first function that holds our menu link item id (mlid) and role id (rid) data:

/**
 * This function returns a list of mlid's with a list of roles that have access to link items.
 * You can change the list to add new menu items or/and roles
 * The list is presented in a format:
 * 'mlid' => array('role_id', 'role_id),
 */
function better_menu_item_visibility_menu_item_visibility_role_data() {
  return array(
'15' => array('1', '2'),
'321' => array('1'),
'593' => array('3'),
// Add as many combinations as you want.
  );
}

This function returns an array with menu link item ids and roles that can access the item. If you already have Menu item visibility installed, you can easily port the data from the db table {menu_links_visibility_role} into this function.

Step 3.

And now let's do the dirty job and process the menu items:

/**
 * Implements hook_translated_menu_link_alter().
 */
function better_menu_item_visibility_translated_menu_link_alter(&$item, $map) {
  if (!empty($item['access'])) {
global $user;

// Menu administrators can see all links.
if ($user->uid == '1' || (strpos(current_path(), 'admin/structure/menu/manage/' . $item['menu_name']) === 0 && user_access('administer menu'))) {
  return;
}

$visibility_items_for_roles = better_menu_item_visibility_menu_item_visibility_role_data();
if (!empty($visibility_items_for_roles[$item['mlid']]) && !array_intersect($visibility_items_for_roles[$item['mlid']], array_keys($user->roles))) {
  $item['access'] = FALSE;
}
  }
}

In short this function skips access check for user 1 and for user that has 'administer menu' permission and does the access check for link menu items listed in better_menu_item_visibility_menu_item_visibility_role_data. As you see, instead of calling database it gets data from the code which is really fast. Let me know what you think and share your ways of fighting with Drupal 's inefficiencies.

Comments

Required for comment verification



Joel Pittet

Try this on for size. Keep the UI, make 1 call to that menu_links_visibility_role table to build up your list and statically store it on the alter hook. That way you reduce the SQL calls on all but one and the trade off is memory for the request to hold on to that array, which if it's not used heavily on the site it would be fairly small like in the case for the article. Thoughts?

Reply · 2 years, 8 months ago
knibals

+1 Did the same!

Reply · 3 years, 9 months ago
Ali Nouman

Hi correct me, if i am wrong but shouldnot this

!array_intersect($visibility_items_for_roles[$item['mlid']], array_keys($user->roles))

be

array_intersect($visibility_items_for_roles[$item['mlid']], array_keys($user->roles)) 

I guess there should be not ! operator.

Reply · 3 years, 9 months ago
Kristoffer Wiklund

I would put uid==1 first in the if case, to do that check first. Too minimize code running for admin and the check is fastest. You will never notice the performance improvement but as good practice.

Reply · 3 years, 9 months ago
Leon Kessler

Instead of creating an entirely new module, how about contributing a patch to the current menu_item_visibility module? Surely this is the correct approach for fighting Drupal's inefficiencies.

Reply · 3 years, 9 months ago
Andrew Berezovsky

To add to my previous comment - there is already an issue and a patch - https://drupal.org/node/1848724

Reply · 3 years, 9 months ago
Andrew Berezovsky

It feels like you should probably create the issue on this in the module queue - Dave Reid is a great developer and maintainer, he might have thoughts on how to improve this situation (I would suggest to change the DB query to request results for all menu items and cache it). Also, for the "Everytime we install a contrib module we should be ready for surprises which come on board with the module." phrase - sounds frightening. I suggest to look on it from the other side - there might be issues (and usually they are already reported), but they are discussed and fixed in the open issue queues as Drupal is open source. It always was Drupal community strength to work together and unite forces on improving the already created solutions and not duplicating the efforts by producing the similar solutions.

Reply · 3 years, 9 months ago
Hans

What is a good use case for such a module? If you want just visibility based on roles you could print some style display:none in the head based on roles. If you don't want certain roles to access the menupath you need an access control solution.

Reply · 3 years, 9 months ago
rpsu

Hi - nice tip, great timing for my self :) One thing to simplify the code, user_access() returns always TRUE for uid 1, so mid-part of 3) can be slightly shorter (and readable):

// Menu administrators can see all links.
if (strpos(current_path(), 'admin/structure/menu/manage/' . $item['menu_name']) === 0 && user_access('administer menu')) {
  return;
}

Right?

Reply · 3 years, 9 months ago