TonyTheGeek [tonythegeek]

1. package = DU Custom Modules
   

   Remove the package name.

2.   $items['admin/user/simpleroles/group_mapping'] = array(
        'title'  =>  'Group Mappings',
        'description'  => 'Change how ldap to drupal roles are mapped for the site',
        'page callback'  => 'drupal_get_form',
        'page arguments'  => array('simpleroles_group_form'),
        'access arguments'  => array('configure simple roles'),
        'file'  => 'simpleroles.group.inc',
     );
     $items['admin/user/simpleroles/user_mapping'] = array(
        'title'  =>  'User Mappings',
        'description'  => 'Change how ldap to drupal roles are mapped for the site',
        'page callback'  => 'drupal_get_form',
        'page arguments'  => array('simpleroles_user_form'),
        'access arguments'  => array('configure simple roles'),
        'file'  => 'simpleroles.user.inc',
     );
   

   Strings used in user interface should have the first word in capital case, and the other words in lower case (with the exception of proper nouns, adjectives derived from proper nouns, and acronyms).

3. See the Drupal coding standards to understand how a module code should be written. In particular, see the part about the namespace respect.

4.   if (!($connect=@ldap_connect($ldap_server))) {
       die("Could not connect to ldap server");
     }
   

   Modules don't use die().

5.   $form['add_drupal_role']['name'] = array(
       '#type' => 'textfield',
       '#title' => 'Drupal Role Name',
       '#size' => 20,
     );
   

   Strings used in the user interface needs to be translated.

6. function simpleroles_user_form_submit($form, &$form_state) {
     switch ($form_state["values"]["op"]) {
   

   Code should not depend from op, which could be not present anymore. It would be better to use a different submission function for each button present in the form that is not the default one.

7.         watchdog('simpleroles', 'A db error occured', $err, WATCHDOG_ERROR);
   
   

   The call misses a parameter. Rather than concatenating two strings, it would be better to use a placeholder.

8.       $t = array();
         foreach ($form_state["values"]["delete"] as $id => $on) {
           if ($on) {
             $t[$id] = "%d";
           }
         }
         if (sizeof($t)) {
           //$query = "DELETE FROM {external_role_mapping} WHERE id IN (%d,%d,%d)";
           $query = "DELETE FROM {external_username_mapping} WHERE id IN (" . implode($t, ",") . ")";
           call_user_func_array("db_query", array_merge((array)$query, array_keys($t)));
         }
   

   It would be shorter, if the code would use db_placeholders().

9.     $schema['external_role_mapping'] = array(
         'description' => t('Table that contains the external role names and the internal roles they map to'),
   

   Schema descriptions should not be passed to t() anymore. See system_schema() for an example of what done by Drupal core code.

10.         call_user_func_array("db_query", array_merge((array)$query, array_keys($t)));
    

    Why isn't the code simply calling db_query()?

11.         drupal_set_message($form_state["values"]["add_drupal_role"]['name'] . t(" was succesfully created."));
    

    Avoid the concatenation of strings when possible, for strings that are translated. In this case, it is enough to use a placeholder.

12. The file LICENSE.txt needs to be removed; Drupal.org CVS doesn't allow to commit that file.

There have not been replies from the OP in the past 7 days. I am marking this report as won't fix.