Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

#5590 closed defect (bug) (fixed)

Undefined offset when updating the last_activity

Reported by: imath Owned by: boonebgorges
Milestone: 2.1 Priority: normal
Severity: normal Version: 2.0
Component: Core Keywords: has-patch needs-unit-tests
Cc:

Description

I think it's linked to r8320

Seems the cached value should be the entire array and not only the value of the user_id index in BP_Core_User::get_last_activity()

Attachments (3)

5590.diff (563 bytes) - added by imath 5 years ago.
5590.02.diff (367 bytes) - added by imath 5 years ago.
5590.03.patch (908 bytes) - added by johnjamesjacoby 5 years ago.

Download all attachments as: .zip

Change History (10)

@imath
5 years ago

#1 @boonebgorges
5 years ago

  • Keywords needs-unit-tests added

The 'bp_last_activity' cache is already keyed by user ID. Storing the whole $retval array doesn't seem right. This needs further investigation and unit tests.

#2 @imath
5 years ago

  • Milestone changed from Awaiting Review to 2.0.1

Well, another way is to not try to update the activity using an index that is not set. See 5590.02.diff

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

@imath
5 years ago

#3 @boonebgorges
5 years ago

  • Milestone changed from 2.0.1 to 2.1

Are we sure this needs to be fixed in 2.0.1? If it was caused by r8320, it's not in the 2.0 branch.

Well, another way is to not try to update the activity using an index that is not set. See 5590.02.diff

This change would definitely need unit tests. I'd have to look closer to see how it would affect existing content, fetching for multiple users, etc.

#4 @imath
5 years ago

Oops, sorry i was wrong, i think i need a good night of sleep.

#5 @johnjamesjacoby
5 years ago

See #5597 for more details on this issue, closed as duplicate of this one since there's discussion here already.

Also see r8326 (committed to trunk) which tests subsequent calls to BP_Core_User::get_last_activity() to compare cached results to original non-cached results.

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

#6 @johnjamesjacoby
5 years ago

In 8327:

Add unit test for comparing subsequent calls to BP_Core_User::get_last_activity() to compare cached results to uncached results. See #5590, #5597, to test logic introduced in r8047. (2.0 branch)

#7 @boonebgorges
5 years ago

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

In 8332:

Overhaul caching for user last_activity values

The last_activity methods in the BP_Core_User class were originally designed
without persistent caching in mind. In r8047, some minimal caching support was
introduced, but it was tacked on and incomplete. In particular, it depended
on an inconsistent structure of return values from the last_activity methods.
See #5128 for more background.

This changeset introduces a number of changes that make last_activity methods
perform more consistently, as well as full support for object caching.

  • BP_Core_User::get_last_activity() always returns a multidimensional array of last_activity arrays, keyed by user_id. This remains true even when last_activity data is being fetched only for a single user.
  • last_activity data is stored in the bp_last_activity cache bucket without the user-id-keyed wrapper.
  • BP_Core_User::get_last_activity() now uses the same caching technique as other BuddyPress and WordPress queries: detect which requested items are not yet in the cache, prime the cache for those items, and then fetch all requested values directly from the cache to build a return value.

Fixes #5590

Props imath, johnjamesjacoby for initial patches

Note: See TracTickets for help on using tickets.