If I do

<?php
$variables
= array(
 
'items' => array(
   
'Item one',
   
'Item two',
   
'Item three',
  ),
);
print
theme('item_list', $variables);
$variables = array(
 
'items' => array(
   
'one' => 'Item one',
   
'two' => 'Item two',
   
'three' => 'Item three',
  ),
);
print
theme('item_list', $variables);
?>

The result will be:

<div class="item-list">
  <ul>
    <li class="first">Item one</li>
    <li>Item two</li>
    <li class="last">Item three</li>
  </ul>
</div>
<div class="item-list">
  <ul>
    <li class="first">Item one</li>
    <li class="first">Item two</li>
    <li class="first">Item three</li>
  </ul>
</div>

When using associative arrays, every item has the "first" class, and the "last" class is missing on the last one.

The documentation says nothing about associative arrays on the items parameter, so I think this is a bug that must be fixed. Otherwise we must fix the docs to clarify that only indexed arrays are allowed.

PS: I think this issue doesn't affect Drupal 8 due to the refactorings done at #256827: Various bugs in theme_item_list()

Files: 
CommentFileSizeAuthor
#5 0001-Issue-1809836-theme_item_list-is-broken-when-items-v.patch2.63 KBdanillonunes
PASSED: [[SimpleTest]]: [MySQL] 39,504 pass(es).
[ View ]
#3 0001-Improved-the-item-list-tests.patch1.87 KBdanillonunes
FAILED: [[SimpleTest]]: [MySQL] 39,511 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#4 0001-Issue-1809836-theme_item_list-is-broken-when-items-v.patch3.08 KBdanillonunes
PASSED: [[SimpleTest]]: [MySQL] 39,498 pass(es).
[ View ]
#1 0001-Issue-1809836-theme_item_list-is-broken-when-items-i.patch1.45 KBdanillonunes
PASSED: [[SimpleTest]]: [MySQL] 39,523 pass(es).
[ View ]

Comments

Title:theme_item_list is broken with associative arraysIssue #1809836: theme_item_list() is broken when items is a associative array
Status:Active» Needs review
StatusFileSize
new1.45 KB
PASSED: [[SimpleTest]]: [MySQL] 39,523 pass(es).
[ View ]

Didn't do the tests locally, but I hope there's no test expecting this bugged behaviour.

Title:Issue #1809836: theme_item_list() is broken when items is a associative arraytheme_item_list() is broken when "items" variable is a associative array

StatusFileSize
new1.87 KB
FAILED: [[SimpleTest]]: [MySQL] 39,511 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

I also improved the tests to cover this case. This patch has just the test change, so now it must no pass.

StatusFileSize
new3.08 KB
PASSED: [[SimpleTest]]: [MySQL] 39,498 pass(es).
[ View ]

And this patch is #1 and #3 merged, aka the patch that must be commited if everything is ok.

StatusFileSize
new2.63 KB
PASSED: [[SimpleTest]]: [MySQL] 39,504 pass(es).
[ View ]

Actually, the last test is redundant now and can be removed.

Status:Needs review» Reviewed & tested by the community

I've tested locally is works perfectly.

Next time could you please name the patch file like this: [description]-[issue-number]-[comment-number].patch? http://drupal.org/node/1054616

Well, so item 1 is the first and item *total number of items* is the last. I don't have to do a weird thing like $i = -1; at start just so I could use a weird if ($i == $num_items - 1) at the end.

Assigned:Unassigned» David_Rothstein

Hm. Not sure about this one. Assigning to David.

Version:7.x-dev» 8.x-dev
Assigned:David_Rothstein» Unassigned
Status:Reviewed & tested by the community» Patch (to be ported)

Oh, no I'm not. :) We need a 8.x patch first.

Version:8.x-dev» 7.x-dev
Status:Patch (to be ported)» Reviewed & tested by the community

@webchick I'm reverting the changes because 8.x was already fixed by #256827: Various bugs in theme_item_list() :)

Assigned:Unassigned» David_Rothstein

Ah, sorry! Missed that. David it is then. :) We don't normally change theme functions in stable releases. This particular case looks fairly harmless, but David is really good at sniffing out "harmless" from actual harmless. ;D

Title:theme_item_list() is broken when "items" variable is a associative arraytheme_item_list() is broken when "items" variable is an associative array (followup for tests)
Version:7.x-dev» 8.x-dev
Assigned:David_Rothstein» Unassigned
Category:bug» task
Status:Reviewed & tested by the community» Active
Issue tags:+7.17 release notes

Looks good to me.

However, the code in Drupal 8 behaves a bit differently from this, and as far as I can see nothing like the test additions in this patch were ever added to Drupal 8 (so this functionality doesn't actually seem to be tested there). That's not so good; I think the tests should be forward-ported.

But in the meantime, committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/ff1d0fc