Review metadata functions for irregularities

[boonebgorges]

In rewriting our metadata functions, I identified a number of peculiarities in their implementation. Some are mere inconsistencies between component (like the format of the return value of _get_metadata() when no meta_key is passed). Some are, IMO, misplaced attempts at sanitization. And some are outright bugs, as when meta_keys are changed silently in the process of adding metadata.

This ticket will serve as a place to collect instances of these issues, and to begin to make recommendations for what can and cannot be fixed or improved.

[boonebgorges]

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

1. 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.

2. 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

1. 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.

2. 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.

3. _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 WP return format.

==

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

[johnjamesjacoby]

I completely agree on all counts. Thanks for the thorough audit and write-up.

[r-a-y]

A1 - Meta key sanitization removal

It's a little weird for devs to be using characeters outside [^a-zA-Z0-9_] for their meta keys, but yeah, let WP handle meta keys.

A2 - Meta value trimming on deletion
B1, B2 - Strict type checking when fetching meta

No complaints here.

B3 - Getting meta with no meta key

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.

So to confirm, you want to move to the WP format or the BP format? Regardless, I'm fine with either option.

[boonebgorges]

So to confirm, you want to move to the WP format or the BP format?

Oops! I mean the WP format. Good catch.

[boonebgorges]

In 8129:

Don't improperly sanitize meta_key in _meta() functions

Many BP meta functions have always stripped certain characters from the
$meta_key parameter before performing their operations. This is a terrible idea
on a number of levels: it doesn't provide any feedback to the user, it silently
performs actions that are not equivalent to the ones intended by the user, and
it doesn't serve any real purpose (since any necessary sanitization happens at
the level of $wpdb). Moreover, it wasn't even applied consistently across all
functions. A truly delightful grab bag.

This changeset removes the sanitization, and updates the necessary unit tests
to reflect the change.

See #5399

[boonebgorges]

In 8130:

Don't trim whitespace from $meta_value param in _delete_meta() functions

The $meta_value parameter of BP's delete_meta() functions allows developers to
limit deletion to rows that match both the $meta_key *and* the specified
$meta_value. However, most of BP's delete_meta() functions have always had a
quirk whereby the $meta_value was run through trim() before matching against
the values in the database. This had the unacceptable side effect that passing
a $meta_value of ' foo ' would delete rows with the value 'foo', but *not*
the value ' foo '!

This changeset reverses the behavior, and modifies the relevant unit tests.

See #5399

[boonebgorges]

In 8131:

Return an empty string from get_meta() functions when no value is found

BP get_meta() functions have been inconsistent about the type of value returned
when no match is found for the object_id+meta_key combination. In some cases,
we followed WP's get_metadata() and returned an empty string. In others, we
returned false.

This changeset aligns all of our get_meta() functions with WP in this regard,
returning an empty string when no matching value is found. A boolean false is
still returned when invalid arguments are passed to the functions.

Relevant unit tests have been updated, including strict assertSame() checking
where appropriate.

See #5399

[boonebgorges]

In 8132:

In update_meta() functions, return integer value if a new metadata is created

Previously, BP update_meta() functions returned true on a successful update
*or* on the successful creation of new metadata. This is not consistent with
WP's update_metadata(), which on the creation of new metadata acts as a wrapper
for add_metadata(), which in turn returns an integer (the ID of the newly
created database row).

This changeset aligns BP's behavior with WP's.

We don't have any tests that directly address the data type returned by these
functions, but we do have a number of assertTrue assertions to verify the
setup of various tests. Where appropriate, these have been changed to the more
generous assertNotEmpty.

See #5399

[boonebgorges]

In 8133:

Use WP's array format for get_meta() functions when no $meta_key is provided

Passing only an object_id to a get_meta() function in WP will return an array
of arrays, keyed by meta_keys, each containing a list of meta_values for that
meta_key. BP's get_meta() functions were inconsistent in this regard: some
returned arrays of stdClass, some returned one-d arrays of meta_values. All
returned values that were practically of little use, because they didn't have
enough information about the located data to make it useful.

This changeset aligns BP's get_meta() functions with WP's. We now allow the
return value of get_metadata() to pass through untouched in all cases. Unit
tests have been updated as necessary.

See #5399

[boonebgorges]

In 8135:

Remove unnecessary parameter sanitization from meta functions

WP's meta API functions already perform the necessary sanitization.

See #5399

[boonebgorges]

OK, I believe this is done. Our meta functions now behave nearly identically to their WP brethren, with the exception that ours let you delete all meta for a given object by omitting the meta_key (bonus!). Thanks for your feedback, everyone!

[boonebgorges]

In 8237:

Don't do a strict type check when deciding on bp_activity_add_user_favorite() return value

Since r8132, bp_activity_update_meta() has returned an integer value when
falling through to add_metadata(). Thus, we should have a more generous test
for whether the update has been successful. See #5399

Fixes #5515