WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 4 years ago

#7540 closed enhancement (duplicate)

allow multiple values for key in usermeta

Reported by: wnorris Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.6.1
Component: Users Keywords: needs-patch
Focuses: Cc:

Description

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.

Attachments (3)

user.php.diff (3.3 KB) - added by sojweb 5 years ago.
Added add_usermeta function (similar to add_post_meta), and modified update_usermeta to accommodate multiple identical keys.
7540.diff (7.3 KB) - added by Denis-de-Bernardy 5 years ago.
make-usermeta-work-like-post_meta.diff (8.1 KB) - added by misterbisson 5 years ago.
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)

Download all attachments as: .zip

Change History (45)

comment:1 wnorris6 years ago

  • Keywords usermeta added
  • Version set to 2.6.1

comment:2 ryan6 years ago

  • Milestone changed from 2.7 to 2.8

Moving enhancements to 2.8.

comment:3 FFEMTcJ5 years ago

  • Milestone changed from 2.8 to Future Release

sojweb5 years ago

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

comment:4 sojweb5 years ago

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.

comment:5 misterbisson5 years ago

  • Keywords users added

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.

comment:6 misterbisson5 years ago

  • Keywords has-patch added
  • Milestone changed from Future Release to 2.8

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

comment:7 ryan5 years ago

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.

comment:8 misterbisson5 years ago

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

comment:9 misterbisson5 years ago

  • Component changed from General to Users
  • Owner anonymous deleted

comment:10 Denis-de-Bernardy5 years ago

  • Keywords usermeta users removed

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

comment:11 Denis-de-Bernardy5 years ago

on delete_user_meta_by_key -- see also #9009

comment:12 follow-up: Denis-de-Bernardy5 years ago

  • Keywords needs-patch added; has-patch removed

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().

comment:13 Denis-de-Bernardy5 years ago

wp_cache_get($user_id, 'users');

even...

comment:14 in reply to: ↑ 12 misterbisson5 years ago

@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');

comment:15 Denis-de-Bernardy5 years ago

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

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

comment:16 Denis-de-Bernardy5 years ago

  • Keywords dev-feedback added

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?

comment:17 Denis-de-Bernardy5 years ago

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

comment:18 misterbisson5 years ago

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

comment:19 Denis-de-Bernardy5 years ago

  • Keywords dev-feedback removed

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

comment:20 Denis-de-Bernardy5 years ago

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-Bernardy5 years ago

comment:21 Denis-de-Bernardy5 years ago

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.

comment:22 Denis-de-Bernardy5 years ago

  • Keywords 2nd-opinion dev-feedback added

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.

comment:23 Denis-de-Bernardy5 years ago

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.

comment:24 misterbisson5 years ago

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

comment:25 Denis-de-Bernardy5 years ago

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 :-)).

comment:26 misterbisson5 years ago

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

comment:27 Denis-de-Bernardy5 years ago

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

misterbisson5 years ago

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)

comment:28 follow-up: misterbisson5 years ago

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.

comment:29 in reply to: ↑ 28 Denis-de-Bernardy5 years ago

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

comment:30 Denis-de-Bernardy5 years ago

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.

comment:31 follow-ups: misterbisson5 years ago

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?

comment:33 in reply to: ↑ 31 Denis-de-Bernardy5 years ago

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

comment:34 in reply to: ↑ 31 ryan5 years ago

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

comment:36 Denis-de-Bernardy5 years ago

  • Keywords dev-feedback removed
  • Milestone changed from 2.8 to Future Release

moving to Future, pending a new patch

comment:37 Denis-de-Bernardy5 years ago

  • Keywords 2nd-opinion removed

comment:38 Denis-de-Bernardy5 years ago

  • Milestone changed from Future Release to 2.9

comment:41 ryan4 years ago

  • Milestone changed from 2.9 to Future Release

comment:42 scribu4 years ago

  • Milestone Future Release deleted
  • Resolution set to duplicate
  • Status changed from new to closed

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

Note: See TracTickets for help on using tickets.