Opened 11 years ago
Closed 11 years ago
#5411 closed defect (bug) (fixed)
Members nav of single group : total members seems not set
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 2.0 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Groups | Keywords: | has-patch commit |
Cc: |
Description
In a single group the Members navigation includes a span to show the amount of members belonging to the group.
I think since changeset 7898 that this members count is no more set. As a result, the Members navigation keeps showing 0.
Attachments (4)
Change History (25)
#2
@
11 years ago
- Owner set to boonebgorges
- Resolution set to fixed
- Status changed from new to closed
In 7934:
#3
@
11 years ago
Actually, I changed my mind about moving the groupmeta calls :) My original thought was that it only takes a single SQL query to fetch all groupmeta, so there wasn't much harm in doing both of the groupmeta calls even when populate_extras was not true. However, I'm now thinking that there are still cases where it's not necessary to have this info at all, so why waste the DB call?
#4
@
11 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
r7934 defaults the 'populate_extras' parameter to true.
However, there are very few instances where we need the extra group data. We only need it to be true when on a single group or in a group loop.
This can save a lot of unnecessary queries since groups_get_group()
or new BP_Groups_Group
is called quite a bit.
02.patch reverts r7934 and fixes a few places where we should be using the current_group
object in BP_Groups_Template
.
#5
@
11 years ago
I agree that there are many places where we do not need the extra group data. However, I think that defaulting to populate_extras=false will break backward compatibility with plugins that may be depending on the populate_extras data to be there. So I think we'll have to do the opposite of what you've done here: pass populate_extras=false in the cases where it's *not* needed.
#6
@
11 years ago
However, I think that defaulting to populate_extras=false will break backward compatibility with plugins that may be depending on the populate_extras data to be there.
I just checked all wp.org plugins and only one plugin suffers from this change no plugin suffers from this change :).
bp-groups-suggestions
expects the group extras so the last active timestamp will work:
https://github.com/wp-plugins/bp-groups-suggestions/blob/9271125afe24e5b18f9f0d7fcbae9d6d563e679a/bp-group-suggest.php#L234
Update: bp_get_group_last_active()
is smart enough to grab the last active timestamp from groupmeta when it doesn't exist in the global.
FYI, if you're interested, here's what I used to search all wp.org plugins on Github:
https://github.com/search?q=user%3Awp-plugins+groups_get_group%28&type=Code
https://github.com/search?q=user%3Awp-plugins+BP_Groups_Group%28&type=Code
#7
@
11 years ago
Whoa, sweet. Never thought to use the github mirror for searching! Thanks for the tip!
You've convinced me :) I'm happy to go ahead with the proposed change, as long as the tests pass.
#8
@
11 years ago
I'm happy to go ahead with the proposed change, as long as the tests pass.
So there's one test that fails because it is expecting 'populate_extras'
to be true. I've attached a patch that alters the test to add the 'populate_extras'
parameter.
I hope this isn't a big enough dealbreaker so we'll have to do the opposite approach of the patch!
#9
@
11 years ago
- Keywords commit added
.02b looks good to me, r-a-y. It's testing something that only needs to be tested when 'populate_extras' is true. Thanks!
#11
@
11 years ago
Could this change be breaking $groups_template
object creation in BP Group Hierarchy? Here's an example of the $groups_template
object, captured within the function bp_group_list_admins()
(my BP is up to r8063).
BP_Groups_Hierarchy_Template Object ( [vars] => Array ( [params] => Array ( ) ) [current_group] => 0 [group_count] => 1 [groups] => Array ( [0] => stdClass Object ( [group_id] => 17 ) ) [group] => BP_Groups_Hierarchy Object ( [vars] => Array ( [parent_id] => 15 [true_slug] => captains-and-cormorants [path] => category-test/captains-and-cormorants ) [id] => 17 [creator_id] => 1 [name] => Captains and cormorants [slug] => category-test/captains-and-cormorants [description] => Especially for you other types [status] => private [enable_forum] => 1 [date_created] => 2013-08-08 14:58:56 [admins] => [mods] => [total_member_count] => [is_member] => [is_invited] => [is_pending] => [last_activity] => [user_has_access] => [args:protected] => ) [in_the_loop] => 1 [pag_page] => 1 [pag_num] => 20 [pag_links] => [total_group_count] => 1 [single_group] => 1 [sort_by] => [order] => )
Could be totally unrelated, but the problem appeared at about the same time.
#12
follow-up:
↓ 13
@
11 years ago
The problem can be traced back to the introduction of the 'populate_extras' parameter in r7898.
The BP_Groups_Hierarchy
class does not have this parameter when it constructs its group since this is brand-new.
What we could do is introduce backwards compatibility support to fill in the group admins and mods when they are empty in the bp_group_list_admins()
function, so when it is called, the group admin / mods will be shown.
Update: There are some other problems besides just fetching the group admin / moderators. I've just informed ddean of the issues here:
https://github.com/ddean4040/BP-Group-Hierarchy/issues/6
#13
in reply to:
↑ 12
@
11 years ago
Replying to r-a-y:
The problem can be traced back to the introduction of the 'populate_extras' parameter in r7898.
The
BP_Groups_Hierarchy
class does not have this parameter when it constructs its group since this is brand-new.
What we could do is introduce backwards compatibility support to fill in the group admins and mods when they are empty in the
bp_group_list_admins()
function, so when it is called, the group admin / mods will be shown.
Update: There are some other probems besides just fetching the group admin / moderators. I've just informed ddean of the issues here:
https://github.com/ddean4040/BP-Group-Hierarchy/issues/6
r-a-y: You're awesome. Thanks for looking into this.
#14
@
11 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
This makes me feel not so bad about being the backward compatibility ogre :)
r-a-y, while obviously it's good for ddean to update bp-groups-hierarchy (my guess for the most popular plugin that'll be affected by these changes), we should try to accommodate them as much as possible. I'd guess that your suggestion for bp_group_list_admins()
is going to be good enough for most purposes; hopefully not many people are attempting to access that part of the global directly.
Off topic, but what are the aspects of bp-group-hierarchy that are broken by the groupmeta changes? Is it just performance/caching issues? If there are breaking changes, could I ask you to open a separate ticket with details? The migration to WP metadata is supposed to be seamless :)
#15
@
11 years ago
I'd guess that your suggestion for bp_group_list_admins() is going to be good enough for most purposes; hopefully not many people are attempting to access that part of the global directly.
We can definitely do that.
However, the issue with BP Group Hierarchy is it already uses two arguments in its constructor class. However, BP_Groups_Group
just added a second parameter (r7898), so that causes some issues that could only be solved if ddean added a third parameter to his constructor or just ignores the . (I don't like the latter option I recommended!)populate_extras
parameter and pastes all the extra group data calls into his BP_Groups_Hierarchy::populate()
method
There might be other ways to solve the issue, but that's what I gathered from my quick audit of BP Group Hierarchy.
Off topic, but what are the aspects of bp-group-hierarchy that are broken by the groupmeta changes?
BP Group Hierarchy copies most of the BP_Groups_Component
class. Because of that, ddean will need to copy in the changes to add the meta_tables
parameter from r7840 since it won't be there in the current version of his plugin.
It's a simple fix.
#16
@
11 years ago
Ah, thanks, r-a-y. IIRC, we added the ability to override BP_Groups_Group just for ddean, so it's pretty unlikely that anyone else is using it. If you're overriding core classes, then caveat emptor I guess.
ddean will need to copy in the changes to add the meta_tables parameter
Gotcha. I guess that falls into the same category.
#17
@
11 years ago
Well, I ended up sending a pull request to ddean since I couldn't help myself!
If you're interested:
https://github.com/ddean4040/BP-Group-Hierarchy/pull/7
#18
@
11 years ago
Thanks, r-a-y :)
Would you mind writing a short bpdevel post that explains this situation, and maybe links to this ticket and/or your pull request? I feel like then we'll have done our due diligence for people who are doing wacky things with the groups component. (I also think we should do the admins_mods fix, as that may affect more people than just ddean.)
#19
@
11 years ago
list_admins_mods.01.patch
adds backpat support when using the bp_group_list_admins()
or bp_group_list_mods()
functions if 'populate_extras'
is false or does not exist.
One thing with the patch is I've set the $args
visibility in BP_Groups_Group
to public
so I can access it. It's either that or create a public method to access the protected property.
I'll write up a short bpdevel post a bit later about the groups component changes.
#20
@
11 years ago
Hi r-a-y - That patch seems fine. In my mind, I guess I'd been thinking something more along the lines of:
if ( empty( $group->admins ) ) { $group = groups_get_group( array( 'group_id' => $group->id, 'populate_extras' => true, ) ); }
This prevents us from having to examine arguments for original intent, and also keeps all the query logic centralized in the BP_Groups_Group class. But it's a fairly small difference from what you're doing (and anyway, it should rarely be triggered) so whatever you think is best is OK with me.
Thanks for doing this!
Good catch, thanks. Moving the groups_get_groupmeta() query is a good idea for a different reason (see #5398 and the change I'm about to commit). I'll do something a bit different to account for the current issue.