Skip to:
Content

BuddyPress.org

Opened 9 years ago

Closed 9 years ago

#3312 closed enhancement (fixed)

Allow replacement of groups object in group component loading

Reported by: ddean Owned by:
Milestone: 1.5 Priority: normal
Severity: Version: 1.5
Component: Groups Keywords:
Cc:

Description

Following up on: http://buddypress.org/community/groups/bp-group-hierarchy/forum/topic/issues-with-bp1-3-trunk-development-version/

A filter in bp-groups-loader.php that would allow $bp->groups->current_group to be replaced or altered before a 404 is sent would allow cleaner extension of the Groups component.

Attached is a patch offering one option for doing this.

Attachments (4)

bp-groups-loader.php.patch (735 bytes) - added by ddean 9 years ago.
groups_object filter proposal
bp-groups-loader.new-filter-name.patch (743 bytes) - added by ddean 9 years ago.
groups_object filter proposal - new filter name
3312.diff (868 bytes) - added by boonebgorges 9 years ago.
bp-groups.patch (1.6 KB) - added by ddean 9 years ago.
groups filter proposal - function-based

Download all attachments as: .zip

Change History (18)

@ddean
9 years ago

groups_object filter proposal

#1 follow-up: @boonebgorges
9 years ago

This approach seems sensible to me. Can we mention 'current' in the filter name? Maybe 'groups_get_current_group_object'

#2 @boonebgorges
9 years ago

  • Milestone changed from Awaiting Review to 1.3

#3 follow-up: @DJPaul
9 years ago

Is the objective to change which class is used, or to be able to modify the instantiated object?

#4 in reply to: ↑ 1 @ddean
9 years ago

Replying to boonebgorges:

This approach seems sensible to me. Can we mention 'current' in the filter name? Maybe 'groups_get_current_group_object'

That sounds great. It would better indicate the use of the resulting object.

#5 in reply to: ↑ 3 @ddean
9 years ago

Replying to DJPaul:

Is the objective to change which class is used, or to be able to modify the instantiated object?

My objective is to change which class is used.

@ddean
9 years ago

groups_object filter proposal - new filter name

#6 @DJPaul
9 years ago

Then it needs to filter the *name* of the class to create, not the created object.

#7 @boonebgorges
9 years ago

Then it needs to filter the *name* of the class to create, not the created object.

Right. But we don't filter class or function names anywhere else in BP or WP (to the best of my knowledge). If you filter the group object, then you can still replace the current group object - you'll just be wasting a few CPU cycles and a DB hit or two.

In any case, I'm fine with a filter on the class name if everyone agrees that it's an OK precedent.

#8 @ddean
9 years ago

There are pros and cons to each way. This looks like the state of the discussion:

Filter instantiated object:

Pros

  • more consistent with other BP and WP filters
  • allows plugin authors to modify the group object without subclassing or creating a replacement BP_Groups_Group class

Cons

  • uses more system resources

Filter class name:

Pros

  • uses fewer system resources
  • a better match for this case

Cons

  • requires plugin authors to subclass or replace the BP_Groups_Group class in order to use the filter
  • breaks with convention for filters in BP and WP

#9 @boonebgorges
9 years ago

Thanks, ddean. That seems like a fair assessment. All things being equal, I'd prefer to go with filtering the object, for the reasons stated (especially the idea that it won't be necessary to create a BP_Groups_Group subclass, as WP plugin development is generally not required to maintain OOP principles). DJPaul, do you have any strong feelings?

#10 @DJPaul
9 years ago

Requiring plugin authors to subclass BP_Groups_Group, or use a little OOP, is in no way a downside. We're going to expect certain methods and functions in that class, and if they are not implemented in a custom implementation, we're going to get bug reports which aren't our fault.

If the aim is to change parts of the existing class, sure, use apply_filters_ref_array. If it's to load in your own class, filter the class name before creation. Need a similar filter in similar places throughout BP. Up to you.

#11 @boonebgorges
9 years ago

I can see use cases each way. Can we go with 3312.diff, which is an implementation of my rallying cry "filters for everyone"?

@boonebgorges
9 years ago

#12 @ddean
9 years ago

I was digging around and realized that the bp-groups-functions file might be a better home for this filter. Putting it in groups_get_group() casts a wider net, which goes with DJPaul's idea of having similar filters in other places.

Or is it bad form to assume that BP_Groups_Component::_includes() will have run by the time the globals setup is running?

I'm attaching a proposal.

@ddean
9 years ago

groups filter proposal - function-based

#13 @DJPaul
9 years ago

I prefer Boone's patch because the scope is narrower; if a plugin filtered into get_groups, it could perhaps affect another plugin, where there may be something bespoke going on. It fits the scope of this ticket better.

#14 @djpaul
9 years ago

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

(In [4735]) Allow replacement of groups object in group component. Fixes #3312, props boonebgorges and ddean

Note: See TracTickets for help on using tickets.