If you are using an array for your list items, and specify "class" as an attribute, with a string as the value, which is the expected behavior, theme_item_list assigns your passed string to the 'class' attribute, recasting it as a string, rather than adding it to the class array.

Here's some code to produce this error:

$list[] = array('data' => 'Item one',
				'class' => 'first-item');
$list[] = array('data' => 'Item two',
				'class' => 'second-item');
theme('item_list', array('items' => $list));

This throws the following error:
Fatal error: [] operator not supported for strings in [...]/includes/theme.inc on line 1840

There is an inelegant workaround for this:

$list[] = array('data' => 'Item two',
				'class' => array('second-item'));

Attached is a patch that treats class separately, and checks that it's not an array already, so the above workaround would still work.

Files: 
CommentFileSizeAuthor
#16 theme-attributes-doc-1135020-16.patch4.53 KBNROTC_Webmaster
PASSED: [[SimpleTest]]: [MySQL] 35,071 pass(es). View
#13 theme-attributes-doc-1135020-13.patch4.91 KBNROTC_Webmaster
PASSED: [[SimpleTest]]: [MySQL] 35,064 pass(es). View
theme_item_list-class_attributes.patch471 byteskevee
PASSED: [[SimpleTest]]: [MySQL] 31,672 pass(es). View

Comments

kevee’s picture

Status: Active » Needs review

I'm noting that this is the same in FAPI as well, so this is perhaps by design. If so, I suggest changing this to a documentation issue so that the theme_item_list page can be updated.

bfroehle’s picture

Status: Needs review » Needs work

Since one can assign multiple classes to a single item, this is by design. As you mention, the Form API also uses the same syntax:
$form['#attributes'] = array('class' => array('search-form'));

kevee’s picture

Component: theme system » documentation

Changing this issue to documentation per the discussion above. Suggest changing the first list item under the 'items' variable to:

items: An array of items to be displayed in the list. If an item is a string, then it is used as is. If an item is an array, then the "data" element of the array is used as the contents of the list item. If an item is an array with a "children" element, those children are displayed in a nested list. All other elements are treated as attributes of the list item element. The class attribute should be passed as an array of class names, rather than a string.

kevee’s picture

Title: theme_item_list incorrectly assigns class attributes » Documentation problem for D7 version of theme_item_list
jhodgdon’s picture

Title: Documentation problem for D7 version of theme_item_list » theme_item_list() needs a note about attributes arrays
Version: 7.x-dev » 8.x-dev
Status: Needs work » Active

In d7, all attributes are arrays, not just 'class'. I don't think we should be singling out class in the doc. Rather we should point out the drupal_attributes() function, which is used to format the attributes.

Needs patch in d8 first, then backport to d7.

Cottser’s picture

Subscribe - documentation needs some love. IMO we should be able to use both 'class' => 'single-class' as well as 'class' => array('single-class') but that's just me.

jhodgdon’s picture

RE #6 - that change was made in Drupal 7 after much discussion... So this issue is about updating the documentation so that it makes the functions clearer.

Cottser’s picture

Yup, understood that much. Sorry, probably not the right place to vent :)

NROTC_Webmaster’s picture

Between the change already noted and #1317628: Clean up API docs for includes directory, files starting with N-Z can this issue be closed?

(corrected the issue number)

jhodgdon’s picture

The suggested change in #5 is to point out that attributes are formatted with drupal_attributes(), which hasn't been done, and I don't see it in that other patch either?

NROTC_Webmaster’s picture

I'm trying to figure out the best way to handle this.
Seven functions have attributes in the $variables array with most of them making a note that they are an array of some form. The obvious exception is theme_item_list(). Should I a) add @see drupal_attributes() to all of the docblocks or b) just reference it within the description for the attributes of theme_item_list which leaves the possibility that someone else will run into a similar problem.

Line 1568: * - attributes: (optional) An associative array of HTML attributes to apply
function theme_datetime($variables) {
Line 1664: * - attributes: (optional) Attributes for the anchor, or for the tag
Line 1669: * - attributes: A keyed array of attributes for the UL containing the
function theme_links($variables) {
Line 1789: * - attributes: Associative array of attributes to be placed in the img tag.
function theme_image($variables) {
Line 1863: * - attributes: An array of HTML attributes to apply to the table tag.
function theme_table($variables) {
Line 2087: * - attributes: The attributes applied to the list element.
function theme_item_list($variables) {

Line 2200: * - #attributes: (optional) An array of HTML attributes to apply to the
function theme_html_tag($variables) {
Line 2277: * - attributes: Associative array of attributes to be placed in the meter
function theme_meter($variables) {

jhodgdon’s picture

Hmmm... Maybe they should all say:

An associative array of HTML attributes [specifics here, such as "for the anchor tag" etc.]. Attributes are formatted using drupal_attributes().

How's that for an idea?

NROTC_Webmaster’s picture

Status: Active » Needs review
FileSize
4.91 KB
PASSED: [[SimpleTest]]: [MySQL] 35,064 pass(es). View

Take a look at this and let me know what you think.

jhodgdon’s picture

Looks good! The only thing I would change is that since we are referencing drupal_attributes(), I think we can take out sentences like this:

If element 'class' is included, it must be an array of one or more class names.

And is that really the complete list of theme functions that use attributes?
http://api.drupal.org/api/drupal/core!includes!common.inc/function/calls...
Wow... looks like there are quite a few... maybe we should reconsider whether we really need to add this sentence to all of them. How necessary is it? Is it common knowledge?

jhodgdon’s picture

Status: Needs review » Needs work
NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
4.53 KB
PASSED: [[SimpleTest]]: [MySQL] 35,071 pass(es). View

jhodgdon,

Slightly updated patch. In regards to the common knowledge I'm really not sure if it is or not. That seems to be the whole reason this issue was opened so I would lean towards saying no.

jhodgdon’s picture

Status: Needs review » Needs work

Agreed it is probably needed.. but if so, the patch probably needs to expand to encompass all or most of the 48 functions that call drupal_attributes.

And maybe we should change the wording to "Attributes are passed to drupal_attributes() for formatting." just to make it one bit clearer? I'm OK with either way, but I think this wording might make it abundantly clear that you need to supply attributes that that function can format properly.

As one more note, the whole thing of class needing to be an array and not a string is incorrect for drupal_attributes() itself. Some of the functions, however, add their own items to class, such as:
$attributes['class'][] = 'whatever'
So actually just saying that it is formatted with drupal_attributes() may not be sufficient... sigh.

NROTC_Webmaster’s picture

:(

I'll take another stab at it, but if we are adding something to all 48 calls should we just add @see?

I think the documentation for it does a good job of explaining how to use it.

My only hesitation with adding the @see is that the actual use might be lost with all of the other variables listed for the functions.

jhodgdon’s picture

We need to be consistent. Either we put it in all 48 (or at least those of the 48 where it's appropriate -- maybe not quite all), or we put it in none of them. I would actually vote for none.

Putting in @see is not going to help IMO, because the See Also section will be far away from the place where people aren't realizing they need to make the individual attributes arrays, and also as I noted above, actually drupal_attributes() doesn't care whether class is an array or a single value, it's just the individual functions that care because they're adding more elements to class.

So... with all this in mind, I am really inclined to call this a "won't fix".

jhodgdon’s picture

Title: theme_item_list() needs a note about attributes arrays » Need standard way to document the format of attributes in theme functions
jenlampton’s picture

Category: bug » task
Issue tags: +coding standards, +Twig, +theme system cleanup

Tagging this one since we'll need to use a consistent way to document all our attributes in Twig templates too :)

joelpittet’s picture

Does this need to be done in D8?

There is no longer drupal_attributes(), it's now the Attribute class. Can this be bumped back down to D7?

Arlina’s picture

Issue summary: View changes

This should be a D7 issue now. The docs are not clear about how to add a class to a <li> item, it just says "All other elements (besides "data" and "children") are treated as attributes of the list item element." This misleads to trying to add a class element as a string (which produces the fatal error), specially since any other element specified as as a string is correctly added as an attribute to the <li>.

joelpittet’s picture

Version: 8.x-dev » 7.x-dev

Dropping back down to D7. D8 we are using Attribute class and doing things like

  • and not passing classes or using drupal_attributes()