Skip to:
Content

Opened 11 months ago

Closed 11 months ago

Last modified 11 months ago

#7419 closed enhancement (fixed)

Use BP_Groups_Group::get() where possible.

Reported by: dcavins Owned by: dcavins
Milestone: 2.8 Priority: normal
Severity: normal Version: 2.7.2
Component: Groups Keywords: has-patch
Cc: dcavins

Description

I'm fairly confident that BP_Groups_Group::filter_user_groups(), BP_Groups_Group::search_groups(), BP_Groups_Group::get_random(), and BP_Groups_Group::get_by_letter() (after #7418) can all be replaced by references to BP_Groups_Group::get() without any negative consequences.

I'm attaching a patch that builds on the change put forth in #7418 to do that.

Attachments (1)

7419.1.patch (11.5 KB) - added by dcavins 11 months ago.
Rewrite BP_Groups_Group::filter_user_groups, BP_Groups_Group::search_groups, BP_Groups_Group::get_random, and BP_Groups_Group::get_by_letter to use BP_Groups_Group::get().

Download all attachments as: .zip

Change History (8)

@dcavins
11 months ago

Rewrite BP_Groups_Group::filter_user_groups, BP_Groups_Group::search_groups, BP_Groups_Group::get_random, and BP_Groups_Group::get_by_letter to use BP_Groups_Group::get().

#1 @Mamaduka
11 months ago

@dcavins nice work here. Had similar patch in mind for a while, but never get time to actually write it.

I think it would be good idea to deprecate those methods as well. Most of them where introduce in first version of BP, before it was converted to plugin.

#2 @dcavins
11 months ago

  • Owner set to dcavins
  • Status changed from new to assigned

Thanks @Mamaduka. Those functions do seem like good candidates to deprecate, and I wonder if updating them actually is the wrong thing to do if we're going to deprecate them.

It always seems easier to rewrite rather than remove functions to me, because you never know how real users are using the code. :)

#3 @Mamaduka
11 months ago

@dcavins I think we can update them to use BP_Groups_Group::get() and deprecate them as well. Have seen similar cases in core before (e.g. see bp_activity_get_sitewide()).

We can't fully move methods into deprecated files, like we do with functions. Unless we introduce __callStatic magic method (PHP >= 5.3) and handle argument logic there. Probably not worth of dev time IMO.

#4 @DJPaul
11 months ago

Our previous strategy of moving deprecated code to those separate files doesn't work. We have bits of BuddyPress that still use those deprecated functions. The effort a release or two ago to stop new installs loading those files backfired because of the above, and right now, every site loads those files.

By all means continue to deprecate things and add the notices etc but don't bother moving it into those /bp-core/deprecated/ files. We'll need to discuss sometime how to approach deprecated documentation.

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


11 months ago

#6 @dcavins
11 months ago

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

In 11384:

BP_Groups_Group: Refactor some fetching functions.

Rewrite BP_Groups_Group::filter_user_groups,
BP_Groups_Group::search_groups, BP_Groups_Group::get_random, and
BP_Groups_Group::get_by_letter to use BP_Groups_Group::get(). Using
get() means that incrementor-based caching will be applied to the
queries and that get_group_extras() can be removed in several places.

Fixes #7419.

#7 @dcavins
11 months ago

  • Milestone changed from Awaiting Review to 2.8
Note: See TracTickets for help on using tickets.