fizk [fizk]

A good idea to me. The current names listed are a good start, I would also like to recommend adding:

- toor
- superadmin
- op
- operator
- oper

Hi, just have five minutes and would love to help on this..

I just stopped doing the coding standard review here. but I realized other 'hard to categorize' issues also:

    ...
    $placeholders = implode(',', array_fill(0, $size, "'%s'"));
    $args = array_merge(array($type), array_keys($masks));
    $count = db_result(db_query("SELECT COUNT(DISTINCT mask) FROM {access} WHERE type = '%s' AND mask IN (". $placeholders .")", $args));
 ...

This placeholders usage generates errors when no rule is selected, this code is spreaded all over the code.

and, in the menu entry definition..

  $items['admin/user/rules/banlist'] = array(
    'title' => 'Ban list',
    'page callback' => 'banlist_view',
    'page arguments' => array(),
    'access callback' => 'banlist_access_callback',
    'access arguments' => array(NULL, (object)array('uid' => 1)),
    'type' => MENU_LOCAL_TASK,
    'weight'           => 10,
  );

Looks a little bit messy, only uid 1 is able to use the module, because uid = 1, any attempt from banlist_access_callback to call user_access will fail using an empty $permission argument.

After some tests, the module just shows a checkboxes fixed list with names for user rules. If you add your own rules, the module just ignores them. It makes enable/disable these rules quicker and eassier as it is done through a checkbox.

In general module requires more love, IMO.. but I'd also suggest put something about 'community' now that you mention, what about import/export features for access rules instead of just another interface?

I'd also recommend to include a readme, or some doc, there is no way to know what the module is expected to be done for things like that of uid 1..

oh, I forgot to mention, some functions are using different namespace:

I guess they are helpers, so they should be changed according to naming convention.

A note about ilo's comments (which are correct):

1. There is a Drupal function to generate the placeholders for SQL queries like that. The function is only present on Drupal 6 or higher; if this module is for Drupal 5, then it cannot use that function.
2. Modules should never terminate their code with ?>. This is explained in coding standards, which should be read.

The function is db_placeholders().

1. function banlist_access_callback($permission, $account = NULL) {
     global $user;
   
     if ($account === NULL)
       $account = $user;
   
     return ($account->uid == $user->uid && user_access($permission));
   }
   

   It should be enough to use if (!isset($account)) {.
   Also, that is not the style suggested by the coding standards.

2. /**
    *  Menu callback
    */
   function banlist_view()
   {
     drupal_set_title('Ban List');
     return drupal_get_form('banlist_form');
   }
   

   WHy isn't the code using directly banlist_form() in the menu callback definition?

3. /**
    * Create an array of masks
    *
    * @param array $masks
    * The array of masks to create.
    *
    * @param string $type
    * The type of mask to create for each value of $masks.
    *
    */
   

   Comments should have a period, at the end of the sentence.

4. The code should override the form used by the Drupal core module, rather than creating a different setting page at a different URL.

5.   $count = db_result(db_query("SELECT COUNT(DISTINCT mask) FROM {access} WHERE type = '%s' AND mask IN (". $placeholders .")", $args));
   

   For queries that select distinct elements there is a Drupal function that builds the query, and which should be used.
   I already reported about the placeholders.

6.   while( ($row = db_fetch_object($r)) != NULL) {
       unset($masks[ $row->mask ]);
     }
   

   The correct code is

     while($row = db_fetch_object($r)) {
       unset($masks[$row->mask]);
     }
   

7. See the Drupal coding standards to understand how a module code should be written. The code is still not following them.

The function is db_distinct_field().
The file LICENSE.txt needs to be removed; Drupal.org CVS doesn't allow to commit that file.

t'd probably require one more field in the table.

The code could add a field more in the table used; that would not create any compatibility problems; just don't give to the field a name already used for the same table in Drupal 7.

  if (!isset($account))
    $account = $user;

  return ($account->uid == $user->uid && user_access($permission));

The code can be optimized; if the account to check is not passed, then the $account->uid == $user->uid is verified.
The code could be better written as

  if (!isset($account)) {
    return user_access($permission);
  }

  return ($account->uid == $user->uid && user_access($permission));

As pointed out from sun, there is already at least one module with the same purpose.

ok, I now went through 20 more search result pages with the most precise search phrase I could think of and I couldn't find the module I found some months ago, so let's proceed here, and re-revert this decision.

Sorry for the annoyance, but most often, it's better for all of us to prevent duplicate efforts.

I still think there is a duplicate, until the module doesn't automatically update the list of usernames to ban.

BTW, the D7 version is http://drupal.org/project/user_restrictions since it was removed from core.

I am giving credits to the users who reviewed these applications. I apologize for bumping this issue.