Has anybody else noticed that the settings for smart_ip do not save the roles to check correctly?

What the smart_ip admin settings saves in the variable is the state of the checkboxes (ie. an array of true or false in the order of the roles choices). Not only is this not going to work when new roles are added or existing roles changed (in which case a new role could show up as checked, etc), the code that checks if the geoip should be checked for a role actually expects the role-id (rid) to be in the variable.

In hook_user it checks against the smart_ip_roles_to_geolocate array, expecting it to contain role ids, but all smart_ip_admin_settings_submit ever saves is an array of (true,true, false,false, etc)

        $roles_to_geo_locate = variable_get('smart_ip_roles_to_geolocate', array(DRUPAL_AUTHENTICATED_RID));
        foreach($roles_to_geo_locate as $rid){
          if(array_key_exists($rid, $account->roles)){
            smart_ip_set_location_data($account, smart_ip_get_location(ip_address()));
            break;
          }
        }

As soon as one saves the roles to check, drupal shows a lot of error messages as soon as a user logs in, because the authenticated user role will never be in the value of smart_ip_roles_to_geolocate

CommentFileSizeAuthor
#2 1076638-roles-not-saved-correctly.patch866 byteslance.gliser

Comments

lance.gliser’s picture

lance.gliser commented

Hello jhm. I'll take a look and whip up a patch if needed.

lance.gliser’s picture

lance.gliser commented
Status: Active » Needs review
StatusFileSize
new 1076638-roles-not-saved-correctly.patch866 bytes

You were absolutely correct jhm. Thank you for catching that.

Here's the fix code:

function smart_ip_admin_settings_submit($form, &$form_state) {
  global $user;

  // Save the roles to geolocate
  $roles_to_geolocate = array();
  foreach ($form_state['values']['smart_ip_roles_to_geolocate'] as $rid => $value) {
    if($rid == $value){
      $roles_to_geolocate[] = $rid;
    }
  }
  variable_set('smart_ip_roles_to_geolocate', $roles_to_geolocate);
  unset($form_state['values']['smart_ip_roles_to_geolocate']);

I'll attach a path as well.

lance.gliser’s picture

lance.gliser commented
Assigned: Unassigned » arpeggio

I still have some account issues arpeggio, the transition to github changed the process and I haven't taken some required step. The patch should let you get this fixed up. I'll include the update when I push in the statistics.

arpeggio’s picture

arpeggio commented
Status: Needs review » Fixed

Thank you lance.gliser. I have already made git push for this issue.

jhm’s picture

jhm commented

Happy I could help

Status: Fixed » Closed (fixed)

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