Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 2 years ago

#6218 closed enhancement (maybelater)

bp_friend_get_total_requests_count() could use already cached data

Reported by: wpdennis Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Friends Keywords: needs-patch, needs-unit-tests, trac-tidy-2018
Cc: info@…

Description

bp_friend_get_total_requests_count() calls BP_Friends_Friendship::get_friend_user_ids( $user_id, true ):

return apply_filters( 'bp_friend_get_total_requests_count', count( BP_Friends_Friendship::get_friend_user_ids( $user_id, true ) ) );

But get_friend_user_ids() never caches the result. If it would use get_friendship_request_user_ids() instead, it would access already cached data:

return apply_filters( 'bp_friend_get_total_requests_count', count( BP_Friends_Friendship::get_friendship_request_user_ids( $user_id ) ) );

Change History (9)

#1 @DJPaul
5 years ago

  • Milestone changed from Awaiting Review to 2.3

I *think* you're right. There's some logic in the suggested function that I think covers the use case of the first. If so, we probably should see if it's appropriate to deprecate one of the functions and convert it into a wrapper.

This would be another nice cache win for 2.3, so I'm moving it, as it doesn't seem too complicated to do.

#2 @Mamaduka
5 years ago

  • Keywords dev-feedback added

Caching in get_friendship_request_user_ids() was introduced in r8105, as a part of object cache improvement for friends component.

If we're going to deprecated one of those function, I think get_friendship_request_user_ids() should be our choice. Since BP_Friends_Friendship::get_friend_user_ids() can return same values and more. But this gonna make caching in later function bit complicated, where we should change cache group based on args.

I would love to here hear 2nd option on this as well.

P.S. What wpdennis suggests is pretty straightforward and happy to provide patch for it.

Last edited 5 years ago by Mamaduka (previous) (diff)

#3 @boonebgorges
5 years ago

  • Keywords needs-patch needs-unit-tests added; 2nd-opinion dev-feedback removed

If we're going to deprecated one of those function, I think get_friendship_request_user_ids() should be our choice. Since BP_Friends_Friendship::get_friend_user_ids() can return same values and more. But this gonna make caching in later function bit complicated, where we should change cache group based on args.

Agreed. The caching will be more complicated but will have benefits beyond this use case. If we're going to add caching here, we'll need a decent set of unit tests to demonstrate invalidation on new requests, defriending, user spam/delete, etc.

#4 @johnjamesjacoby
5 years ago

mamaduka: if you don't mind putting a patch together (with some tests) that would be very awesome of you.

I think the direction Boone is suggesting makes sense, though if it's easier to keep it broken up into multiple methods while this is being enhanced, I'm okay with that approach too, and we can always polish it up to be beautiful later, once we've confirmed the individual pieces are working correctly. Whatever is easier, really.

#5 @r-a-y
5 years ago

  • Milestone changed from 2.3 to 2.4

#6 @Mamaduka
5 years ago

Thanks @r-a-y for moving this to 2.4. Didn't get much time to work on this patches. Plus change require additional unit tests. Will make sure this one gets in core for 2.4 cycle.

#7 @DJPaul
4 years ago

  • Milestone changed from 2.4 to Future Release

#8 @DJPaul
2 years ago

  • Keywords trac-tidy-2018 added

We're closing this ticket because it has not received any contribution or comments for at least two years. We have decided that it is better to close tickets that are good ideas, which have not gotten (or are unlikely to get) contributions, rather than keep things open indefinitely. This will help us share a more realistic roadmap for BuddyPress with you.

Everyone very much appreciates the time and effort that you spent sharing your idea with us. On behalf of the entire BuddyPress team, thank you.

If you feel strongly that this enhancement should still be added to BuddyPress, and you are able to contribute effort towards it, we encourage you to re-open the ticket, or start a discussion about it in our Slack channel. Please consider that time has proven that good ideas without contributions do not get built.

For more information, see https://bpdevel.wordpress.com/2018/01/21/our-awaiting-contributions-milestone-contains/
or find us on Slack, in the #buddypress channel: https://make.wordpress.org/chat/

#9 @DJPaul
2 years ago

  • Milestone Awaiting Contributions deleted
  • Resolution set to maybelater
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.