Opened 9 years ago
Closed 9 years ago
#6679 closed enhancement (fixed)
Allow bp_field_css_class() to accept more than one class name.
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 2.5 | Priority: | normal |
Severity: | normal | Version: | 2.3.3 |
Component: | Extended Profile | Keywords: | good-first-bug has-patch |
Cc: | brajesh@…, dcavins |
Description
Currently, bp_field_css_class()
can only accept a string and will convert that string to a single class name. The attached patch maintains that behavior but also will split comma-separated strings or work with an array of class names.
Attachments (2)
Change History (13)
#3
@
9 years ago
Feel like this might be a duplicate, think I raised this notion of inability to pass multiple class strings a while back, might be worth checking tickets?
This might be a case where we ought to look at any other functions that might benefit from this.
#4
@
9 years ago
- Cc dcavins added
- Owner set to dcavins
- Status changed from new to accepted
Thanks for your comments! Looks like a similar bug was squashed here: https://buddypress.trac.wordpress.org/ticket/6069.
@sbrajesh thanks for your comments and the reference. Is there an advantage to using preg_split( '#\s+#', $class );
(as WP does in the file your referenced) instead of explode( ' ', $class )
? (Are there cases I can include in my tests that would demonstrate that?)
On 6069, @imath used sanitize_html_class()
as the callback, which seems like the right choice: https://developer.wordpress.org/reference/functions/sanitize_html_class/
@hnla I agree that being on the lookout for this stuff is useful and will poke around for other similar situations.
Thanks again for the comments; I really appreciate the feedback.
#6
@
9 years ago
- Keywords needs-patch good-first-bug added; has-patch removed
The above comments are a good start, I just want to be "that guy" and say you can't use an anonymous function there because PHP 5.2-nofun.
#7
@
9 years ago
@dcavins
There are multiple reasons.
In the patch you are exploding on comma (') not on space. That was my first concern.
Even if you used space, there are issues. If you use space instead of regular expression you are expecting that there should be limiting to simple space while regexp will work whether there is one space between the class or multiple.
Another thing is if you use space, you will need to filter out your array for empty value.
Here is an example:-
<?php $test = " a b c d e"; $test = explode ( " ", $test ); var_dump($test);
See the result and you will see why it is more suitable to use regexp. Hope that clarifies.
#8
@
9 years ago
- Keywords has-patch added; needs-patch removed
I've attached a new patch that uses regex and sanitize_html_class()
to filter input. I've also added tests.
Some interesting things about using preg_split( '#\s+#', ' pumpkin spice ' )
vs explode()
:
preg_split()
gracefully handles multiple delimiters, like space-space-tab-space, that would causeexplode()
to lose its mind.- Both will generate extra array elements if the string contains leading or trailing spaces. They're empty, so I don't think we have to guard against that case.
The WP reference from Brajesh (thanks again for your comments and interest) uses esc_attr()
to sanitize strings, but it seems like sanitize_html_class()
is the better choice. For example:
esc_attr( '%class$name<' )
yields %class$name<
sanitize_html_class( '%class$name<' )
yields classname
which seems better.
Thanks again for the comments.
-David
#9
@
9 years ago
You need to reset the value of the global after the tests. See test_bp_has_activities_scope_favorites_no_items
for an example handling another one of our globals.
Adding support for comma separated string is meaningless in the context of css classes.
Here is how WordPress does it for post_class
https://github.com/WordPress/WordPress/blob/master/wp-includes/post-template.php#L427
space should be considered as the separator for multiple class and not the comma.
Also, passing the class to esc_attr will be a good idea.