Skip to:
Content

BuddyPress.org

Opened 6 years ago

Closed 4 years ago

#7882 closed enhancement (fixed)

Stop mirroring user last_activity data to usermeta

Reported by: boonebgorges's profile boonebgorges Owned by: imath's profile imath
Milestone: 7.0.0 Priority: normal
Severity: normal Version: 2.0
Component: Members Keywords: has-patch commit
Cc:

Description

Previously: #5128

The primary storage location for user 'last_activity' is the activity table. For backward compatiblity, we mirror that data in usermeta. See https://buddypress.trac.wordpress.org/browser/tags/3.0.0/src/bp-members/bp-members-functions.php?marks=1002-1009#L976

Now that we are moving toward something like semver, we have more of an opportunity to phase this out. I suggest that in 4.0, we stop doing this mirroring by default, but allow plugins to reenable it via filter. We would then publish a notice on the bpdevel blog warning plugin authors about the change.

Attachments (3)

7882.diff (1.9 KB) - added by boonebgorges 6 years ago.
7882.02.patch (4.1 KB) - added by r-a-y 4 years ago.
7882.fix-unittest.patch (3.6 KB) - added by imath 4 years ago.

Download all attachments as: .zip

Change History (13)

#1 follow-up: @DJPaul
6 years ago

I think it’s time to stop the sync yes.

But how about add a filter to user meta that intercepts the request before it’s made, and we call activity meta and return that instead? Maybe that’s the filterable option rather than reenabling the sync.

#2 in reply to: ↑ 1 @boonebgorges
6 years ago

Replying to DJPaul:

I think it’s time to stop the sync yes.

But how about add a filter to user meta that intercepts the request before it’s made, and we call activity meta and return that instead? Maybe that’s the filterable option rather than reenabling the sync.

If I'm understanding your suggestion correctly, then we already do this: https://buddypress.trac.wordpress.org/browser/tags/3.0.0/src/bp-members/bp-members-functions.php?marks=1053,1043#L1014

But yes, if we have a "enable legacy mode" filter, we can have it apply to both writing and retrieving.

@boonebgorges
6 years ago

#3 @boonebgorges
6 years ago

  • Keywords has-patch added; 2nd-opinion good-first-bug needs-patch removed

7882.diff is a first pass. The mirroring is disabled by default, but can be reenabled.

I haven't changed anything about the way the get_user_meta( 'last_activity' ) behavior works. It still triggers _doing_it_wrong(), and it still returns the value from the activity table. I don't see a reason why the new filter would affect this. Even if you're not mirroring - perhaps *especially* if you're not mirroring - developers ought to see the notice, and I don't think it causes any problems to continue to provide the correct value out of the activity table.

#4 @boonebgorges
6 years ago

  • Milestone changed from 4.0 to Up Next

#5 @r-a-y
5 years ago

  • Keywords dev-feedback added

This will break legacy user queries that need the entry in the $wpdb->usermeta DB table in order to sort by last active or online:
https://buddypress.trac.wordpress.org/browser/tags/4.2.0/src/bp-core/classes/class-bp-core-user.php#L284
https://buddypress.trac.wordpress.org/browser/tags/4.2.0/src/bp-core/classes/class-bp-core-user.php#L341

Also the get_user_extras() method would need to be updated to fetch the timestamp from the activity DB table:
https://buddypress.trac.wordpress.org/browser/tags/4.2.0/src/bp-core/classes/class-bp-core-user.php#L721

It would almost be like rewriting the legacy user query to use the newer one, which would defeat the purpose of implementing this ticket! ☺

I think a better approach would be to use the existing 'bp_use_legacy_user_query' filter to determine if we should continue mirroring, instead of introducing a newer filter solely for this as in 7882.diff.

Version 0, edited 5 years ago by r-a-y (next)

#6 @boonebgorges
5 years ago

  • Keywords needs-patch added; has-patch removed

I think a better approach would be to use the existing 'bp_use_legacy_user_query' filter to determine if we should continue mirroring, instead of introducing a newer filter solely for this as in 7882.diff.

This approach sounds fine to me. The inline docs for that filter should be updated to reflect the fact that using the legacy query will force this data to be mirrored.

#7 @imath
4 years ago

  • Milestone changed from Up Next to 7.0.0

@r-a-y
4 years ago

#8 @r-a-y
4 years ago

  • Keywords has-patch added; needs-patch removed

02.patch only allows the 'last_activity' usermeta mirroring if the 'bp_use_legacy_user_query' filter is forced to true. I've added PHPDoc where appropriate.

One other thing I wanted to fix was the BP_Core_User::get_user_extras() method and how it should use the activity DB table instead of usermeta for last activity. This is now done as well.

#9 @imath
4 years ago

  • Keywords commit added; dev-feedback removed

Hi @r-a-y

I've just tested. It works as expected. Thanks a lot for your work on this. In 7882.fix-unittest.patch, I've added fixes for unit test ;)

You'll just need to include them into your commit 👍

#10 @imath
4 years ago

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

In 12754:

Members: stop mirroring user last_activity data to user metadata

The primary storage location for user last_activity is the activity table. For backward compatibility reasons, we used to mirror that data into user metadata.

Backward compatibility user last_activity metadata mirroring is now only done if the legacy user query is enabled. This can be achieved by forcing the bp_use_legacy_user_query filter to return true.

Props boonebgorges, r-a-y

Fixes #7882

Note: See TracTickets for help on using tickets.