Skip to:
Content

Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#6006 closed enhancement (fixed)

User Types API

Reported by: boonebgorges Owned by:
Milestone: 2.2 Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch
Cc: hugoashmore@…, r_a_m_i@…

Description

For almost every BP client site I've ever built, I've had to build some kind of support for "user types". Sometimes it's so that the member directory could be filtered; sometimes it's so different types of users can have different permissions; sometimes it's so different types can have different kinds of profiles; and so forth. But it always comes down to a system that involves sorting users into buckets. In the past, I've used xprofile, taxonomies, and usermeta for the purpose, all to various levels of success.

It's time for BP to provide the basics of a user type API, so that plugin devs and site builders will have some common tools for building the user type functionality required by their implementation. Here are some initial proposals on how I think it should work. I'm glad to own this task for 2.2, but I'd be very happy to hear feedback.

Data storage

At WCSF 2014 http://bpdevel.wordpress.com/2014/11/12/at-wcsf-some-attendees-of/, we talked about storing user types as usermeta. I think this would work. But the more I think about it, the more I think that a taxonomy suits this purpose better. Taxonomies perform better and are designed for one-to-many relationships, which will make it more elegant to support multiple user types per member in the future.

So I'm proposing that BP register a private taxonomy 'bp_user_type'.

CRUD functions

If we go the taxonomy route, then individual user types will be terms in the 'bp_user_type' taxonomy, user-usertype relationships will be rows in wp_term_relationships, and our CRUD functions will be wrappers for WP's _terms() functions. bp_set_type_for_user() will wrap wp_set_object_terms(), bp_get_type_for_user() will wrap wp_get_object_terms(), bp_get_user_types() will wrap get_terms(), and so forth.

Registering user types

I propose a function bp_register_user_type( $type, $args ). Syntax will parallel register_post_type(), register_taxonomy() etc as much as possible. User type data will be stored in the $bp global at 'bp_init'. We'll need functions like bp_get_user_type_object().

UI

At a first pass, I propose that the only UI provided by BP is a user-type selector metabox on Dashboard > Users > Community Profile. We'll only show this selector if any user types have been registered. I'm leaning toward making the selector a dropdown, which will mean that we'll only formally support having a user in a single user type at least in the first revision, but I would be willing to be overruled on this.

Querying

We should add a parameter 'user_type' to BP_User_Query. It will support a string or an array of values, and do an IN tax query to generate the necessary SQL clauses.

Attachments (4)

6006.patch (20.5 KB) - added by boonebgorges 3 years ago.
bp-member-type-test.php (355 bytes) - added by boonebgorges 3 years ago.
6006.2.patch (26.3 KB) - added by boonebgorges 3 years ago.
6006.3.patch (26.2 KB) - added by boonebgorges 3 years ago.

Download all attachments as: .zip

Change History (32)

#1 @boonebgorges
3 years ago

Related: #5192. Once we have a basic API in place, we can start thinking about situations like this where BP core could detect the existence of registered roles, and then provide some nice UI. In the case of #5192, we would show a selector in the xprofile field/group admin that says something like "Show this field/group to users of the following type(s):"

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


3 years ago

#3 @johnjamesjacoby
3 years ago

My only concern with taxonomies is mixing global user tables with blog taxonomy tables. In multisite/network installations, supporting a global set of user types means querying whatever site is defined to have the taxonomy terms, etc... This complicates setups with multiple datacenters and/or servers where global tables are likely not directly accessible to blog tables.

It's the slippery slope to using blogs for general data storage that we've talked about (and successfully avoided) since fielding questions about converting to custom post types in 2010.

The table scope difference is really the only hang-up I have here. Other than that, we get a lot for free by using core tables and API's, and it's certainly more flexible than 1 usermeta entry.

#4 @boonebgorges
3 years ago

Good points, johnjamesjacoby. Thinking about this a bit more, BP_User_Query doesn't actually reference the users table directly in most cases - it looks at the activity or xprofile table. See https://buddypress.trac.wordpress.org/browser/tags/2.1.1/src/bp-core/bp-core-classes.php#L241. I think - though please correct me if I'm wrong - that we can depend on those tables being in the same database as the root blog taxonomy tables, at least in any HyperDBish setup.

The two cases where it matters are: 'popular' (queries usermeta) and 'alphabetical' when hitting wp_users. I think in these cases, we might need to do a separate query that gets dumped into an IN clause. The negative performance implications of this strategy could be largely mitigated by aggressively caching usertype data, which I think we can probably do in much the same way that WP caches term trees (see eg _get_term_hierarchy()).

When I build the first rev of this, I'll try to take these cases into effect, and hopefully you'll be willing to put some eyes on them :)

#5 @r-a-y
3 years ago

I think we should figure out what a generic, core taxonomy class and helper functions would look like before delving deeper into user type taxonomies.

The group taxonomy ticket also has some ideas about this. See the first half of 4017.03.patch.

#6 @boonebgorges
3 years ago

I think we should figure out what a generic, core taxonomy class and helper functions would look like before delving deeper into user type taxonomies.

Yeah, I referenced the group taxonomy ticket (though not by number) during the dev chat. I do think we should look toward common infrastructure for BP taxonomy - since all the concerns about blog-switching etc will be the same - but I also don't think we necessarily need to solve that general problem before we attack user types. Worst case scenario, IMO, is: we decide to do both this and #4017 for 2.2, and in 2.2 we reproduce some of the blog-switching logic in both classes because we're not confident about the best way to abstract it to be shared, in which case we just abstract it out in 2.3 or whatever. I don't think this would be a serious problem. Though TBH I think the BP_Term stuff is pretty good, and probably just needs some unit tests to be ready for use.

#7 @imath
3 years ago

Great work boonebgorges. I agree with r-a-y it would be really nice to have a generic BP taxonomy class that could be used for user types, eventually group tags ...

#8 @hnla
3 years ago

  • Cc hugoashmore@… added

#9 @boonebgorges
3 years ago

See #6028 for a suggested approach for taxonomy support in BP.

@boonebgorges
3 years ago

#10 @boonebgorges
3 years ago

  • Keywords has-patch added

6006.patch is a first pass at adding member type support. (I decided to go with "member" type because that's the term we use elsewhere in BP.) Here's a summary:

  • Added the minimal amount of taxonomy wrappers necessary make it work. See #6028.
  • The functions for registering and fetching taxonomy types are: bp_register_member_type(), bp_get_member_type_object(), bp_get_member_types(). These mirror their post-type brethren exactly. Member type objects currently support very few arguments - just some minimal 'labels'. This can be expanded as we come up with new stuff to support.
  • The functions for handling member type for specific users are: bp_set_member_type(), bp_get_member_type(). I don't like how the latter function is close to bp_get_member_types() and _type_object(). But I wanted to keep the function names and their syntax simple, because these CRUD functions are the ones that plugin authors are going to be using the most frequently. Feedback welcome on this point.
  • When member types are detected, a meta box is added to the Community Profile page in the Dashboard. You can use this to change type for a given user.
  • 'member_type' support has been added to BP_User_Query. Accepts a single value, comma-separated values, or an array. I was going to add support for 'any' and 'none', but it's made a bit complicated by some limitations in WP's API. See http://core.trac.wordpress.org/ticket/29181. This can be added later if people want it. As for the issues that johnjamesjacoby describes above: I've put the logic into a subquery, so it should not be an issue for the time being. I think this will be more than fine for 99.9% of installations, and we should iterate for the .1% if we find that performance is somehow limited.

bp-member-type-test.php is a simple mu-plugin that shows how to register member types.

I think this is getting pretty close to an MVP for 2.2, but I would definitely value feedback from other members of the team before moving forward. Thanks!

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


3 years ago

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


3 years ago

#13 @imath
3 years ago

I've tested :)

Very interesting work. But IMHO, it might be a bit "frustrating" for some users.

So i can register user types, and go into the "Admin" extended profile to set a user to one of the types i've registered : very nice. I can see how to use this while building plugins but for regular users : it's a bit limited.

I've seen you are not using get_terms() but a specific buddypress() global to store and get the types. So when fetching the user types, we don't get the terms count. I think that's the kind of thing people would like to know : how many members are from this type ?

Maybe it's a bit too early, i guess to get the count of 1 type you can do a BP_User_Query, but if i want to get the count of each type, you have to run as many BP_User_Query as there are types :(

Without a loop parameter, i find it frustrating because to filter members by user types, you need to run the same query 2 times

  1. First to get the user ids to build the include args of bp_has_members()
  2. and second when precisely the specific bp_has_members() loop is run.

Finally i've quickly built a widget and used it in a subsite. It's working fine. But if i'm adding the same widget another time, i'm switching blogs 2 times. Is there a way to avoid this ? Or maybe it's safer to switch blogs each time a BP Taxonomy function is used..

Thanks a lot for this great work :)

#14 @boonebgorges
3 years ago

Thanks for the feedback, imath!

I can see how to use this while building plugins but for regular users : it's a bit limited.

Yes, that's the idea. It doesn't do *anything* for regular users right now. It's up to plugins to build interfaces for regular users.

So when fetching the user types, we don't get the terms count. I think that's the kind of thing people would like to know : how many members are from this type ?

The reasons for not depending on get_terms() are the same as not depending on SELECT DISTINCT post_type FROM $wpdb->posts to get a list of post types: (a) it might reference member types that are no longer registered in plugins (and thus would not work properly), and (b) this query won't contain the necessary information (like labels) to build our interfaces. A more subtle reason has to do with the fragileness of the WP taxonomy system: there is the issue of terms that are shared between taxonomies (at least pre-4.1), the problem of term IDs not being persistent after export/import, the problem of occasional orphaned terms. For these reasons, the array of member types registered using bp_register_member_type() must be primary.

That said, it seems like a fine idea to use get_terms() to fetch info about the term use. Maybe something like this: when using bp_get_member_types() or bp_get_member_type_object(), do a get_terms() or get_term() query (which would perhaps be optional) that would get stuff like term_id and count and then populate it into the member type object(s) before returning. I can see the value in this. Though it's worth noting that, at least in the case of user counts, this info is also likely to be unreliable: it doesn't account for user last activity, and we'd need to be sure to delete member_type relationships for users when they're deleted or marked as spam. I guess it might be worth caching "true" member type counts (ie those that account for last_activity etc) in a separate place, like the options table - much like we do for, eg, friend counts.

Without a loop parameter, i find it frustrating because to filter members by user types, you need to run the same query 2 times

I am less sympathetic to this. Just use BP_User_Query with the member_type param. That's what it's for.

If i'm adding the same widget another time, i'm switching blogs 2 times. Is there a way to avoid this ?

I haven't made any attempt to implement any sort of caching. But yes, at the very least, we could put the results of some of these queries into a non-persistent cache, so that you only have to run them once per page load. (Note that at least some of the underlying stuff is already cached in the WP API internals. I think that this works as expected inside of switch_to_blog().) I will also say this: I obviously want everything to work correctly on subsites and in various network configurations, but it's very premature to be *optimizing* for those setups at the moment. I don't want to overengineer, at least for the first version. Doing a member_type loop on a secondary site is going to account for < 1% of the use of this new functionality, at least at first, so I think it makes sense to make incremental improvements in this area as it becomes more popular.

#15 @boonebgorges
3 years ago

Another thought, coming out of what imath has suggested here: I do think it makes sense to pre-fetch member types for users when initializing a members loop, as part of the 'populate_extras' routine. And then, when fetching bp_get_member_type(), we can check for the existence of this pre-fetched value. This would be similar to the way we pre-fetch groupmeta in the loop. I think this does seem appropriate for v1 of this feature, and I'll work on it for a second patch.

@boonebgorges
3 years ago

#16 @boonebgorges
3 years ago

imath's suggestions were helpful in a couple of ways. 6006.2.patch expands the original patch in the following ways:

  • Cache support is added for user member_type values. This is good from a performance point of view in general, but it also provides the foundation for the next point:
  • When doing a BP_User_Query, member type information for each returned user is now pre-loaded into the cache with a single database query. This means that there is zero additional overhead to using bp_get_member_type() within the loop.
  • Added some actions and filters.
  • Centralized the logic of member type objects a little bit, so that all such requests go through bp_get_member_types() instead of touching the global directly.

This last point seems like a small one, but I did it in support of one of imath's comments that I did *not* implement. He expressed some remorse that I wasn't using get_terms() to pull up live taxonomy data as part of the member type object(s). By centralizing the logic of pulling up member type objects, I make it so that it's possible to do a single get_terms() query and populate these objects with taxonomy data on the fly.

However, I've decided not to do this in 2.patch. My reasoning is as follows: from the point of view of plugin authors, I would like the member type API to be as implementation-agnostic as possible. That is, if you're running bp_register_member_type() in a plugin, you should not need to know or care that member types are a WP taxonomy. This has two benefits. (1) If we ever decided that these internals needed to change - even away from taxonomy altogether - we wouldn't need to worry too much about backward compatibility. (2) It makes it just a little harder for plugin authors to do the kinds of weird things with taxonomy terms that might backfire down the road. For example, I'm discovering the hard way as I work with WP's taxonomy API that a fair number of plugins do odd things like cache term_ids in postmeta, which means that they're tied to that specific ID being persistent, which it may not be in case of an export or some other operation. Now, in theory, I could load the term_id and term_taxonomy_id etc for each member_type into the member type object, but that would just encourage plugin authors to do strange stuff like that, and I don't think that's something we want to actively encourage.

As for the point about member type *counts*. Yes, I agree that this is something people would like. I do *not* think it should be done by looking at term_taxonomy:count, because that value does not take BP activity into account. If this is something lots of people want, we can build wrappers that use BP_User_Query internally, and perhaps have a separate cache for this data. This task might be made a bit easier with some minor mods to BP_User_Query - like support for fields=ids. But I think that all of this can be handled in a separate ticket, since IMO it's nice to have, but not critical, for v1 of the API.

#17 @ramiy
3 years ago

  • Cc r_a_m_i@… added

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


3 years ago

#19 @DJPaul
3 years ago

I have not tested this but have done a code review. Here's what I'm thinking:

  • Use bp_is_root_blog() prior to switch_to_blog; just for consistency with most of our other switch_to_blog calls.
  • wp_get_object_terms() used to be an uncached function call (https://vip.wordpress.com/documentation/caching/uncached-functions/) -- I don't know if any recent changes have added caching, but if they have not, we should use get_the_terms().
  • Instead of _ex( ' - ' ) which I understand but looks weird, can we re-use an existing string we have for this type of thing? See https://buddypress.trac.wordpress.org/browser/trunk/src/bp-xprofile/bp-xprofile-classes.php#L1701
  • In process_member_type_update(), I can follow through and understand how the validation for $_POST['bp-members-profile-member-type'] works, but: a) it's not immediately obvious, and b) I think sanitize_text_field is appropriate here in addition to stripslashes. Can we move (duplicate) the validation logic into process_member_type_update()?
  • In process_member_type_update(), there are no validation checks to see if the current user is entitled to update this field. Is this intentional -- can a user update their own user_type field, or are we restricting it to super-admins etc? Is there a capability check we should be using?

@boonebgorges
3 years ago

#20 @boonebgorges
3 years ago

Thanks for the helpful feedback, DJPaul. See [6006.3.patch].

Use bp_is_root_blog() prior to switch_to_blog; just for consistency with most of our other switch_to_blog calls.

I think we might want to revisit whether this is necessary at some point, but yes, let's be consistent. Change made. (Note that the same check shouldn't happen around restore_current_blog(), since it will cause the switch-back not to occur.)

we should use get_the_terms()

Yes, wp_get_object_terms() is uncached. However, we can't use get_the_terms(), as it's for posts only, and bp_member_type is a taxonomy on users. What get_the_terms() is is a wrapper for wp_get_object_terms() that uses the object term cache. I don't see a corresponding tool for user taxonomies in WP. We could build one, but (a) this is redundant with the cache implementation currently in the patch (which is specific to member types, and ignores other taxonomies), and (b) it might cause negative performance issues if the installation is using other plugins that have user taxonomies. I'm actually pretty on the fence about this, and could go either way; but definitely we should either stick with the specific caching already in the patch, or scrap it and build our own taxonomy term caching for users - but not both. For the moment, I've done nothing.

Instead of _ex( ' - ' ) which I understand but looks weird, can we re-use an existing string we have for this type of thing

Done.

Can we move (duplicate) the validation logic into process_member_type_update()?

Done.

Is there a capability check we should be using?

This method is hooked to 'bp_members_admin_load', which only fires on the member edit page, which itself is protected via the logic used to generate that admin page in the first place. That said, there's no harm in adding the additional cap check, and it does make the protection clearer, so I've done it.

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


3 years ago

#22 @boonebgorges
3 years ago

In 9210:

Introduce Member Types API.

BuddyPress member types are akin to WordPress's custom post types. Developers
can use bp_register_member_type() to register their types with BuddyPress,
and BP will automatically provide a number of pieces of functionality:

  • Mechanisms for storing and fetching member types (as a WP taxonomy term), with full cache support.
  • A 'member_type' argument for the bp_has_members()/BP_User_Query stack, which allows filtering member loops by member type.
  • Admin UI for changing member types on Dashboard > Users > Community Profile (appears when member types have been registered).

We'll continue to build out more core member type functionality in future
versions of BuddyPress. In the meantime, this is a good starting point for BP
site implementations to have a shared infrastructure for storing and retrieving
this data.

See #6006.

#23 @boonebgorges
3 years ago

First pass introduced in [9210]. I'll leave this ticket open to catch cleanup tasks, and as a reminder to create a stub codex page + an announcement on bpdevel.

#24 @boonebgorges
3 years ago

In 9211:

Better regex for parsing member type taxonomy SQL in WP < 4.1.

See #6006.

#25 @boonebgorges
3 years ago

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

bpdevel post https://bpdevel.wordpress.com/2014/12/08/member-type-api/
codex page https://codex.buddypress.org/developer/member-types/

Let's mark this as resolved, and handle follow-up issues in separate tickets. Thanks for your help landing this, everyone.

#26 @johnjamesjacoby
3 years ago

Great work, Boone. Hugely excellent.

This ticket was mentioned in Slack in #buddypress by jjj. View the logs.


3 years ago

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


2 years ago

Note: See TracTickets for help on using tickets.