Skip to:
Content

Opened 5 years ago

Last modified 8 days ago

#3961 assigned enhancement

Hierarchical groups

Reported by: sooskriszta Owned by: dcavins
Milestone: 2.7 Priority: normal
Severity: normal Version:
Component: Groups Keywords: dev-feedback has-patch
Cc: hnla, sooskriszta, ddean, egyptimhotep@…

Description

http://buddypress.org/community/groups/requests-feedback/forum/topic/need-for-hierarchical-groups/

http://wordpress.org/support/topic/plugin-buddypress-hierarchical-groups

Hierarchical groups functionality should be part of the core, complete with member propagation – giving 3 options to admin … upwards propagation, downwards propagation, no propagation…

Not having hierarchical groups in BuddyPress core is like if hierarchical pages were not part of WordPress core…

Attachments (11)

hierarchy_groups_patch#4.zip (90.6 KB) - added by abweb 5 months ago.
Archive contains patch file and required files
3961.1.patch (74.8 KB) - added by dcavins 6 weeks ago.
First patch
buddypress-options-hierachy-setting.png (29.6 KB) - added by dcavins 6 weeks ago.
Main group hierarchy setting in BP options screen.
group-manage-group-settings.png (18.1 KB) - added by dcavins 6 weeks ago.
Single group hierarchy options screen, front end.
wp-admin-group-settings.png (25.2 KB) - added by dcavins 6 weeks ago.
Single group hierarchy options screen, wp-admin.
group-child-group-screen.png (23.3 KB) - added by dcavins 6 weeks ago.
New subgroup screen for a group.
groups-directory-with-breadcrumbs.png (48.3 KB) - added by dcavins 4 weeks ago.
Groups directory with hierarchy breadcrumbs.
3961.02.patch (75.7 KB) - added by dcavins 4 weeks ago.
Second round.
3961.03.patch (12.7 KB) - added by dcavins 4 weeks ago.
Minimal patch that makes schema changes and support for querying by parent_id via groups_get_groups()
3961.04.patch (16.5 KB) - added by dcavins 3 weeks ago.
Updated minimal patch that makes schema changes and support for querying by parent_id via groups_get_groups()
3961.05.patch (17.6 KB) - added by r-a-y 3 weeks ago.

Download all attachments as: .zip

Change History (64)

#1 @boonebgorges
5 years ago

  • Cc ddean added
  • Keywords dev-feedback 2nd-opinion added
  • Milestone changed from Awaiting Review to Future Release

Not having hierarchical groups in BuddyPress core is like if hierarchical pages were not part of WordPress core…

That's a bit extreme. Hierarchical groups would be useful for some implementations of BuddyPress, but it's not obvious that it's overwhelmingly required for most instances.

From the data schema point of view, hierarchical easy - just allow for a parent_id or something like that. But there are a ton of questions to be asked about what this means for users. Member propagation is one question, as you've addressed above. But what else would the parent-child relationship mean for groups? Would aspects of the groups be merged (forums, activity, etc)? Would each group have a display of its child and parent groups, perhaps in the header? Would this require additional filters on group directories, or possible tree-like views of group directories?

Again, the technical issues here are (mostly) not that difficult. But it's only worth adding the feature(s) if it can be done in a way that is clear to all users, and that proves useful to most.

In any case, I have a couple of alternate suggestions to bundling all of this with BP:

1) Build proper group taxonomy and meta support. If BP had built-in support for group categories and/or tags, it would solve a big part of the impetus behind group hierarchy (that is, explaining that certain groups are related to each other). IMO, group taxonomy is of much broader appeal than standalone hierarchy, and it's clearer (to me at least) how it would be implemented from a UX point of view. If this were done, then it would be pretty easy to build a plugin that does the additional work of, eg, propagating group membership among similarly categorized groups.

2) Build better support for ddean's BP Group Hierarchy plugin http://wordpress.org/extend/plugins/bp-group-hierarchy/ into BP core (ddean - I added you as a CC to this ticket - hope that's not too rude :) ). This plugin is popular and highly regarded, but I think that David has run into a couple of problems with implementation that have led to some less-than-ideal situations. For one, his plugin has to modify BP's groups table to add a parent_id column; we might consider doing this in BP itself. He's also built his own version of bp_has_groups() just so that he can add hierarchy support, and he does some clever tricks to mess with BP_Groups_Template. If we add a parent_id column to our db schema, we could also add hierarchy support (parent_id, and maybe others if it's relevant) to the bp_has_groups() stack of functions. Then we let the plugin provide all the necessary UI elements, like it already does.

In the short term, I lean toward the latter. It's something we can do fairly easily, it'll make ddean's life much easier, and it won't affect BP users who aren't interested in group hierarchy. Long term, we could then talk as a team about how/whether to implement some of the user-facing stuff in BP in future versions.

#2 @DJPaul
5 years ago

I think adding a database column and modifying groups' query stack for something we aren't going to expose directly in core is not the right approach. If we came to the decision that this is something we wanted to do in core, we can certainly build these parts in, even if the release doesn't allow us enough time to build a UI for it (leaving it to the plugin).

#3 @ddean
5 years ago

Hi, all! Sorry for being tardy; I didn't have an email address set in my Trac profile, so I didn't get the cc.

I thought the long term plan was to make groups into a custom post type, with the built-in taxonomy support that provides, so I've been happy to keep patching things in until then. Is that still the ultimate goal?

While I appreciate anything that makes my life easier :), most of the integration work has already been done and, for compatibility's sake, couldn't just be taken out. Whatever you all decide to do is fine, and I'd be happy to provide a short list of changes that could improve extensibility.

This seems to be the key:

Again, the technical issues here are (mostly) not that difficult. But it's only worth adding the feature(s) if it can be done in a way that is clear to all users, and that proves useful to most.

If download counts are to be believed, hierarchical group users account for ~1% of BuddyPress users, and I've received only a handful of requests for membership propagation. And, though I can certainly see the value of this feature, the time needed to build and test a UI that makes it flexible enough to be useful without becoming a management or security issue has been enough to keep me away from it so far.

#4 @sooskriszta
5 years ago

Innovator's dilemma. Should we go after current audience or a bigger market out there?

Does use of hierarchical groups by 1% of BuddyPress users mean it is not a much-needed function or does it mean that the plugin doesn't do what users want it to do? (no offense ddean..i don't mean it doesn't do what it's intended to do...subtle difference)

I remember the days when WordPress was simply a *blogging* platform and didn't have hierarchical pages. Look at it now - since this became a core feature, a good proportion of people use WP as their CMS, including yours truly.

I truly believe that hierarchical groups with upwards/downwards propagation will open BuddyPress up for users and uses that are beyond its reach at the moment...

Last edited 4 years ago by sooskriszta (previous) (diff)

#5 @boonebgorges
5 years ago

I thought the long term plan was to make groups into a custom post type, with the built-in taxonomy support that provides, so I've been happy to keep patching things in until then. Is that still the ultimate goal?

Maybe. We have to clear some conceptual hurdles first, related to scaling and schema. IMO, the two easiest components to map onto CPTs are Messages and Groups (though not group members). I may write a proof-of-concept after 1.6 comes out, but it will be a while before those decisions become final.

Regardless of the CPT situation, I think that we should move forward with proper meta/tax for groups. On that note, see https://buddypress.trac.wordpress.org/ticket/4017.

#6 @sooskriszta
4 years ago

  • Version changed from 1.5.3 to 1.5.5

#7 @boonebgorges
4 years ago

  • Version 1.5.5 deleted

#8 @sooskriszta
4 years ago

I don’t suppose a ton of people currently use hierarchical groups in BuddyPress. But I am certain that features like this will go a long way in opening up new markets for the script, way beyond just small techie groups.

It’s a usability thing. Let me take an example:

Site for networking of dentists in the UK.
What I have in mind is at top level 4 groups:

  • England
  • Wales
  • Scotland
  • Ireland

Then within each group, there are subgroups for counties.

Within each county subgroup, there are sub-subgroups for town/city etc.

Within the county sub-subgroup for town/city for particularly large cities, there could be sub-sub-subgroups for boroughs.

If Paul joins the group for Exeter, he should automatically become a member of Devon and England respectively.

I’m sure there might possibly be other ways of doing this (e.g. adding categories or tags); I just don’t see anything so instinctive or natural.

There can be multiple other examples.
For a medical professionals organization:
Doctor > Neurology > Clinical neurophysiology
For a university:
Business School > MBA > Managerial Accounting
etc

#9 @sooskriszta
3 years ago

24000 downloads of @ddean's Group Hierarchy plugin http://wordpress.org/plugins/bp-group-hierarchy/stats/ which is about 1.6% of total BuddyPress downloads. Consider also that BuddyPress has been out since April 30, 2009 while the plugin was first released in February 2011 and the actual proportion of the plugin users is likely to be much higher perhaps as high as 3-4%.

Further, 100% of the active installations of BP Group Hierarchy are of the latest version, meaning that these folks are likely using the latest version of BuddyPress too. If that's the case, and if we can use downloads as a proxy for active installations (and it may not be):
BP Group Hierarchy 1.3.8 active installs ~ 25,000
BuddyPress 1.7+ active installs = 1,562,730 x 32.6% ~ 500,000
BP Group Hierarchy 1.3.8 active installs as a proportion of BuddyPress 1.7+ active installs = 5%

This is still a small minority and I'd be the first to admit that for quite a while hierarchical group use will remain a minority activity, though it will probably be a bigger minority.

What I do posit is that

  1. it is not an insignificant minority.
  2. the function will help grow the pie.

#10 @sooskriszta
3 years ago

2.0? :-)

#11 @vernonfowler
2 years ago

30,510 downloads of BP Group Hierarchy to date 31/3/2014
591 downloads of BP Group Hierarchy Propagate - http://wordpress.org/plugins/bp-group-hierarchy-propagate/
I love both these plugins.

As much as some of our communities depend on these plugins layered on top of BuddyPress, I can't see Group Hierarchy making it into core for as long as it has less than 50% of the number of BuddyPress installs.

I'd rather find ways to further optimise BuddyPress itself (and bbPress for that matter). Right now P3 (Plugin Performance Profiler) reports 24% (0.3533 secs) and 12% (0.1749 secs) of my load times are dedicated to these 2 plugins respectively - the 2 heaviest plugins I have (shortly followed by Adminimize at 11%).

This ticket was mentioned in IRC in #buddypress-dev by boonebgorges. View the logs.


2 years ago

#13 @ubernaut
2 years ago

  • Cc ubernaut@… added

#14 @sooskriszta
2 years ago

One big unresolved issue is propagation:

If Group 1 is a parent, and Groups 2 and 3 are children of Group 1, then should someone joining Group 2 automatically become a member of Group 1? Or should someone joining Group 1 automatically become a member of Group 2?

Theoretically, both approaches seem quite valid. However, let's look at live cases and why folks want hierarchical groups. The parent-child relationship essentially a bit like successive filters applied to the groups. At the top level (say Group 1), there's everyone. Then we filter down by interest/ specialization/ location/ class/ what have you. So in Group 2 we have fewer folks - only those that are into Group 2 stuff. Then we have Group 4 - a child of Group 2. It has even fewer folks. Super specialists.

In other words, in live situations, it seems that the dominant use case is one where someone (say A) joining a child should automatically be added to the parent. (of course, there needs to be a check so that if A is already a member of 1, then that doesn't throw an error when A joins 2).

Also, there would be some communities that would not want propagation and would want parents and children to operate independently. Thus, propagation should be a checkbox option.

What of activity streams? Same upwards propagation principle applies. As anyone who has used a CRM software can attest, if I post something in the activity stream of a contact person then that propagates up to the activity stream of the contact person's organization's activity stream.

#15 @sooskriszta
2 years ago

So, for instance:
University A uses BuddyPress for its Intranet.
Top level group is Professors.
2nd level groups are Schools: Graduate, Law, Business and Medicine.
Subgroups are departments, e.g. at Business School: Accounting, Finance, Marketing, Operations and Organizational Behavior.

The University would want everyone joining Finance group to also be a part of the Business School group and Professors group.

It may switch activity propagation off or on. If it turns activity propagation on then the discussions at Finance group percolate all the way up to the Professors group. If activity propagation is off then they don't.

But if there is an announcement that it needs everyone to read, the university only wants to post it once on Professors group and wants everyone to read. Well, hey, there is no need for the downwards propagation of activity stream - all professors are already propagated up and can read the announcements posted in the Professors group!

#16 @sooskriszta
2 years ago

  • Summary changed from Need hierarchical groups to Hierarchical groups

#17 @sooskriszta
2 years ago

  • Component changed from Core to Groups

#18 @sooskriszta
2 years ago

Would #4017 bring us closer to hierarchical groups?

#19 @boonebgorges
2 years ago

Would #4017 bring us closer to hierarchical groups?

Not strictly speaking, though you could definitely use group taxonomies to fake the parent/child relationship. (A truly general solution would be for WP to adopt a relationships API. See https://core.trac.wordpress.org/ticket/14513.)

@abweb
5 months ago

Archive contains patch file and required files

#20 @dcavins
7 weeks ago

  • Milestone changed from Future Release to 2.7
  • Owner set to dcavins
  • Severity changed from major to normal
  • Status changed from new to assigned

I intend to explore adding the basics of hierarchical groups for 2.7. I will not be addressing any sort of member propagation, only relationships and the groups query (the core bits).

#21 @dcavins
6 weeks ago

I'm attaching a first patch for adding hierarchical groups in BuddyPress. This includes the following changes:

  • New parent_id column to the groups table in the database.
  • Setting for enabling hierarchical groups.
  • Per-group setting for parent group.
  • Per-group setting for what kind of member can create subgroups of the group.
  • Functions for finding the parent ID, ancestor group IDs and descendent groups.
  • A bp_user_can() filter for whether a user can create subgroups of a particular group.
  • New screen to show subgroups of a group at /group-name/hierarchy.
  • Breadcrumb template function, bp_group_permalink_breadcrumbs().
  • Tests.

What this patch does not do:

  • Change the group directory to somehow display hierarchical relationships.
  • Change the url for a group to be /groups/parent-group/this-group/
  • Any kind of membership propagation.

Other notes:

  • bp_groups_get_child_groups(), bp_groups_get_child_group_ids(), and bp_groups_has_children() would all be unnecessary if the group visibility pieces from #6677 were added to BP. Then, you could just use groups_get_groups() or bp_has_groups() with dynamic group visibility, now that parent_id support is added in BP_Groups_Template.

Please test and give feedback. I'll also attach screenshots of some of the controls below.

Thanks in advance for your thoughts and criticism.

@dcavins
6 weeks ago

First patch

@dcavins
6 weeks ago

Main group hierarchy setting in BP options screen.

@dcavins
6 weeks ago

Single group hierarchy options screen, front end.

@dcavins
6 weeks ago

Single group hierarchy options screen, wp-admin.

@dcavins
6 weeks ago

New subgroup screen for a group.

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


6 weeks ago

#23 @hnla
6 weeks ago

  • Cc hnla added

#24 follow-up: @mercime
5 weeks ago

@dcavins Wow! That's :alot: of work there :) Thank you.

Took this out for a spin a few minutes ago in a regular WP installation :)

  • Re Parent Group select in wp-admin and front-end: only 20 groups show up in the select box out of the 76 expected (77 groups less the group you're editing).
  • Suggest removal of the "Member Groups" link from a group's navigation if it is not a Parent Group.
  • Add missing explicit label for Parent Group's select element in front end and back end.
  • Re "Member Groups" - I can imagine that those who have been using the Group Hierarchy plugin would already know what "Member Groups" refers to. The terminology could be confusing to new users who could think that it's a list of groups where the members of the specific group are also members of.
Last edited 4 weeks ago by mercime (previous) (diff)

#25 in reply to: ↑ 24 ; follow-ups: @dcavins
5 weeks ago

Replying to mercime:
Thanks for the test drive! Comments on your comments below.

@dcavins Wow! That's :alot: of work there :) Thank you.

Took this out for a spin a few minutes ago in a regular WP installation :)

  • Re Parent Group select in wp-admin and front-end: only 20 groups show up in the select box out of the 76 expected (77 groups less the group you're editing).

Ha! Pagination! I'd really like to improve on the select anyway by making it an AJAX autocomplete type thingy. My main site has 600ish groups, so a select is pretty terrible.

  • Suggest removal of the "Member Groups" link from a group's navigation if it is not a Parent Group.

I've followed BPGH's logic here: Show the tab if subgroups exist or if the current user can create subgroups (BPGH includes a "create" link on that screen). It seems a bit subtle for me, but BPGH also makes it possible to restrict who can create top-level groups and who can create subgroups, so I guess it's useful in that case where the current user can't access the regular "create group" button, but could create a subgroup. Do we want that kind of granularity?

  • Add missing explicit label for Parent Group's select element in front end and back end.

Great!

  • Re "Member Groups" - I can imagine that those who have been using the Group Hierarchy plugin would be already know what "Member Groups" refers to. The terminology could be confusing to new users who could think that it's a list of groups where the members of the specific group are also members of.

I too am conflicted. What should these objects be called?

Thanks very much for your comments! I'll work on a new iteration of this next week.

I'm still trying to figure out how I'd like to reflect hierarchy on the main groups directory--like BPGH with subgroup expands, simply including breadcrumbs with each result, or something else? The AJAX expand that BPGH uses seems to cause issues for users who expect to scan the list and find the group they're looking for, and never find second or deeper level groups because they don't show up. Maybe show them more like threaded comments with the option of closing groups of groups (could be a pretty expensive page load, and pagination would be odd)? Opinions welcome!

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


5 weeks ago

#27 in reply to: ↑ 25 @sooskriszta
5 weeks ago

Replying to dcavins:

Replying to mercime:

  • what "Member Groups" refers to. The terminology could be confusing to new users

I too am conflicted. What should these objects be called?

Subgroups?

#28 in reply to: ↑ 25 ; follow-up: @sooskriszta
5 weeks ago

Replying to dcavins:

I'm still trying to figure out how I'd like to reflect hierarchy on the main groups directory--like BPGH with subgroup expands, simply including breadcrumbs with each result, or something else? The AJAX expand that BPGH uses seems to cause issues for users who expect to scan the list and find the group they're looking for, and never find second or deeper level groups because they don't show up. Maybe show them more like threaded comments with the option of closing groups of groups (could be a pretty expensive page load, and pagination would be odd)?

How about megamenu style, i.e. open it as a hierarchical list. But no need to grab all the other elements like description, freshness, image, etc?

#29 in reply to: ↑ 28 @dcavins
4 weeks ago

Replying to sooskriszta:

Replying to dcavins:

I'm still trying to figure out how I'd like to reflect hierarchy on the main groups directory--like BPGH with subgroup expands, simply including breadcrumbs with each result, or something else? The AJAX expand that BPGH uses seems to cause issues for users who expect to scan the list and find the group they're looking for, and never find second or deeper level groups because they don't show up. Maybe show them more like threaded comments with the option of closing groups of groups (could be a pretty expensive page load, and pagination would be odd)?

How about megamenu style, i.e. open it as a hierarchical list. But no need to grab all the other elements like description, freshness, image, etc?

I'm personally a fan of simplification, but I think that presenting the groups as a nested text list would be a pretty extreme departure from what we're currently doing... Happily, if we can make @hnla's snippets library idea work, it would be a simple matter for a variety of styles for that directory to be easy to achieve. Thanks for your feedback!

#30 @hnla
4 weeks ago

Years back I had a client project that used Group Hierarchy extensively, styling that was fairly complex, in fact the functionality was too. We can certainly try running up a series of style options that can be applied via the modules approach.

#31 @dcavins
4 weeks ago

  • Keywords has-patch added

I'm uploading a new patch that incorporates much of the helpful feedback provided here.

  • Make sure the "parent" select is showing all possible groups by disabling pagination in the groups_get_groups() call.
  • Fixed accessibility problems with form elements.
  • Only show the "subgroups" tab if subgroups exist for that group. (This is a change from BPGH's behavior, but since I'm not proposing an override to the global bp_restrict_group_creation() for subgroups, it probably makes sense to always access the group creation screens from the same place.)
  • Changed language to "Subgroups". End users can change it to whatever with a language file, of course. ;)
  • Added hierarchy breadcrumbs to groups in the groups directory as the smallest possible change to the directory that shows hierarchical relationships. (Will attach image)
  • Take the setting bp_restrict_group_creation() into account when calculating who can create subgroups.
  • Add the parent setting to the settings screen for existing and in-creation groups.
  • Don't show impossible options on the "who can create subgroups" setting if group creation is globally restricted.

I was thinking of building a walker to display the groups hierarchically like threaded comments, but there are some technical hurdles that lead me to think that you couldn't use bp_has_groups() to fetch the groups in the common way, and I'm concerned that people who've gotten used to working with that loop would be in for a surprise. I think we'd have to fetch all the groups and let the walker work it out. We'd also have to base pagination on the number of top-level groups for the current directory (groups with a parent_id of 0 for the main directory and with the parent_id of the current group for a subgroup directory). These things are doable but a major departure. If we wish to go this route, I think we should add a new template group-tree.php or similar because it will work very differently. (I'm happy to work on this, but I don't want to go down a path that we think is a bad idea.)

@dcavins
4 weeks ago

Groups directory with hierarchy breadcrumbs.

@dcavins
4 weeks ago

Second round.

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


4 weeks ago

#33 @dcavins
4 weeks ago

This ticket was the subject of useful and interesting conversation in the weekly meeting on August 3: https://wordpress.slack.com/archives/buddypress/p1470251765001059

#34 @dcavins
4 weeks ago

I'm attaching a minimal patch that adds no outward-facing functionality but adds the parent_id column to the groups database and support for querying by parent_id via groups_get_group(). These changes would make it much more straightforward to add hierarchical group functionality via a non-core plugin.

@dcavins
4 weeks ago

Minimal patch that makes schema changes and support for querying by parent_id via groups_get_groups()

#35 @DJPaul
4 weeks ago

3961.03.patch looks OK to me. I would suggest adding the groups prefix onto the new bp_update_orphaned_groups_on_group_delete function, but that's about it.

Let's have some other review, then all we need is a few unit tests to: prove the bp_has_groups change, and that the orphaned-parent-group tidy-up works.

I think @r-a-y mentioned in Slack that we needed to maybe clear some Group caches. Perhaps he'll chime in here how best we can do that.

#36 follow-ups: @DJPaul
4 weeks ago

@dcavins I am probably misreading this but:

$child_group = groups_get_group( array( 'group_id' => $cgroup->id ) );

That block. Isn't $cgroup already the $child_group ??

#37 in reply to: ↑ 36 @dcavins
4 weeks ago

Replying to DJPaul:

@dcavins I am probably misreading this but:

$child_group = groups_get_group( array( 'group_id' => $cgroup->id ) );

That block. Isn't $cgroup already the $child_group ??

I'm sure you're right. That was one place where I was using my new convenience functions that I removed between .02 and .03 patches. So I made the change quickly and made sure phpunit passed, but haven't read the minimal patch thoroughly for new weirdness. Thanks for your careful review!

#38 in reply to: ↑ 36 @dcavins
3 weeks ago

Replying to DJPaul:

@dcavins I am probably misreading this but:

$child_group = groups_get_group( array( 'group_id' => $cgroup->id ) );

That block. Isn't $cgroup already the $child_group ??

Ah. It's the group data, but not the BP_Groups_Group object that we need to call save() on.

Re your earlier suggestions:
I'm attaching a slightly different version of the minimal patch that reinstates the tests I had written to check the behavior of parent_id queries in the BP_Groups_Group class and the orphan-tidying function. (Somehow I left them out of the minimal patch, too.) Please let me know if there are other tests that you can think of--I'm happy to write them. I've also prefixed the function name to be consistent as suggested: bp_groups_update_orphaned_groups_on_group_delete().

Thanks again for taking a look at the patch.

@dcavins
3 weeks ago

Updated minimal patch that makes schema changes and support for querying by parent_id via groups_get_groups()

#39 @egyptimhotep
3 weeks ago

  • Cc egyptimhotep@… added

#40 @r-a-y
3 weeks ago

  • Keywords 2nd-opinion removed

3961.05.patch includes cache invalidation so older cached objects are invalidated and refetched at run-time in the BP_Groups_Group::populate() method.

Includes one unit test - test_invalidate_old_group_cache_after_db_schema_change().

@r-a-y
3 weeks ago

#41 @DJPaul
3 weeks ago

Ah. It's the group data, but not the BP_Groups_Group object that we need to call save() on.

Maybe that's something that could be looked at for being changed in the future

#42 @ubernaut
3 weeks ago

  • Cc ubernaut@… removed

#43 @DJPaul
3 weeks ago

  • Cc ubernaut@… added

The patch is looking really close. Question for @r-a-y:

The populate change you made to invalidate old caches. Doing this here strikes me a little odd, because this is basically a one-time (per group) only thing. Once all the groups have been cleared, it'll never get used again.

Did you consider fetching all Group IDs during the database upgrade routine, and clearing those there?
We don't need to worry about re-building the cache there, and I'd even suggest a raw SQL query to fetch the IDs if groups_get_groups doesn't support such an optimal query (I haven't looked at its query builder for a while).

#44 @DJPaul
3 weeks ago

  • Cc ubernaut@… egyptimhotep@… removed

#45 @DJPaul
3 weeks ago

  • Cc egyptimhotep@… added

sorry for messing up the CCs

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


3 weeks ago

#47 @r-a-y
3 weeks ago

The populate change you made to invalidate old caches. Doing this here strikes me a little odd, because this is basically a one-time (per group) only thing.

Writing up some upgrade code would probably take up more lines and that's also a one-time use case.

My small addition means that the cache is only purged when needed. Also, looping through all the groups can be intensive depending on the number of groups. Our upgrader isn't mature enough to handle taxing situations yet (#6841).

#48 @DJPaul
3 weeks ago

The upgrader code is all about being used once, right? update_to_x_version is only called if you're on an older version.

The performance concern is why I said a direct SQL query to only return the IDs avoids any of the (unintentional) overhead of using the existing groups_get_group function*. It also avoids out-of-date cached information being returned, when the whole point is to clear the cache. It's about the only place I'd ever use a direct SQL query.

Unless someone is going to commit to "fixing" the upgrader code in 2.7 or 2.8 -- of which I've seen no significant interest in recently -- worrying about giant-scale BP sites, is beginning to feel like an edge case excuse.

(*The function may already have great SQL for returning IDs only, and for forcing a query instead of using a cache. I haven't looked.)

#49 @dcavins
3 weeks ago

I think @r-a-y's invalidation scheme makes sense and wouldn't be costly. If we must do it at upgrade, the simplest code is probably:

<?php
function bp_update_to_2_7() { 
        bp_add_option( 'bp-emails-unsubscribe-salt', base64_encode( wp_generate_password( 64, true, true ) ) ); 
        /* 
         * Also handled by `bp_core_install()`. 
         * Add `parent_id` column to groups table. 
         */ 
        if ( bp_is_active( 'groups' ) ) {
                bp_core_install_groups();
                
                // Invalidate all cached group objects.
                global $wpdb;
                $bp = buddypress();

                $group_ids = $wpdb->get_column( "SELECT id FROM {$bp->groups->table_name}" );
                foreach ( $group_ids as $group_id ) {
                        wp_cache_delete( $group_id, 'bp_groups' );
                }
        } 
} 

which seems pretty straightforward, but I don't know how efficient wp_cache_delete() is. @r-a-y or @boonebgorges probably know something about it. :)

#50 @DJPaul
3 weeks ago

wp_cache_* are not sets of functions you ever need to worry about performance with.

#51 @ubernaut
3 weeks ago

  • Cc ubernaut@… added

#52 @ubernaut
3 weeks ago

  • Cc ubernaut@… removed

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


8 days ago

Note: See TracTickets for help on using tickets.