[buddypress-trac] [BuddyPress Trac] #7130: bp_notifications_get_all_notifications_for_user gets all records and can cause memory leak

[buddypress-trac]

#7130: bp_notifications_get_all_notifications_for_user gets all records and can
cause memory leak
-------------------------------------+------------------
 Reporter:  hberberoglu              |       Owner:
     Type:  defect (bug)             |      Status:  new
 Priority:  normal                   |   Milestone:  3.0
Component:  Toolbar & Notifications  |     Version:
 Severity:  normal                   |  Resolution:
 Keywords:  has-patch                |
-------------------------------------+------------------

Comment (by mauteri):

 If we did something like this, it would make more sense to introduce a
 finer-grained cache key for notification queries, as suggested by
 @mauteri.

 @boonebgorges should i open another ticket for this in that case? This is
 on the extreme, but I did a comment import without removing the new-
 comment notification hook and had some users that had 5 figures of
 notifications :-O Site ran out of memory and fatal error at the header
 when user logged in. Not putting a limit on things never seems like a
 great idea especially when you consider large sites.

 The grouping is problematic, as limiting makes the call inaccurate,
 however, in reality having that many unread notifications seems to be
 unlikely for most users. Maybe that upper limit should be 500 or even 1000
 rather than 100 as you suggested. I do think there should be an upper
 limit here. Also to go along with a finer-grain cache key, we can apply a
 new filter before the ksort, so if BuddyPress limits the number of
 notifications returned by default, developers can easily override it.

--
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/7130#comment:14>
BuddyPress Trac <http://buddypress.org/>
BuddyPress Trac