Review X-ray code for best practices, security, readability, performance, anything you can think of!

Hi,
I reviewed the 7.x-1.0-beta1 version located here: http://drupal.org/node/1006172

A few issues:

• Coder reports several formating issues and issues with usage of t().
• In the method theme_xray_people_summary() you are assuming that $data = $variables['data']; produces an array with 2 elements. When there are no blocked users, your code raises an exception because further on in the method you are trying to use a undefined element (i.e, undefined index).
• The source of my previous point is located in the method xray_people_summary(). I believe you require a 2 row result, one for blocked users, one for un-blocked users. Your current query does not produce that result. You are relying on a logical "group by". If the group value (I.E. user.status == 0)) doesn't exist, no record is for the blocked group is produced.

  Here is one option that will produce the 2 record result:

  function xray_people_summary() {
    $data = array();
  
    $unblocked_query = db_select('users')->fields('users', array('status'));
  
    $blocked_query = clone $unblocked_query;
  
    $blocked_query->condition('uid', 0, '<>')->condition('status', 0, '=');
    $blocked_query->addExpression('COUNT(uid)', 'num');
  
    $unblocked_query->condition('uid', 0, '<>')->condition('status', 1, '=');
    $unblocked_query->addExpression('COUNT(uid)', 'num');
  
    $blocked_query->union($unblocked_query, 'DISTINCT');
    $result = $blocked_query->execute();
  
    foreach ($result as $record) {
      switch ($record->status) {
        case 0 :
          $data['users'][0] = $record->num;
          break;
        case 1 :
          $data['users'][1] = $record->num;
          break;
      }
    }
  
    // @TODO roles, permissions
    return $data;
  }
  

  I a am sure there are other ways to obtain the same result. This was just my first thought (i.e. use an SQL union).

• This is a "take it or leave it" suggestion.

  Please consider replacing the use of the method debug().

  In many shops, triggering an exception for expected functionality (I.E. rendering report data) is deemed unacceptable. Thus, why not avoid the controversy and implement a simple method which reports the data, yet avoids triggering an exception.

  For example a simple replacement functin:

  function display_data_structure($data, $label = NULL, $print_r = FALSE) {
  
    $string = check_plain($print_r ? print_r($data, TRUE) : var_export($data, TRUE));
    $string = '<pre>' . $string . '</pre>';
  
    return $string;
  }
  

  Usage:

    drupal_set_message(display_data_structure($conf),'status');
  

Hope that helps.
Lorin