Update `bp_new_group_invite_friend_list` for new $args to support full list markup

[hnla]

bp_new_group_invite_friend_list() generates a list of friends wrapped in li markup however it does not account for the required parent markup leaving that to the template and therefore additional checks to ensure hardcoded markup not displayed if list function empty.

Patch adds two args to handle before & after args string & a simple class arg to output classes on the parent element.

The args are deliberately defaulted to empty strings to ensure we don't upset existing use, they would need to be explicitly passed for any generated markup.

This relates to a recent change to Nouveau create group steps where we needed to update the template and used this function before realising that it only rendered li elements without a 'ul' construct.

[DJPaul]

1) Let's use sanitize_html_class() on the supplied classes.
2) I am wondering if and how we could use KSES to whitelist the HTML attribute we're building here. Or if we need to. Any similar code in WP core we could compare this against would be interesting.

Apart from that, idea is sound.

[hnla]

1. Will patch to.

2. looked at kses but not sure it's appropriate here? In only accepting a plain string isn't kses and it's element array overkill, I'm not sure how we use it in this specific context ( we do utilise wp_kses elsewhere in BP core iirc though).

Am in favour though of preventing daft strings being passed in and rendered e.g 'dib', 'uk' so maybe a simpler approach, create our own array of allowed strings and then do a basic in_array check whilst retaining the esc_html... maybe:

$allowed_elements = array('ul', 'div', 'p');
if ( ! empty( $r['after_list'] ) && in_array( $r['after_list'], $allowed_elements ) ) {

[DJPaul]

1. Great!

2. I don't think that's necessary. As I said, I can't remember if WordPress does a similar construction, but in its widgets, it has before_titleetc which is handled like so:

$output .= $args['before_title'] . $title . $args['after_title'];

How about having this patch's before_list and after_list treated the same - not be escaped and just accept whatever it's given? (when you call the function, you'd pass it e.g. <h3> rather than h3.) It'd give flexibility in case someone needs to do some crazy WordPress-style hacks. :)

If we did that, we'd need to remote the parent_classes attribute and rely on people passing the entire element e.g. <h3 class="yolo"> which is what the widgets code does.

(The widgets) isn't the best code in the world, by any means, but just wanted a contrasting option for your consideration. You are more familiar than I about consistency with (and if) how we do this sort of thing for other BuddyPress functions -- if we do, consistency is more important to me.

[hnla]

not be escaped and just accept whatever it's given?

rely on people passing the entire element e.g. <h3 class="yolo"> which is what the widgets code does

We certainly can do, however does this not present 'opportunities' do we not worry about what string may be passed through, if we have no particular security concerns I would be happy to just let the dev pass in what they need i.e '<ul class="invite-friends-list">'

I'll think on more about this and other BP core examples.

[DJPaul]

Any developer can pass any value to any function. We all know that. That's why we're so careful with sanitising user-supplied data, because it could be anything.

If there's no way to inject a value into a unit of code at runtime (be that a search form value, or the result of an API request, or data from an RSS feed, etc), then it's safe -- at least from this very specific perspective.

We don't need to harden BuddyPress against developers making poor choices with how they write their code (i.e. making up their own HTML elements, because that's harmless - it just wouldn't work).

[hnla]

^yep

However taking into account we can pass strings of type '<div class="">' in to various arg functions unfettered in core and also use wp_kses liberally in places I've re-factored patch along the lines you suggested:

Allow args in style of array( 'before' = > '<ul class="">')
Added a simple allowed array and filtered the before/after args on wp_kses.

We manage things somewhat, in this context there are limited options, so ensure what is passed in confirms to that narrow set.

Equally I can appreciate just leaving it wide open.

[hnla]

Update $args, add allowed array to use to filter through wp_kses, remove class arg.

[DJPaul]

The indentation for $allowed is incorrect, but that aside, I don't see much point to KSES this now we've explored this more; user-supplied data will not be directly passed to this function. Bigger chance that it annoys a developer trying to some crazy customisation, especially as $allowed element is hardcoded.

I know I suggested looking at KSES originally and you weren't sure :) but please remember this is as much code review as it is a collaborative technical discussion, and that I can talk rubbish with the best of them. :)

I think the couple of new arguments and the change to $retval is all we need!

[hnla]

Agreed, will re-factor to remove the KSES, and make it more KISS!

The indentation for $allowed is incorrect

Yep be better as spaces :)

[hnla]

Final version: Simplified approach, let user pass markup in, no kses.

[DJPaul]

Do it!

[hnla]

In 11828:

Pass additional $args to 'bp_get_new_group_invite_friend_list()`

This function renders markup for the looped item defaulting to 'li' but leaves it to the template to provide the parent element.

Commit provides two new args 'before' & 'after' to allow the function to return the complete element markup if desired, simplifies templates having to render markup and checks for empty function return.

Props DJPaul.

Fixes #7656