Enable BP_XProfile_Field_Type classes to register display filter

[boonebgorges]

It makes sense that xprofile field types should be able to apply custom filters to their output value. We currently do this in BP in an ad hoc way, using the 'bp_get_the_profile_field_value' filter. See eg ​https://buddypress.trac.wordpress.org/browser/tags/2.0.1/bp-xprofile/bp-xprofile-filters.php#L118

Now that we have BP_XProfile_Field_Type, maybe we can do better. My suggestion: If a field type class has a method display_filter(), we automatically apply it to the output of fields of that type. The attached patch has a suggested approach. It's a rough draft - ReflectionClass objects created in this way should be cached, function naming is up for debate, etc. But it demonstrates how we might move the formatting for one specific core field type (datebox) into the class, and gestures toward richer formatting possibilities for other field types. See eg #5501.

[boonebgorges]

In 5630.patch I was using ReflectionClass objects because I didn't want to force subclasses to override an abstract method, but I realized that that was unnecessary - I could just put a passthrough method in the parent class. See 5630.02.patch for a more straightforward implementation.

[DJPaul]

The general approach looks OK.

• Should display_filter be declared a static function?
• Do you think we should somehow add a filter for display_filter, or do you think it's ok for folks to just re-use the existing bp_get_the_profile_field_value filter for that?

[boonebgorges]

5630.03.patch cleans up some documentation, and marks the display_filter() method static.

Do you think we should somehow add a filter for display_filter, or do you think it's ok for folks to just re-use the existing bp_get_the_profile_field_value filter for that?

I spent a bit of time thinking about this. In theory, I would prefer to apply the display_filter() formatting earlier in the chain, so that any time someone got data out of an xprofile field, the filter would be applied. But our current methods for fetching xprofile data are pretty uneven. In the loop (bp_get_the_profile_field_value()) we do some weird juggling with globals, and we (apparently) don't even unserialize until the very last minute. Then there's xprofile_get_field_data(), which is intended for use outside of the bp_has_profile() loop. It gets data from a totally different pipeline (BP_XProfile_ProfileData::get_value_byid()). I think it's a worthwhile project to clean this all up at some point, but I don't think it's necessary for the improvement currently under discussion.

All of that being said, 'bp_get_the_profile_field_value' does seem like the right place to do the filtering, given the current order of operations. I've also added a wrapper function that allows the filter to be applied to xprofile_get_field_data().

[boonebgorges]

In 8547:

Introduce BP_XProfile_Field_Type::display_filter(), and implement in BP_XProfile_Field_Type_Datebox.

The new display_filter() method makes it easy for field types to customize the
appearance of its values. In the case of Datebox fields, this means formatting
the MySQL- or UNIX-formatted date in the format of bp_format_time().

Fixes #5630

[johnjamesjacoby]

In 8632:

Introduce URL field type:

• New field class extending BP_XProfile_Field_Type for URLs
• Clean-up existing BP_XProfile_Field_Type extensions (formatting, bp_parse_args() VS array_merge(), whitespace, brackets, etc...)
• More to do here, specifically regarding output and display_filter() methodology.

Props boonebgorges, williamsba1, sooskriszta. See #5501, #5630.

[johnjamesjacoby]

Having this method run at priority 5 limits its effectiveness to not including any HTML formatting, as esc_html is ran at priority 8.

I had originally thought/hoped display_filter() was intended to own the output experience, but now see it works alongside existing output sanitization filters. I'd like to see this method (or another one) be fully responsible for handling its output.

[johnjamesjacoby]

Attached patch shows how the URL field would need to hook in after esc_html is ran, so that it can parse the link and modify it on output to match the spec in #5501.

[boonebgorges]

Good catch about esc_html(). I hadn't thought about that.

Changing the priority seems fine to me. I think there's still value in having it be less than 10, so that third-party filters at the default priority will receive an already-formatted string. 8 seems fine (as long as we know that it'll run after the esc_html call which is also at 8 - though it seems like this is a race condition)

[johnjamesjacoby]

I'll change the priority for now. Any thoughts on what unhooking all the bp_get_the_profile_field_value filters in bp-xprofile-filters.php might look like if we move them into their respective display_filters methods instead?

I.E. Running convert_smilies on a URL field is pretty silly at this point.

[johnjamesjacoby]

In 8644:

Adjust filter priority to allow it to scoot in after esc_html(). We'll likely be doing a bit more tweaking here. See #5630.

[boonebgorges]

Any thoughts on what unhooking all the bp_get_the_profile_field_value filters in bp-xprofile-filters.php might look like if we move them into their respective display_filters methods instead?

I agree that this makes much more sense. A couple of complications:

• Moving them inside the filter will break any plugins that are currently using remove_filter() on these filters. This is a legitimate concern, as I'd imagine that some plugins are removing some of our security features to allow additional markup, etc. I can imagine a way of building in backward compatibility for this sort of thing, but it would be pretty arcane.
• Similarly, if we move them inside of display_filters(), it'd be most natural to do something like:

$value = convert_smilies( $value );

But calling the functions directly, instead of using the filter API, provides a bit less flexibility to plugin devs, since there won't be a straightforward way to remove the filtering without overriding the entire class. While it's a bit ugly, we might consider doing something like this in display_filter():

add_action( 'bp_xprofile_display_filter_' . $this->name, 'convert_smilies' );
add_action( 'bp_xprofile_display_filter_' . $this->name, 'esc_html' );

// use this hook if you want to remove_filters()
do_action( 'bp_xprofile_pre_display_filter_' . $this->name, $this );

$field_value = apply_filters( 'bp_xprofile_display_filter_' . $this->name, $field_value, $this );

// then maybe remove the filters?

Not totally straightforward, but it will allow for plugin devs to modify the behavior in a fine-grained way (like they can currently). Maybe this is something we can look at for 2.2.

[DJPaul]

Yeah, let's 2.2. I think this is going to take some figuring out.