Clean up theme_item_list() and make it a simpler theme function

Sorry, no-one is going to review this unless I mark it CNR :-)

Good idea. A couple suggestions though:

• some of the string manipulations with $output are a little twisted and make the code hard to read.
• Having both theme_list_item() and theme_item_list() is confusing. I'd rather have a weird name like theme_item_list_item() than be confused about which function I shoud use.
• Shouldn't theme_list_item() also be declared in the theme registry?

Thanks for reviewing this.

1. I assume you are referring to theme_item_list(). I’ll admit it’s not the standard $output .= '...';, but twisted? One line is out of order, but are you referring to this, or too many . ’s, or embedding $output in the RHS?
   I think it is good practice to add opening and closing tags on the same line, building outwards in a co-ordinated way. Would double quoted stings make it more readable?;-
   function theme_item_list($items = array(), $title = NULL, $type = 'ul', $attributes = NULL) {
  $attrs  = drupal_attributes($attributes);
  $items_ = _theme_item_list($items);
  $output = "<$type $attrs>$items_</$type>";
  if (isset($title)) {
    $output = "<h3>$title</h3>$output";
  }
  $output = "<div class=\"item-list\">$output</div>";
 
  return $output;
}
   or a template file?
2. Yes, I can see what you mean: Will use your suggestion.
3. I’m very hazy on this: I can cope with modules, but, in core, what should the hook in hook_theme() be? (assuming use of hook_theme()). I can’t find any examples in core! Does it not work for you because of this?

Regarding point 1, I see what you're doing now: building the HTML from the inside rather than from the beginning. Even though it's not really standard, it does makes sense. Maybe a little comment would make it clearer.

For point 3, theme functions from .inc files (like theme_item_list()) are all defined in drupal_common_theme() inside of common.inc.

About #3.1: in that case single-quoting is better than double quoting in that case.

About the string order, it is more conventional in Drupal (and arguably easier to read), to build the output in the text flow order. Something like that will do:

By the way: there was a bug in your patch: the <h3> should go inside the <div> but outside the <ul>/<ol>.

1. Used (mainly) single-quoted strings and added a comment. Building from the inside makes it easier to modify: If you change a <div> the </div> is right there, just begging to be kept in sync. Each line stands up for itself (once I get it right :-( ). Thanks for the catch Damien, I had put this in #3-1. but didn’t make a new patch.
3. Thanks: couldn’t find this at all. added entry for theme_item_list_item.

Just as a question, is there some legacy reason why we call it an "item_list" rather than just a "list"? I can't think of any definition of a "list" that does not consist of "items". After all, we don't call it "theme_cell_table" either.

This is particularly weird with the name "theme_item_list_item".

Perhaps an opportunity to clean up the API a bit more?

Also, patch no longer applies.