Convert 'class' attribute to use an array, not a string

Here is a started patch.

I do not like this patch, it affects an important function in a rather arbitrary manner. Why implode with a space...?

Whoa, this is huge... Could you make it use both a string or an array to save on making changes everywhere?

Cha0s, what are your thoughts on something like this....

well, theme table comes to my mind as something that supports different data types. but then it's obviously obscenely complex.,, so I am on the fence alas whether i like or not this. It should be noted despite noone said in the isseu the patch actually reacts to my earlier comment and only suppprts arrays for classes so other attributes wont be imploded by a space (or anythign else).

Well.. the above patch is not even close to applying and now that #493746: Enhance drupal_attributes() for multiple valued values went in, I think it's time to bring this back onto the table. I kinda started from scratch with this patch.

After talking with JohnAlbin in IRC and seeing his post http://drupal.org/node/493746#comment-1858392 I think everything covered by this patch will be consumed by the forthcoming patch that JohnAlbin is working on. There's really no need for this issue any more.

Heh. Actually, I think it might be quicker to do this issue as a follow up to #493746: Enhance drupal_attributes() for multiple valued values. But let me paste my previous comment here:

Regarding the already-committed change to drupal_attributes() in #493746, it isn't going to help much unless we actually go through all of Drupal’s code and make all 'class' implementations into arrays.

Otherwise, at any given point, we won't know if 'class' is an array or a string. And we will just end up changing this awkward code:

    if (isset($options['attributes']['class'])) {
      $options['attributes']['class'] .= ' active';
    }
    else {
      $options['attributes']['class'] = 'active';
    }

with this awkward code:

    if (!isset($options['attributes']['class']) || is_array($options['attributes']['class'])) {
      $options['attributes']['class'][] = 'active';
    }
    else {
      $options['attributes']['class'] = array($options['attributes']['class'], 'active');
    }

Which, you may notice, is actually longer then the previous awkward code.

Here's my half-implemented patch merged with ultimateboy's patch. Its still not yet complete. I searched through D7's code base for 'class' and only got as far as changing stuff through profile.admin.inc. So there's more work to do.

I finished converting all the 'class' bits I could see. Let's see what the testbot has to say.

[edit: 38 fails, 22956 exceptions. Woot! ;-) ]

Another try.

[edit: 3 fails, 110 exceptions. Getting closer.]

Another try. Also re-rolled after the comment rendering patch went in.

Whoops. parse error after re-roll from missing ).

Success! Ok, who wants to review the 100k patch? :-D

It’s 90% wrapping strings in array()

Dont have time to wrap these changes into a patch.. but a quick review gives me the following. After reading every line of this patch, all I've got are a few code comment issues.. fantastic.

// This
- * $form['my_elements'][$region][$delta]['weight']['#attributes']['class'] = "my-elements-weight my-elements-weight-" . $region;
+ * $form['my_elements'][$region][$delta]['weight']['#attributes']['class'] = array('my-elements-weight my-elements-weight-' . $region);

// Should be
- * $form['my_elements'][$region][$delta]['weight']['#attributes']['class'] = "my-elements-weight my-elements-weight-" . $region;
+ * $form['my_elements'][$region][$delta]['weight']['#attributes']['class'] = array('my-elements-weight', 'my-elements-weight-' . $region);

Given how large this patch is, its probably best to get a few more eyes on it, but from my point of view, it is good to go.

[edit] I simply read the patch, didnt actually test it out. I have no idea if every instance of "class" was covered, but everything that is changed by this patch is good.

Re-rolled with ultimateboy's comment change.

Re-rolled.

Re-roll

Please review while its still fresh! :-D

I re-looked over this and am happy.. but I really want another pair of eyes on this many changes.

I am subscribing to this.

Went through and found one...............

+++ includes/form.inc	6 Aug 2009 06:42:04 -0000
@@ -2440,13 +2434,7 @@ function theme_button($element) {
 function theme_image_button($element) {
-  // Make sure not to overwrite classes.
-  if (isset($element['#attributes']['class'])) {
-    $element['#attributes']['class'] = 'form-' . $element['#button_type'] . ' ' . $element['#attributes']['class'];
-  }
-  else {
-    $element['#attributes']['class'] = 'form-' . $element['#button_type'];
-  }
+  $element['#attributes']['class'] = 'form-' . $element['#button_type'];
 
   return '<input type="image" name="' . $element['#name'] . '" ' .

Should that be $element['#attributes']['class'][] = 'form-' . $element['#button_type']; ?

Beer-o-mania starts in 12 days! Don't drink and patch.

All I ever wanted.

Some hunks failed. Incorporated RobLoach's finding.

I'm afraid Field UI just added a couple more...

Additionally accounting for new code, especially the new code that has no tests yet (field_ui).

There was an extra end parameter in field_ui.admin.inc..........

+++ modules/field_ui/field_ui.admin.inc	20 Aug 2009 01:29:58 -0000
@@ -22,7 +22,7 @@ function field_ui_fields_list() {
       $rows[$field_name]['data'][0] = $field['locked'] ? t('@field_name (Locked)', array('@field_name' => $field_name)) : $field_name;
       $rows[$field_name]['data'][1] = t($field_types[$field['type']]['label']);
       $rows[$field_name]['data'][2][] = l($bundles[$bundle]['label'], $admin_path . '/fields');
-      $rows[$field_name]['class'] = $field['locked'] ? 'menu-disabled' : '';
+      $rows[$field_name]['class'] = $field['locked'] ? array('menu-disabled') : array(''));
     }
   }

Errr, here!

I think this is a good change, and would like to commit it. Is it possible to get one more review/sanity check?

Re-rolled for new code.

And did another sanity check.

Needs another re-roll. :(

Re-rolled for new code + additional sanity check.

Ok! Testing bot comes back green. This is a great improvement for themers. Committed to HEAD.

Please document this in the theme upgrade guide http://drupal.org/update/theme/6/7

This is really module update and not theme. Themes cannot really take too much advantage of this at this time. This is more of a coding standards change in modules, so I updated: http://drupal.org/update/modules/6/7#class_attribute_array

Tag

Oops, my mistake. Thanks, ultimateboy!