Skip to:
Content

BuddyPress.org

Opened 4 years ago

Closed 4 years ago

#6679 closed enhancement (fixed)

Allow bp_field_css_class() to accept more than one class name.

Reported by: dcavins Owned by: dcavins
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)

6679.01.patch (1.1 KB) - added by dcavins 4 years ago.
6679.02.patch (4.5 KB) - added by dcavins 4 years ago.
Use regex and sanitize_html_class to filter input.

Download all attachments as: .zip

Change History (13)

@dcavins
4 years ago

#1 @sbrajesh
4 years ago

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.

#2 @sbrajesh
4 years ago

  • Cc brajesh@… added

#3 @hnla
4 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 @dcavins
4 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.

#5 @DJPaul
4 years ago

  • Milestone changed from Awaiting Review to 2.5

#6 @DJPaul
4 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 @sbrajesh
4 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 @dcavins
4 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 cause explode() 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&lt;
sanitize_html_class( '%class$name<' ) yields classname which seems better.

Thanks again for the comments.

-David

#9 @DJPaul
4 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.

@dcavins
4 years ago

Use regex and sanitize_html_class to filter input.

#10 @dcavins
4 years ago

Thanks for the reminder DJPaul. I can just imagine Ray having to hunt down a failing test at some later date because I polluted the global.

#11 @dcavins
4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 10460:

Allow bp_field_css_class() to accept multiple classes.

Allow bp_field_css_class() to accept more than one class
name passed as a space-separated string or array.

Fixes #6679.

Note: See TracTickets for help on using tickets.