Skip to:
Content

BuddyPress.org

Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#2952 closed enhancement (fixed)

Filter user meta keys

Reported by: wpmuguru Owned by: boonebgorges
Milestone: 1.5 Priority: normal
Severity: Version:
Component: Core Keywords: has-patch dev-feedback
Cc:

Description

This is the companion patch to http://trac.buddypress.org/attachment/ticket/2561.

I have my plugin written to segregate the BuddyPress content by WordPress network using the bp_core_get_table_prefix filter. However, there are a couple values stored in the user meta which is shared across networks.

The attached patch will give the user meta keys a prefix based on the BP table prefix rather than hardcoded bp_.

Attachments (6)

2952.diff (5.1 KB) - added by wpmuguru 9 years ago.
2952.2.diff (5.2 KB) - added by wpmuguru 9 years ago.
bp-multi-network.php (742 bytes) - added by wpmuguru 9 years ago.
refresh of the plugin with user meta filters
2952.3.diff (42.6 KB) - added by boonebgorges 8 years ago.
2952.4.diff (39.4 KB) - added by wpmuguru 8 years ago.
minor fixes to .3.diff
bp.2952.diff (4.2 KB) - added by nacin 8 years ago.

Download all attachments as: .zip

Change History (33)

@wpmuguru
9 years ago

#1 @wpmuguru
9 years ago

I've attached a plugin you can use to test the patch in a multi netowrk install. The plugin needs to go in the mu-plugins folder to ensure the filter is active before BP loads.

#2 @wpmuguru
9 years ago

2952.diff needs some work. It includes the WP table prefix which we don't want. I'll see if I can whip up another patch tomorrow.

@wpmuguru
9 years ago

#3 @wpmuguru
9 years ago

That now filters the meta keys instead of prefixing with the WP table prefix.

@wpmuguru
9 years ago

refresh of the plugin with user meta filters

#4 @DJPaul
9 years ago

  • Milestone changed from Awaiting Review to 1.3

#5 @DJPaul
9 years ago

  • Keywords 2nd-opinion added

I like 2952.2.diff much better as it won't potentially break plugins which look for the current names. I'm not keen on putting more stuff into the $bp global, however. Could move it into a wrapper function of sorts. Thoughts?

#6 @boonebgorges
9 years ago

wpmuguru and I chatted about this earlier. I had the same reaction about putting more stuff into $bp, but I think it's important that they be filterable (an argument against concatenating on the fly). A wrapper function could play a similar role, I guess.

#7 follow-up: @DJPaul
9 years ago

Would this need to apply to all usermeta keys that BuddyPress create? There are some like 'notification_groups_admin_promotion' which aren't covered by this patch. (Aside: we need to group up some of these options in the meta tables)

#8 @wpmuguru
9 years ago

If there are other meta keys or BP based plugins might create user meta data, then we probably should look at doing a single prefix and concatenate keys as necessary.

I wanted to avoid adding filters within the user meta call because on any of the member listing the meta is retrieved per user, so potentially filter * 20.

#9 in reply to: ↑ 7 @wpmuguru
9 years ago

Replying to DJPaul:

(Aside: we need to group up some of these options in the meta tables)

I would be up for helping with that. Doing that would also make this a single filter/variable as well.

#10 @wpmuguru
8 years ago

There are more user meta rows per #2987. Since that patch is in, merging the user meta into an array will be easier.

Also, in light of the other user meta data, the user meta patch I wrote above would be much more appropriate if the data were stored in the user object instead of the $bp one. Thoughts?

#11 @DJPaul
8 years ago

  • Keywords dev-feedback added; 2nd-opinion removed
  • Owner set to johnjamesjacoby
  • Status changed from new to assigned

John, what do you think about the approach the patch takes?

#12 @boonebgorges
8 years ago

I chatted with wpmuguru and we discussed an alternative strategy. Instead of stashing meta keys in the global, what if we had BP-specific wrapper functions for saving usermeta? That way, we could filter *all* meta keys as they get saved. So something like:

function bp_update_user_meta( $user_id, $meta_key, $meta_value ) {
  $meta_key = apply_filters( 'bp_update_user_meta_key', $meta_key );
  
  return update_user_meta( $user_id, $meta_key, $meta_value );
}

The downside is that we'll have to go through the codebase and make the necessary changes, and also that we'll have to remember to use the wrapper in future meta saves. But, on the other hand, it means that we don't have to do a cumbersome apply_filters everywhere where a piece of metadata is saved. Thoughts?

#13 @boonebgorges
8 years ago

I was thinking about this last night, and I decided that the initial approach that wpmuguru suggested is probably better.

Attached is a huge patch 2952.3.diff that implements the change. It should be totally backward compatible. It means that plugin authors should change the way that their usermeta is saved, but if they don't, it won't break anything unless multi-network is turned on.

The way things are structured in this patch, each meta key is run through its own filter bp_user_meta_keys-$key_name. It's also possible (and probably preferable) to hook a function to bp_setup_globals, with a high priority (maybe 999), and sweep through all of the meta_keys in one fell swoop:

foreach( $bp->user_meta_keys as $key_name => $key_value ) {
  // do your modifications
}

It should be noted that the naming of the keys is inconsistent. Some start with bp_, while most do not. So bp-multi-network.php should probably be a bit more generous about how the prefixing works than it currently is.

wpmuguru, it'd be great to get your feedback on this fairly soon, as the patch is likely to go stale. Thanks!

@boonebgorges
8 years ago

#14 @wpmuguru
8 years ago

Reading through it, it looks good. The only thing I saw that I would change is in bp-core-component.php from

apply_filters( "bp_user_meta_keys-$key_name", $key_value );

to

apply_filters( "bp_user_meta_key_$key_name", $key_value );

It's only doing one key on each pass of the filter & in WP string replaced filters & hooks are usually joined with an _ not with a -.

I'll give it a run later today.

#15 @boonebgorges
8 years ago

Cool. There are some places in WP where filter names are generated with a - in this way (maybe it makes for easier exploding?) but in any case I'm happy to change it to an _.

@wpmuguru
8 years ago

minor fixes to .3.diff

#16 @wpmuguru
8 years ago

@boone - you were missing a couple commas/semi-colons which I added in my patch. I didn't change the filter name.

That worked ok in my testing.

#17 @boonebgorges
8 years ago

  • Owner changed from johnjamesjacoby to boonebgorges

OK, thanks wpmuguru. (It was a big patch :) )

DJPaul or jjj, can one of you have a glance to give this approach the thumbs-up or thumbs-down?

#18 @DJPaul
8 years ago

If the goal is to filter the user meta key names, can't that be done in get_user_meta() etc? If there's a problem about not knowing what keys belong to BP, then we could just change all the keys to begin with "bp_".

#19 @boonebgorges
8 years ago

DJPaul - The problem is that WP does not have filters on the meta keys in get_user_meta() etc. The user_meta functions do nothing but call get_metadata(), update_metadata(), etc, and those functions do not have any appropriate filters either.

We could suggest adding these filters upstream in WP, if we thought the suggestion had a chance of succeeding.

#20 @wpmuguru
8 years ago

Even if the filters were given a thumbs up upstream, it will be at least 3.2 before they make it in.

#21 @boonebgorges
8 years ago

At the rate we're moving relative to the WP team, it's likely that BP 1.3 will require WP 3.2. This change would make that a hard requirement.

Even if WP filtered these keys, the part of this patch that registers BP's user_meta_keys would still be useful. Refactoring so that they are all prefixed with bp_ would be a nightmare, seems easier to check against a whitelist.

#22 @nacin
8 years ago

At a glance, I have two thoughts:

One, rather than a full user meta keys object, just do: update_user_meta( $user_id, $bp->prefix . 'some_key' ... ).

Two, take that a step further and abstract it:

$bp->update_user_meta( $user_id, 'some_key' ... ) or bp_update_user_meta()

I think the current approach would make it more difficult for plugins to add new keys, when in reality you're just trying to adjust the prefix.

I'm noticing now that some keys are not prefixed, and that really hurts chances for compatibility here.

#23 @boonebgorges
8 years ago

After extended discussion with jjj and DJPaul, I'm going with an approach similar to 2952.4.diff, with the addition of an additional bp_get_user_meta_key() function for fetching (and filtering) user meta keys.

This method promises maximum backward compatibility, and maximum flexibility for plugin authors.

I'll write up a bpdevel post to summarize the changes.

#24 @boonebgorges
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [4372]) Abstracts user_meta keys so that they can be filtered. Fixes #2952

#25 @nacin
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

If you're going to have such a helper function, which is a Good Idea, then I don't see why you should also have the $bp->user_meta_keys aspect. Seems useless at this point. Just make bp_get_user_meta_key() return its value, but filtered, no?

Regardless, re-opening if only because user_meta_keys is set up as an array, but then used as an object.

#26 @boonebgorges
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [4378]) Refactors user_meta key filtering to remove overhead in the bp global. Fixes #2952. Props nacin.

@nacin
8 years ago

#27 @boonebgorges
8 years ago

(In [4380]) Additional bp_get_user_meta_key() applications in query classes, plus wpdb->prepare() cleanup. References #2952. Props nacin.

Note: See TracTickets for help on using tickets.