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

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

Submitted by rpsu on Tue, 2014-03-18 16:52

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?

Submitted by Tim on Tue, 2014-03-18 19:56

actually not, your code checks only the case when 'admin/structure/menu/manage/' is viewed by a user who has 'administer menu' permission (including uid 1), I added uid == 1 to skip checks in all conditions, not only when user is on 'admin/structure/menu/manage/' page, got it? :)

Submitted by rpsu on Wed, 2014-03-19 09:14

Ah, okay. Got it :)

Submitted by Hans on Tue, 2014-03-18 21:46

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.

Submitted by Andrew Berezovsky on Wed, 2014-03-19 06:58

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.

Submitted by Tim on Wed, 2014-03-19 16:35

Well, by talking about "surprises" I meant that you always should be aware of what module you're installing and what bottlenecks it can potentially cause: we're an open source community this is a plus but that doesn't guarantee that you get a perfect solution by downloading a module.

Submitted by Andrew Berezovsky on Wed, 2014-03-19 07:01

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

Submitted by Tim on Wed, 2014-03-19 16:29

Good to hear.

Submitted by Leon Kessler on Wed, 2014-03-19 15:05

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.

Submitted by Kristoffer Wiklund on Wed, 2014-03-19 16:24

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.

Submitted by Tim on Wed, 2014-03-19 16:26

Good note, done.

Submitted by Ali Nouman on Wed, 2014-03-19 18:13

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.

Submitted by Tim on Thu, 2014-03-20 10:09

there should be "!" operator and it is there. According to php docs: array_intersect() returns an array containing all the values of array1 that are present in all the arguments.

So, if array_intersect() finds at least something in the $visibility_items_for_roles[$item['mlid']] it will return TRUE, so if user has one of the roles required, it will return TRUE, if it doesn't match, it will return FALSE so we're looking for the FALSE key to deny access to the user.

So simply put, in line

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

in human language we do: "if the menu link is in condition array, we check if any of the roles in that array matches at least to one of current user roles, if the result is negative, we hide the link".

Hope I explained that well.

Submitted by knibals on Thu, 2014-03-20 17:55

+1 Did the same!

Add new comment

You are here