Various bugs in theme_item_list()

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?

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:

function theme_item_list($items = array(), $title = NULL, $type = 'ul', $attributes = NULL) {
  $output  = "";
  $output .= '<div class="item-list">';
  if (isset($title)) {
    $output .= "<h3>$title</h3>";
  }
  $output .= "<$type" . drupal_attributes($attributes) . '>' . _theme_item_list($items) . "</$type>";
  $output .= '</div>';
  return $output;
}

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

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.

Agree that theme_list seems better then theme_item_list.

But pushing to D8.

Similar to #98696-15: Various bugs in theme_links(), I think we can resolve this issue easily.

Although it's true that theme_item_list() is relatively complex, the reason for that is its current programming logic only; mainly the inner foreach to handle nested child lists.

Simplifying that inner code automatically resolves the overall issue. (I recommend to hit the "Hide deletions" button of Dreditor)

It could only further simplified by adding a preprocess function for theme_item_list(), but I don't really like that idea.

Sorry.

Any feedback? I really think that this is a major improvement compared to the current theme function body.

subscribing.

Overall, I find the patch in #12 makes this a lot easier to read and understand. It's a really nice cleanup and I don't see any reason why it should have to wait until D8 (though theme_list() would be nice in D8).

The current version in core is pretty confusing. For example, this used to trip me up a lot when trying to adding zebra striping classes using the same, which is usually the sole reason I need to go in and override it:

if ($i == 0) {
  $attributes['class'][] = 'first';
}
if ($i == $num_items - 1) {
  $attributes['class'][] = 'last';
}

This still confuses me:

foreach ($item as $key => $value) {
  if ($key == 'data') {
    $data = $value;
  }
  elseif ($key == 'children') {
    $children = $value;
  }
  else {
    $attributes[$key] = $value;
  }
}

This patch makes it MUCH better. I've tested this and think it's ready.

@Dries/webchick: I'd like to repeat that it's easier to see the resulting massive simplification of this patch by hitting the "Hide deletions" button of Dreditor. Without doing so, the changes look quite scary, but they are definitely not - it's only the resulting logic that matters.

#12: drupal.theme-item-list.12.patch queued for re-testing.

Just because someone is upset doesn't mean we have to mark something as won't fix. Someone else is more than welcome to pick up work on a ticket.

Looks like a nice TX improvement, but too late for Drupal 7.

This issue will be moved back into the D8 queue, after it has been committed to Edge 7.x.

Massively improved the theme_item_list() tests, which revealed quite some bugs in the nested child list handling, as well as empty list handling.

Just ran the tests with core's theme_item_list(), and yeah, it outputs a list container and heading even if there is no list, and the attributes handling for nested child list is utterly broken.

Hmm, when I test this with the pagers on the tracker page, I get empty h3's, and in the recent comment block, I don't. I looked at the variables and the pager title is NULL, while the recent comment block title is an empty string. :/

Further improved the tests to account for the item list title. We just found out and discussed that theme_pager() is guilty for those stale headings and that needs to be fixed in core.

Thanks :)

Tests pass and it looks good.

Moving back to core.

This was RTBC before . Why is it CNW now?

Reasonable question. That is, because theme_item_list() in core

1. outputs a stale DIV.item-list, even when there is no list.
2. outputs a heading/title for a list, even when there is no list.
3. applies HTML attributes that were intended for a list item to a child list.
4. does not support HTML attributes for a child list at all.
5. does not output so called zebra classes (i.e. "odd" and "even").
6. outputs newlines between list items, which do not allow for certain, common use-cases of themers.
7. is insufficiently tested, so all of the above bugs are not catched.

And lastly, the last patch for core did not sufficiently account for most of these bullets, so it has to be redone based on the committed patch.

Happy to clarify more, in case I missed something.

I'd be most grateful if someone would be able replace the code in core 1:1 with the code + tests in http://drupal.org/project/edge.

It's all ready and merely a matter of replacing it. Even tested on D7 production sites.

I'll take a crack at this, probably tomorrow.

Oh wow, I sure let this slide. Haven't had the time.

From Edge with love.

+ *     - ...: All other key/value pairs are used as HTML attributes for the list

This is just a tiny nitpick but I find "...:" in this comment odd. Can we change or drop it or is it just me? Other than that, this is ready and has been since it got committed to Edge months ago thanks to you. ;)

We need to keep that, because it's essential information for how to apply HTML attributes to the LI tag. I've removed the - ...: part now, HTH.

That's better, thanks. :)

#37: drupal8.theme-item-list.37.patch queued for re-testing.

OK this looks great to me but this comment made my eyes roll into the back of my head:

+          // List attributes are normally defined in the 'attributes' variable,
+          // but for nested child lists, all non-numeric keys in 'children' must
+          // be treated as child list attributes.

I think it's "nest child lists" then "child list attributes", and the fact that the sentence both begins and ends with the same two words.

Maybe 'lists with nested children', and 'attributes for the children'?

Thanks! Completely rephrased that comment, hopefully for the better. ;)

Small grammar nitpick

+++ b/includes/theme.incundefined
@@ -1807,51 +1809,69 @@ function theme_item_list($variables) {
+          // theme variable, but not inherited to children. For nested lists,

s/inherited to/inherited by

Uhm, also fixed that.

Code was already RTBC, and the new comment is much better.

Committed to 8.x.

This is a near complete refactoring of the theme function, so marking CNR for 7.x.

#43: drupal8.theme-item-list.43.patch queued for re-testing.

Personally, I think attempting to backport will be a waste of time. Same for #98696: Various bugs in theme_links().

Much smaller theme layer backports have been attempted and failed, so removing the tag and marking this fixed.