Skip to:
Content

Opened 3 years ago

Last modified 21 months ago

#6413 assigned defect (bug)

xprofile fielddata visibility should be stored in xprofilemeta

Reported by: boonebgorges Owned by: boonebgorges
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Extended Profile Keywords: 2nd-opinion
Cc: johnjamesjacoby

Description

Per-user visibility levels are currently stored as a serialized array in usermeta. This is clunky, and it makes it impossible to do intelligent queries in pure SQL. See #6211 for an example. Let's move it to xprofilemeta where it belongs.

Attachments (1)

6413.diff (9.1 KB) - added by boonebgorges 2 years ago.

Download all attachments as: .zip

Change History (11)

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


2 years ago

#2 @boonebgorges
2 years ago

  • Milestone changed from Future Release to 2.5
  • Owner set to boonebgorges
  • Status changed from new to assigned

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


2 years ago

#4 @johnjamesjacoby
2 years ago

  • Cc johnjamesjacoby added

Tangentially related: #6350, #6347.

@boonebgorges
2 years ago

#5 @boonebgorges
2 years ago

  • Keywords 2nd-opinion added

6413.diff is a partial pass at the modifications to xprofile, along with the necessary migration routine. I still have to adapt the logic of hiding fields based on visibility in BP_XProfile_Group.

I wanted to post the partial patch because I'm not feeling very good about how the migration will work. In 6413.diff, I've used a cron technique: during upgrade, a cron job is scheduled, which will run every minute until the callback detects that the migration is done. It's currently migrating 25 users at a time. This number could be increased, but not, I think, by very much. Even at 100 per batch, it could take hours for large sites to be migrated. This seems bad. On the other hand, if we don't use wp-cron, then we need to either:

  1. come up with either a UI (like: click a button, fire an AJAX request, see the progress in a progressbar, try to support no-js? etc) or,
  2. come up with some new technique for background processes. Maybe loopback requests (which wouldn't have to wait a full minute like our cron job) or maybe something like https://deliciousbrains.com/background-processing-wordpress/

This particular migration routine is not the only place where this issue will arise in the future. Moving Favorites to a relationship table seems like another place where the load may be too large to do in a single batch. So let's come up with a strategy that we can reuse in the future.

#6 @imath
2 years ago

Thanks a lot for your searches and for the patch about this particular concern. I agree we need to find a nice way to handle upgrade routines.

If i understand correctly, 1000 users would be upgraded in 40 minutes with this technic. Which is pretty long! So i guess the upgrade routine is not the only problem we'll have to deal with. While the user is not upgraded yet, the profile fields are visible to everyone or hidden to everyone ? I guess in large communities there's a good chance someone tries to see your profile while the routine is processing..

What's a bit annoying is the lack of feedback to the admin about how many users have been upgraded, how many are not yet..

I think it will be difficult to perform a "quicker" upgrade routine without using javascript (wp-background-processing uses Ajax, WordPress Multisite Upgrade routine reloads the page after a timeout, and i think bbPress converters are also using Ajax requests)

So i'm in favor of:

  • javascript/Ajax + a transient to eventually restart where an upgrade was interrupted a bit like what wp-background-processing seems to do.
  • a UI to let the admin trigger the upgrade and to inform him about the progress of the upgrade
  • ideally a way to handle things while the upgrade has started but is not completed yet (eg: display or not the content of the profile field, or use the usermeta till it exists...)

#7 @boonebgorges
2 years ago

@imath - Thanks for the feedback. I share your concerns about the slowness of the migration. Though, for what it's worth, *any* batch system we build means that there will be at least *some* lag time between BP upgrade and full migration. This means, in turn, that BP should always be able to handle both old and new data, at least for a version or two (until we can be reasonably certain that all installations will have performed the migration). I'll work on a patch that covers this backward compatibility. In this case, I think we can do it with lazy processing: when loading a user's profile, if we detect that the user's visibility settings have not yet been migrated as part of the upgrade routine, then we migrate the specific user's data before rendering the page. (This is possible in the case of profile visibility because this data isn't really used anywhere but inside of a bp_has_profile() loop. It'd be harder to do this with other kinds of data.)

I've opened #6841 to discuss the more general issue of data migrations and update routines. Would like to hear your thoughts there.

#8 @imath
2 years ago

+1 to the "lazy processing", great idea :)

#9 @boonebgorges
2 years ago

  • Milestone changed from 2.5 to 2.6

Pending #6841 :(

#10 @DJPaul
21 months ago

  • Milestone changed from 2.6 to Future Release
Note: See TracTickets for help on using tickets.