Improper loading of node in admin screen prevents call to node_access()

I got an E_NOTICE today (you probably want to have that feature turn ON in D6 as well!)

Looking into it, I found the cause... the fact is that you load nodes with a SELECT statement. That won't work correct in 99% of the cases you have to handle.

The following is your original code. As you can see, you are building an array of nodes from the SELECT statement. At least, there is an SQL rewrite, so it is most certainly secure.

/**
 * Menu callback for admin/content/webform. Displays all webforms on the site.
 */
function webform_admin_content() {
  $result = db_query(db_rewrite_sql("SELECT n.* FROM {node} n WHERE n.type = 'webform'"));
  $nodes = array();
  while ($node = db_fetch_object($result)) {
    $nodes[] = $node;
  }

  return theme('webform_admin_content', $nodes);
}

What you want to do is something like this instead:

/**
 * Menu callback for admin/content/webform. Displays all webforms on the site.
 */
function webform_admin_content() {
  $result = db_query(db_rewrite_sql("SELECT n.nid FROM {node} n WHERE n.type = 'webform'"));
  $nodes = array();
  while ($n = db_fetch_array($result)) {
    $nodes[] = load_node(array('nid' => $n['nid']));
  }

  return theme('webform_admin_content', $nodes);
}

Otherwise, the node_access() call in the theming function (and in case someone overwrites the theme...) does not do what you expect it to do (since the format is not defined from your SELECT call.) It is still secure, since you generate an Edit link which will send you on a page that does not work and that's okay, but it would be better to do as intended and hide the link when not available.

In regard to the db_rewrite_sql() call, don't you need to add 'nid' as the 2nd parameter? or maybe array('nid')... I'm not totally sure, but I thought you were supposed to add something there.

Thank you.
Alexis Wilke