[buddypress-trac] [BuddyPress Trac] #5553: BP 2.0 upgrade routine improperly deletes existing user roles if activation_key usermeta is present

[buddypress-trac]

#5553: BP 2.0 upgrade routine improperly deletes existing user roles if
activation_key usermeta is present
-----------------------------------+--------------------
 Reporter:  boonebgorges           |       Owner:
     Type:  defect (bug)           |      Status:  new
 Priority:  highest                |   Milestone:  2.0.1
Component:  Core                   |     Version:  2.0
 Severity:  critical               |  Resolution:
 Keywords:  has-patch 2nd-opinion  |
-----------------------------------+--------------------

Comment (by boonebgorges):

 Hi all - thanks for your attention to this ticket.

 Let's back up for a second here and reassess the problem we're trying to
 solve. We are migrating old-style unactivated signups to the new schema,
 and we want to make sure that we do this in as reliable a way as possible.
 "Reliable" is a tricky concept here. Obviously, we want to make sure that
 we successfully migrate as many items as is reasonably possible. However,
 we should err on the side of caution, because false positives - migrations
 that should not have been performed - have serious consequences.

 In contrast, false *negatives* - cases where we fail to migrate an
 unactivated signup - are not nearly as serious. Practically speaking, the
 worst that can happen here is that a signup will not show up in the new
 Pending Users interface. Not ideal, but obviously not a dealbreaker -
 activation should still work fine.

 And it's worth remembering that these false negatives are always the
 result of some other plugin doing some sort of weird stuff (such as the
 bp-disable-activation plugin I describe above). So there's no way we can
 predict the negative side effects of false negatives.

 So, while I understand the motivation behind imath's additions, I think
 they're only asking for trouble. As DJPaul notes, deleting metadata is
 dangerous stuff, and in this case it would serve no purpose from BP's
 point of view - BP 2.0+ doesn't care about `activation_key` in usermeta
 anymore - while it could cause bugs in other plugins that expect that data
 to be there (which are, very likely, the plugins that caused the false
 positive in the first place). Likewise, since BP only looks in wp_signups
 for activation_key values, there's no point in copying anything over to
 wp_usermeta. Our job is not necessarily to clean up after other plugins,
 it's just to make sure that we don't screw up our own data because of what
 other plugins do.

 On that note, 5553.02.patch looks like the prudent route to me.

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