[buddypress-trac] [BuddyPress Trac] #5399: Review metadata functions for irregularities

[buddypress-trac]

#5399: Review metadata functions for irregularities
--------------------------+---------------------------
 Reporter:  boonebgorges  |       Owner:  boonebgorges
     Type:  enhancement   |      Status:  assigned
 Priority:  normal        |   Milestone:  2.0
Component:  Core          |     Version:
 Severity:  normal        |  Resolution:
 Keywords:  2nd-opinion   |
--------------------------+---------------------------
Changes (by boonebgorges):

 * keywords:   => 2nd-opinion


Comment:

 Here's a summary of the problems I see. For the most part, the same
 problems persist across all four components (activity, blogs, groups,
 xprofile). I'll highlight when there are separate concerns.

 The problems are organized into categories. The first are what I see as
 outright bugs, quirks that do not serve the intended purpose and introduce
 undesirable behavior. The second are points where we are inconsistent,
 either internally or with the behavior of the WP functions. In my view,
 the first category of bugs should be fixed immediately. The second
 category should be considered on a case by case basis; I'll give arguments
 for each below.

 __A. Bugs__

 a. Meta key "sanitization". We limit the allowed character set on meta
 keys to `|[^a-zA-Z0-9_]`. This in itself is kinda silly. The really bad
 part is that we silently strip these characters from the meta_key in
 nearly every meta function. That means that, eg, the following two
 function calls will overwrite each other, but give no indication that
 they've done so:

 {{{
 bp_activity_update_meta( $activity_id, 'foo', 'bar' );
 bp_activity_update_meta( $activity_id, 'f!o!o!', 'bar' );
 }}}

 See related unit test:
 https://buddypress.trac.wordpress.org/browser/trunk/tests/testcases/activity/functions.php#L125

 This is obviously incorrect. My recommendation is that we remove the
 "sanitization" altogether and let WP deal with it. If this is unacceptable
 for some reason, let's start returning false when there are illegal keys,
 to indicate failure.

 b. Trimming meta_value when passed to _delete_meta(). The _delete_
 functions take an optional parameter $meta_value. When present, metadata
 will only be deleted when the value matches $meta_value. For some reason,
 we trim the parameter value before processing the delete. That means that
 the following two function calls do the same thing:

 {{{
 bp_activity_delete_meta( $activity_id, 'foo', '       bar        ' );
 bp_activity_delete_meta( $activity_id, 'foo', 'bar' );
 }}}

 and, moreover, there is no way to delete metadata only where the value is
 '       bar       ' (which is what you'd think the first one would do). My
 recommendation is that we remove this trimming.

 __B. Inconsistencies__

 a. Return value of unsuccessful _get_. Our _get_meta() functions, by and
 large, return false when no value is found. Contrast this with the core
 meta functions, which all return empty strings when no value is found:

 {{{
 // All return '' when no value is found
 get_user_meta( $user_id, 'foo', true );
 get_post_meta( $post_id, 'foo', true );
 // etc
 }}}

 Core metadata functions *do* return false on outright failure, like when
 you don't pass a proper object id:

 {{{
 // bool(false)
 var_dump( get_user_meta( '', 'foo', true ) );
 }}}

 My recommendation is that we change our behavior to align with WP: return
 `''` when a value is not found, and return false for miscellaneous errors.
 The only potential problem is when someone is doing a strict check against
 the return value of one of these functions:

 {{{
 $group_location = groups_get_groupmeta( $group_id, 'location' );
 if ( false === $group_location ) {
         echo 'You have not entered a location yet!';
 }
 }}}

 I couldn't find any examples of this kind of thing in the plugin repo, and
 I find it highly unlikely that there are real-world examples of it. So the
 risk is fairly low.

 b. Return value when update_ functions pass through to add_. Core
 update_meta functions return true when you're performing an *update*. But
 when no value already exists for the key, it's handed off to
 add_metadata(), and on success, the meta_id (an int) is returned. Thus:

 {{{
 // First time added: int(54)
 var_dump( update_post_meta( $post_id, 'foo', 'bar' ) );

 // Updated: bool(true)
 var_dump( update_post_meta( $post_id, 'foo', 'bar2' ) );
 }}}

 Our update_ functions, on the other hand, return true in both cases. My
 recommendation is that we adopt the WP approach. The danger would be
 strict type checking in plugins:

 {{{
 if ( true !== groups_update_groupmeta( $group_id, 'foo', 'bar' ) ) {
         echo 'Sorry, your value could not be updated';
 }
 }}}

 But again, I'm very doubtful that anyone is doing this.

 c. _get_ return value format when no meta_key is passed. When you use a
 core get_meta function without a meta_key, you get back all of the values
 found for that object:

 {{{
 add_post_meta( $post_id, 'foo', 'bar' );
 add_post_meta( $post_id, 'foo', 'bar2' );
 add_post_meta( $post_id, 'foo1', 'bar' );
 get_post_meta( $post_id );

 // array (
 //     'foo' => array(
 //         'bar',
 //         'bar2',
 //     ),
 //     'foo1' => array(
 //         'bar',
 //     ),
 // )
 }}}

 BP's current implementation is inconsistent across components.

 {{{
 bp_get_activity_meta( $activity_id );
 // array (
 //     0 => stdClass
 //          'meta_key' => 'foo',
 //          'meta_value' => 'bar'
 //     1 => stdClass
 //          'meta_key' => 'foo1',
 //          'meta_value' => 'bar1'
 // )

 groups_get_groupmeta( $group_id );
 bp_blogs_get_blogmeta( $blog_id );
 bp_xprofile_get_meta( $field_id, 'field' );
 // array(
 //     'bar',
 //     'bar1',
 // )
 }}}

 It's pretty clear to me that the second variation, where meta_keys are not
 returned, is more or less useless. For this reason, I think we can safely
 assume that no one is actually using it, and we can use the WP return
 format. For bp_activity_get_meta(), the return value is at least
 structured in a way where someone might use it.

 However, I think that the chance of breakage is fairly slight, and we can
 publicize it prior to release. I recommend that we move to the BP return
 format.

 ==

 Thanks for reading this far :) Feedback welcome on each item: A1, A2, B1,
 B2, B3.

--
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/5399#comment:2>
BuddyPress Trac <http://buddypress.org/>
BuddyPress Trac