Enhance drupal_attributes() for multiple valued values
moshe weitzman - June 17, 2009 - 02:57
Description
Studio theme has an enhanced version of drupal_attributes which can take an array of values and turn that into a space delimited value. The usual use case is the class attribute which separates multiple values with a space. This mutliple valued value is a common pattern. See theme_item_list() which also accepts an array as value - for nested lists.
This is backwards compatible and has no performance implications so it should be pretty non controversial.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| attributes.patch | 1.26 KB | Idle | Failed: Failed to apply patch. | View details |
#1
This probably goes without saying, but +1 ;)
#2
I'm not opposed to this patch, per se, since it just modified how
drupal_attributes()functions. However, I am opposed to using this function in any of core's tpl files.See #306358: Add $classes to templates with new theme_process_hook functions (comments #65-76) for the full discussion, but the short answer is… tpl files are a huge win for Drupal theming because they make it easy (once you find the damn thing) to modify a bunch of the markup. Hiding the class and id attributes inside a
<div <?php print drupal_attributes($attributes); ?>>means we add an additional barrier for non-PHP-savvy themers when all they want to do is add a class to a div. -1#3
Re #2, wouldn't the answer logically be providing improved access and usability to attributes handling rather than sticking with the kludgey solution of multiplying .tpl files just to add classes and/or IDs? Leave the .tpl files for structure?
#4
I think it goes without saying that I am in support of this patch. Being able to store classes in an array is beneficial.
I won't argue the theme implementation of this as it is most likely outside the scope of this specific patch. To even come close for this to be used for themes as I do in Studio, we will need a better, more consistent structure. We would need something similar to Jacine's Skinr module would also have to be a must to satisfy the "barrier to entry" argument.
What I am saying is that we will need a much more intensive patch and I am sure that the conversation will go on for miles to make it happen. It is not worth arguing for as simple and autonomous of a change as this.
#5
Absolutely. This patch gives the opportunity for so much more. It is quite simple. While it does not change anything apart of core just yet, it starts the process of a more united attribute system. There are really no side effects of adding this functionality. I just adjusted the code comment spacing slightly to match coding standards of wrapping lines.
To address JohnAlbin in #2, how this added functionality is implemented into template files will obviously be discussed in great detail later, however even this simple addition makes
drupal_attributes()just a bit more powerful. But that should be addressed at a later time. There are possibilities as discussed at D4D Boston this past weekend such as<div class="<?php print $attributes['class'] ?>">which allows for non php savvy themers to still add classes to the theme without hindering those who want more control of attributes. That idea would expand upon #306358: Add $classes to templates with new theme_process_hook functions, adding a huge amount of potential without making it more difficult to simply add an attribute into the tpl itself. The other option of course is to add a UI for adding attributes to elements as Skinr does, but none of this discussion should stop this match from moving forward. This obviously provides for a lot of potential, without really changing how core is working presently. Huge +1.#6
Thanks Ultimateboy. Since we've not heard any dissent on this, lets move to RTBC.
#7
Thanks Ultimateboy. Since we've not heard any dissent on this patch specifically, lets move to RTBC.
#8
Related issue, enabled by this patch: #326539: Convert 'class' attribute to use an array, not a string
#9
In d7 there are 2 new variables, $classes and $classes_array. You can modify the $classes_array in THEME_preprocess_HOOK function. That array gets flattened during template_process, which runs after all preprocess functions, to a string in $classes. You can modify the $classes string in a THEME_preprocess_HOOK function.
@laura_s, so in D7 you already don't need to mulitply tpl files just to change classes. :-)
@ultimateboy, “adding a huge amount of potential”. How so? I've been working on #306358: Add $classes to templates with new theme_process_hook functions for so long, I don't see the potential with this patch that you think is obvious. Wouldn't adding an $id variable to all templates give the same results, but be simpler and more consistent with the existing $classes variable? [ note, my failure to see the "obvious" might be related to the fact that its 3:30am here. :-p ] Perhaps this potential is more related to forms then to tpls?
@Damien, “enabled by this patch”. Meaning those proposed drupal_render changes would require this change to drupal_attributes? That issue hasn't moved in 7 months, so I don't see that as a compelling reason unless someone gets that issue moving again. I do like the idea of that patch, though.
I would like to see just a touch more discussion before RTBC.
#10
OK, lets stop discussing future patches in this one.
@JohnAlbin - you downgraded this patch but didn't specify what we should discuss that actually pertains to the patch. Please specify what you mean, or move back to RTBC. You make some great points, but lets save them for the next issue. I'll be sure to invite you :).
#11
Maybe an example is best. JohnAlbin in #9, you are not quite right in saying that "perhaps this potential is more related to forms then to tpls" is somewhat correct. The power comes almost anytime multiple classes are added and then passed to
drupal_attributes().The individual changes will obviously come later which is why they are not included in the initial patch, but as a quick example of how this improves the code, here is an example of an updated theme_links: http://drupalbin.com/9968. Notice how the string concatenation that was going on to add classes is now replaced with an array of classes which is then handed down todrupal_attributes().#12
I see a check_plain() taken out but not put back in.
#13
After talking to catch in IRC, I am adding back in the check_plain. Ideally, attributes should be passed in sanitized so that we dont have to check_plain for every attribute output, but that should be done in another patch.
#14
@Moshe I just wanted to make sure this patch’s potential wasn't just duplicating functionality that was already in D7. ultimateboy's example in #11 clears up my concern; this patch would be useful in theme functions, for example. I had "tpl blinders" on before. :-D
Patch looks good.
#15
There is some valuable discussion here that I do not see reflected in the PHPDoc of drupal_attributes. Let's expand that out to capture the "Ah-Ha" moment that helped John to realize the power of this, because I'm still sitting here kind of in the dark.
Additionally...
+ * An associative array of HTML attributes. If an attribute value is itself+ * an array, then its values are imploded using a space as delimiter (e.g.
+ * a multiple value <em>class</em> attribute).
We don't embed formatting HTML in Doxygen as far as I know, so let's ditch the em tags. Also, I think the additions here rightly belong in the @return section (describing what the output will be) or else inline comments in the function, since they're not (afaik) instructions to the calling function.
Basically, please re-work the docs a bit to help other themers to understand why this is so awesome, and make proper use of it in their themes.
Tests would also really help with documenting the usage of this function. There are tons of us in #drupal who can help you with this. It's not necessarily a requirement for committing this, but it would surely help make it easier. This way, the expected behaviour is written down in code, so we're not likely to break it again.
#16
I re-did the docblock and added some simpletests.
I'm also taking this opportunity to streamline drupal_attributes a bit more. We don't need to check if $attributes is empty or not an array. If someone is calling drupal_attributes with a string, that's a bug. And if the param is an empty array, then the foreach loop will simply be skipped. Also, not returning _anything_ (void return) if the attributes param is empty seems ungood to me, so it now returns an empty string in that case (the @return documentation claimed we were always returning a string, btw.)
Oh, and I did find a bug in theme_image() as it was passing NULL to drupal_attributes(), so that is fixed too. Let's see if the bot picks up anymore bugs.
#17
The last submitted patch failed testing.
#18
Fixed broken tests.
To make this easier on reviewers, this patch does 4 simple things:
'class' => array('first', 'last')will becomeclass="first last"#19
I like the additions added by JohnAlbin and the tests look good. I'd say this is good to go.
#20
Committed to CVS HEAD. Thanks.
#21
Ooh let's expand this further. :)
- Adds typehinting to the function so it truly errors if NULL is passed.
- Refactors the function to just plain make it prettier.
#22
#23
why &$data instead of $data?
#24
Because we alter that variable and existing array so we can just do implode(' ', $attributes) at the end instead of creating another variable. It's minor at best.
#25
The last submitted patch failed testing.
#26
Suppose it would have been good to actually test the followup patch.
#27
Excellent, minor change.
The re-write is much more elegant.
I'd say this could be committed... minor changes...
#28
The last submitted patch failed testing.
#29
bad trial run of new testing bot
#30
RTBC + 1 on the current patch.
Also, regarding the already-committed patch, 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.
I've got a half-implemented patch that starts on this conversion which I'll post after the one in #26 goes in.
#31
I just moved my follow-up for this issue to #326539: Convert 'class' attribute to use an array, not a string. The patch in #26 is still RTBC. :-)
#32
Committed #26 to CVS HEAD. Has this been documented yet in the upgrade instructions? If so, please mark this fixed. If not, document it before marking this fixed. Thanks!
#33
This particular issue does not require any upgrade instructions as it doesnt alter any functionality.. #326539: Convert 'class' attribute to use an array, not a string will simply be a change in coding standards. Marking as fixed. Docs change will be associated with the classes to array change.
#34
Automatically closed -- issue fixed for 2 weeks with no activity.