Convert 'class' attribute to use an array, not a string
$form['#attributes']['class'] is currently implemented as a string, but multiple classes are allowed, and that's currently solved by cat'ing onto the end of it ($form['#attributes']['class'] .= ' classname'), but this is bad. Since class in this context is actually a list of classes, we need to convert this element into an array.
I'm by no means familiar with 99.9% of the internal workings of Drupal 'cause I haven't looked at it yet. DamZ mentioned that we could do this in a backwards-compatible manner, using is_array(), and I'm down for that... but personally with something like this, I'd prefer to have a patch that would convert them all at once. Is this kosher? It would most likely be a huge patch. I just don't like the idea of adding cruft to the code.
Also, tests will have to be updated (...or written?) to test the class element working as it should.
#1
Here is a started patch.
#2
I do not like this patch, it affects an important function in a rather arbitrary manner. Why implode with a space...?
#3
Here's a patch that leaves backwards compatibility in the dust... also I created a _drupal_attribute function to handle rendering individual attributes. That takes out the generalized "if array then implode with spaces" and targets that behavior to only the 'class' case.
#4
Whoa, this is huge... Could you make it use both a string or an array to save on making changes everywhere?
#5
Cha0s, what are your thoughts on something like this....
#6
While I'm somewhat a supporter of backwards compatibility personally, as far as I know, the philosophy in Drupal is to change something and no leave cruft around to handle old behavior. Based on that impression I get, I don't think we should be supporting strings as classes.
For one thing, how could someone interfacing with your code add to/remove from your classes without string manipulation? That's extremely dirty; the reason I propose the change to begin with.
EDIT: The best solution I can think of to reconcile that behavior would be to add some rule to the Coder module to check for this, but from my experience of actually making the changes, that's probably going to raise a fair amount of false positives.
#7
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).
#8
Got a new patch rolled.
#9
... Rolled again to remove the function call.
#10
The last submitted patch failed testing.
#11
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.
#12
The last submitted patch failed testing.
#13
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.
#14
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:
<?phpif (isset($options['attributes']['class'])) {
$options['attributes']['class'] .= ' active';
}
else {
$options['attributes']['class'] = 'active';
}
?>
with this awkward code:
<?phpif (!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.
#15
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! ;-) ]
#16
The last submitted patch failed testing.
#17
Another try.
[edit: 3 fails, 110 exceptions. Getting closer.]
#18
The last submitted patch failed testing.
#19
Another try. Also re-rolled after the comment rendering patch went in.
#20
Whoops. parse error after re-roll from missing ).
#21
Success! Ok, who wants to review the 100k patch? :-D
It’s 90% wrapping strings in
array()#22
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.
#23
Re-rolled with ultimateboy's comment change.
#24
The last submitted patch failed testing.
#25
Re-rolled.
#26
The last submitted patch failed testing.
#27
Re-roll
#28
The last submitted patch failed testing.
#29
Please review while its still fresh! :-D
#30
I re-looked over this and am happy.. but I really want another pair of eyes on this many changes.
#31
The last submitted patch failed testing.
#32
I am subscribing to this.
#33
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.
#34
All I ever wanted.
Some hunks failed. Incorporated RobLoach's finding.
#35
I'm afraid Field UI just added a couple more...
#36
Additionally accounting for new code, especially the new code that has no tests yet (field_ui).
#37
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!
#38
I think this is a good change, and would like to commit it. Is it possible to get one more review/sanity check?
#39
The last submitted patch failed testing.
#40
Re-rolled for new code.
And did another sanity check.
#41
Needs another re-roll. :(
#42
Re-rolled for new code + additional sanity check.
#43
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
#44
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
#45
Tag
#46
Oops, my mistake. Thanks, ultimateboy!
#47
Automatically closed -- issue fixed for 2 weeks with no activity.