Problems with homebox_pages(), hook_menu(), ...css_classes_for_block() [#795868]

/**
 * Retrieve an array of pages matching specified conditions.
 *
 * @param $membersonly
 *   Set this to TRUE to exclude the 'anonymous' role.
 * @param $permission
 *   A string containing a permission. If set, only roles containing that
 *   permission are returned.
 *
 * @return
 *   An associative array with the role id as the key and the role name as
 *   value.
 */
function homebox_pages() {
  $result = db_query('SELECT * FROM {homebox_pages} ORDER BY name');

  while ($page = db_fetch_object($result)) {
    $filter = '![^abcdefghijklmnopqrstuvwxyz0-9-_]+!s';
    $page->safe_name = preg_replace($filter, '-', drupal_strtolower($page->name));
    $pages[] = $page;
  }

  return empty($pages) ? NULL : $pages;
}

Those @param's aren't there and they sound pretty important. What happened?

====

/**
 * Implementation of hook_menu().
 */
function homebox_menu() {
  $items = array();

  // User pages
  if (is_array(homebox_pages()) && count(homebox_pages()) > 0) {
    foreach (homebox_pages() as $page) {
      $items['homebox/'. $page->pid] = array(
        'title' => $page->name,
        'page callback' => 'homebox_build',
        'page arguments' => array(1),
        'access arguments' => array('access homebox '. $page->safe_name),
        'type' => MENU_NORMAL_ITEM,
      );
    }
  }

You're calling homebox_pages() three times when it should only be called once and stored.

Comments

Comment #1

mstef CreditAttribution: mstef commented 11 May 2010 at 13:46

WARNING: Unfiltered and unchecked URL argument placed into an SQL query

// User pages
  if (is_array(homebox_pages()) && count(homebox_pages()) > 0) {
    foreach (homebox_pages() as $page) {
      $items['homebox/'. $page->pid] = array(
        'title' => $page->name,
        'page callback' => 'homebox_build',
        'page arguments' => array(1),  //** <----HERE!! *******///
        'access arguments' => array('access homebox '. $page->safe_name),
        'type' => MENU_NORMAL_ITEM,
      );
    }
  }

function homebox_build($pid) {

  // If no default block layout is set return a Drupal 404
  if (db_result(db_query("SELECT COUNT(*) FROM {homebox_default} WHERE pid = %d", $pid)) == 0) {
    drupal_not_found();
    exit();
  }

I know you're checking for a %d but it's better practice to check with is_numeric() first. This also avoids SQL errors.

Comment #2

mstef CreditAttribution: mstef commented 12 May 2010 at 19:20

Title:

Problems with homebox_pages() and hook_menu()

» Problems with homebox_pages(), hook_menu(), ...css_classes_for_block()

$movable is missing a match/check for the $pid

function _homebox_get_css_classes_for_block($block, $pid) {
  $classes = array('homebox-portlet');
  // Is the block movable?
  $movable = (bool) db_result(db_query("SELECT movable FROM {homebox_default} WHERE bid = %d", $block['bid']));