allow multiple values for key in usermeta

[wnorris]

Update the usermeta functions to behave the same as the postmeta functions in regards to key uniqueness... that is, allow multiple values with the same key name. They should certainly behave similarly, and allowing for multiple values is certainly more flexible and allows for more optimized searching of the usermeta data.

[ryan]

Moving enhancements to 2.8.

[sojweb]

Added add_usermeta function (similar to add_post_meta), and modified update_usermeta to accommodate multiple identical keys.

[sojweb]

I've attached a patch file (user.php.diff) that might be useful in remedying this problem; I've tried to mimic the behavior of the post meta functions, while maintaining the current behavior of the user meta functions.

[misterbisson]

sojweb and I both tried solving this from different angles. I'm attaching a patch that basically adopts all the *_post_meta() functions for user meta and maintains backwards compatibility by calling the new *_user_meta() functions from within the old *_usermeta() functions.

Why make such a large change? Because post meta and user meta are just too similar a concept to have them work so differently, or have code that works so differently.

[misterbisson]

Boldly changing the milestone to 2.8 and adding the "has-patch" keyword.

[ryan]

When setting up a user, _fill_user() is called to populate the user object with meta values. The object is then cached, complete with meta. The patch adds a separate layer of user meta caching and removes consultation of the user cache. We're doing two queries and caching to two places for the same thing. Need to consolidate that somehow.

[misterbisson]

@ryan. Yeah, I knew somebody was going to call me on that.

I've updated _fill_user() to use update_usermeta_cache(). There are three caches _fill_user() sets up:

	wp_cache_add($user->ID, $user, 'users');
	wp_cache_add($user->user_login, $user->ID, 'userlogins');
	wp_cache_add($user->user_email, $user->ID, 'useremail');

This change to allow multiple entries for a meta_key has serious implications for the users cache, as there's a lot of code that expects there to be only one entry per meta_key there. The other two caches are based on data in the users table and unaffected by this change.

The approach I took is to preserve the format of the users cache (one value per meta_key). The increased memory usage implications are probably minimal for most sites (/broad assumption), though a future patch could normalize these caches.

[Denis-de-Bernardy]

as long as backwards compat is enforced, I'm all +1.

[Denis-de-Bernardy]

on delete_user_meta_by_key -- see also #9009

[Denis-de-Bernardy]

other notes...

the patch no longer works against trunk.

other suggested improvements:

wp_cache_get($user_id, 'user_meta');
wp_cache_delete($user_id, 'user_meta');

should be replaced with calls to:

wp_cache_get($user_id, 'user');
wp_cache_delete($user_id, 'users');

(see Ryan's note)

add_user_meta could check the cache too, on a unique key

... and the above note on delete_user_meta_by_key().

[Denis-de-Bernardy]

wp_cache_get($user_id, 'users');

even...

[misterbisson]

@Denis-de-Bernardy

Please see my reply to Ryan's comment. Code that's reading from the users object cache is expecting one value per key. There's really no way to avoid double caching without breaking backwards compatibility.

wp_cache_get($user_id, 'user_meta');
wp_cache_delete($user_id, 'user_meta');

should be replaced with calls to:

wp_cache_get($user_id, 'user');
wp_cache_delete($user_id, 'users');

[Denis-de-Bernardy]

in _fill_user(), the meta values get passed as object variables.

you can then test if they exist using isset($user->meta).

[Denis-de-Bernardy]

mmm... on second thought... there may be bugs, or rather backwards compat issues, if the new API spits out arrays when $single is set to true.

I personally see no obvious workaround: assuming $single = true in the $user object is plain wrong.

@ryan: thoughts?

[Denis-de-Bernardy]

(I'd say tested commit on the latest patch if it's refreshed to work with trunk.)

[misterbisson]

@Denis-de-Bernardy: you're quick. I figured I'd have a couple minutes to clean up the patch ;)

New patch attached, addresses #9009 and trunk compatibility. As before, it maintains compatibility with the previous object caches (users, userlogins, and useremail), while adding a new one (user_meta) that allows multiple values per key.

[Denis-de-Bernardy]

mmm. actually, there are still a couple of problems. (it's mostly cache related.) looking into it.

[Denis-de-Bernardy]

here's the potentially ugly use-case...

first:

add_user_meta($user_id, 'foo', 'bar', false);
var_dump(get_user_meta($user_id, 'foo')); // array(0=>'bar')

then:

$user = new WP_User($user_id);
var_dump($user->foo); // 'bar'

the only workaround I can think of is to store the $single status into the user_meta table.

[Denis-de-Bernardy]

in the patch I uploaded, the various *_user_meta() functions do not behave exactly the same as the *_usermeta() functions. but it's mostly to get a 2nd opinion on the method.

usage would be as follows for single-key options:

add_user_meta($user_id, 'foo', 'bar', true);
get_user_meta($user_id, 'foo');
update_user_meta($user_id, 'foo', 'bar');
delete_user_meta($user_id, 'foo');

and for multi-key options:

add_user_meta($user_id, 'foo', 'bar');
get_user_meta($user_id, 'foo');
update_user_meta($user_id, 'foo', 'bar', 'old');
delete_user_meta($user_id, 'foo', 'old');

in other words, the meta key would have to be initially declared as multi-key, else it'll get treated (always) as a single key.

one very questionable option I took is in update_user_meta(): it assumes that anything passed *with* a previous value will be a multi-key option; and anything without one is a single-key option.

[Denis-de-Bernardy]

adding to the previous comment, this line in update_user_meta():

$wpdb->update( $wpdb->usermeta, $data, $where );

is plain wrong, in the sense that it would need to do:

• if single, make sure a single key is left in there
• if multi, make sure all keys are marked as not single

but I'd rather get a 2nd opinion/dev feedback before things move any further. the likelyhood is this ticket may end up as wontfix due to the huge potential for bugs.

[Denis-de-Bernardy]

oh, and that is not to mention the numerous other queries in WP that would need tinkering (in wp-includes/user.php and wp-admin/includes/user.php), due to the introduction of multi-key options.

[misterbisson]

@Denis-de-Bernardy:

The approach I was taking was to introduce a set of *_user_meta() functions that behaved identically to the *_post_meta() functions. The value I see in this, aside from solving the problem in this patch, is to allow greater reusability between the two similar areas of the code. As noted above, I can imaging a future patch joining the *_user_meta() and *_post_meta() functions into one (with a variable that switches them between post and user contexts).

The make-usermeta-work-like-post_meta.diff, like any that would address the ticket, has the risk of introducing unexpected behavior, but that risk is mitigated by the fact that those behaviors are well understood in the context of post meta. (And paves the way to consolidating/eliminating code later.)

On the other hand, any patch that leads to a solution would be very valuable to me.

[Denis-de-Bernardy]

I figured, yeah. My own concern (and, I believe, Ryan's) would be to make sure nothing breaks in the process. It's like... I've plugins that have come to rely on the fact that get_usermeta() is redundant for $user->foo, and I'm certainly not alone. The fact of the matter is, I'd much rather that postmeta behaves the same as usermeta, rather than the other way around.

The usefulness of multi-key postmeta lies in being able to run queries against their values when they're arrays without string indexes. I've found this was rarely useful in practice, because when you *do* need to run queries on such things you're often better off creating or modifying a table outright, to avoid SQL joins that are slower than they should (keep in mind that the meta value is stored as LONGTEXT :-)).

[misterbisson]

I agree: any site that would actually put this to significant use would have to add an index to that column, and anybody who would join against anything other than the user_id is begging for performance problems.

Still, Considering the small number of users that most sites have, the potential negative effects are quite limited. And sites that do host a large number of users have a variety of performance issues to be aware of (and, hopefully, are in a position to manage them).

[Denis-de-Bernardy]

actually, quite a few sites use WP to run membership sites. some with very large numbers of users.

[misterbisson]

Patch to make the user meta functions work the same as post meta. Revised against trunk and to incorporate suggestions by Denis-de-Bernardy (and now stripped of some bad characters that snuck in to my earlier patch)

[misterbisson]

You're probably right to suggest there are more membership-based sites than I'm aware of. Still, the patch I propose here won't harm their performance. That is, the patch isn't inherently bad for performance even if does create another opportunity for a plugin dev to write inefficient code. That's true of much of WP, and it'd be a shame to walk away from an opportunity to add value to WP because there's a chance somebody else might not use it well.

[Denis-de-Bernardy]

Replying to misterbisson:

You're probably right to suggest there are more membership-based sites than I'm aware of. Still, the patch I propose here won't harm their performance.

Indeed, but your patch contains a bug:

​http://core.trac.wordpress.org/ticket/7540?replyto=28#comment:20

[Denis-de-Bernardy]

there also is the cache-related problem that Ryan highlights, and which I attempted to fix in the patch I suggested. But it is 100% likely to get rejected by a core dev, due to the potential for bugs and performance issues.

Thus, I'm leaning towards closing this as wontfix, until someone thinks of an alternative solution that kills both birds in one go.

[misterbisson]

I didn't understand your reply (​http://core.trac.wordpress.org/ticket/7540#comment:20) earlier, now I do. I have an idea of how to fix it, will add new diff shortly.

I need some clarification regarding the cache issue, however. My patch adds the user_meta cache so that the structure of the the user cache (and everything that depends on it) doesn't need to change. Is that small cache redundancy the problem, or am I missing something again?

[ryan]

See #9253

[Denis-de-Bernardy]

Replying to misterbisson:

I need some clarification regarding the cache issue, however. My patch adds the user_meta cache so that the structure of the the user cache (and everything that depends on it) doesn't need to change. Is that small cache redundancy the problem, or am I missing something again?

Only Ryan could confirm, but that's more or less how I understood his comment:

​http://core.trac.wordpress.org/ticket/7540?replyto=31#comment:7

[ryan]

Sorry, haven't looked over the latest patch yet, but a small redundancy can add up big for WPMU installs.

[Denis-de-Bernardy]

see also: #9640

[Denis-de-Bernardy]

moving to Future, pending a new patch

[Denis-de-Bernardy]

see also #3243

[scribu]

See #10837.

[scribu]

Marking this as dupe of #10837, in the ideea that we should be deprecating the *_usermeta functions, instead of changing their behaviour.