[buddypress-trac] [BuddyPress] #4636: Reinstate activity item for user profile changes

[buddypress-trac]

#4636: Reinstate activity item for user profile changes
--------------------------+------------------
 Reporter:  DJPaul        |       Owner:
     Type:  defect (bug)  |      Status:  new
 Priority:  normal        |   Milestone:  1.8
Component:  XProfile      |     Version:  1.1
 Severity:  normal        |  Resolution:
 Keywords:  needs-patch   |
--------------------------+------------------

Comment (by boonebgorges):

 Thanks for the patch, ericlewis.

 In general, I'm a big fan of returning `WP_Error` objects rather than a
 dummy `false` value. It's possible (however unlikely) that changing the
 return value of these functions will break something else, though. For
 example, if someone runs a check like this:

 `$success = xprofile_set_field_data( $foo, $bar, $baz );
 if ( false === $success ) { // or even ! $success
    echo 'oh noez';
 }`

 It's fairly unlikely that we're doing anything like this in BuddyPress,
 but could you please grep through the codebase to be sure? It's almost
 certain that we'll cause problems with at least some plugins, but IMO it's
 worth it in this case, because the `WP_Error` objects are just so much
 better (though I'd like to hear another dev chime in with an opinion on
 that).

 I'm particularly concerned about your 'same_value' return value. This is a
 case where the function currently returns *true*. Check this out:
 https://buddypress.trac.wordpress.org/browser/trunk/bp-xprofile/bp-
 xprofile-classes.php?annotate=blame#L1033 When the value exists, it'll hit
 the first `if` case (`UPDATE...`). Even when the $value is the same as
 previously, the $last_updated value is *different*, so the UPDATE query
 returns 1 rather than 0. (Side note: in what is, I believe, a bug
 introduced in r5643 #4930, the function returns true *even if the
 last_updated value is the same*. It seems like a bug to me because in this
 case no "update" has taken place, but then again `update_metadata()` also
 appears to return true in similar cases, so maybe I have the wrong
 intuition here.)

 I know you need this functionality to make your activity logic work. So,
 suggestions welcome. One is to do something like this inside of
 xprofile_set_field_data():

 {{{
 // Before doing anything, get the existing value
 $prev_value = xprofile_get_field_data( $field, $user_id );

 // ... do all of the update logic, then at the end of the function

 $updated = $field->save();
 do_action( 'xprofile_set_field_data', $updated, $field, $user_id, $value,
 $is_required, $prev_value );
 return $updated;
 }}}

 Then you can hook your activity logic to the `'xprofile_set_field_data'`,
 and check $value against $prev_value. This maintains backward
 compatibility for all current return values. (It also makes your
 `WP_Error` improvements irrelevant for the current task, though I like
 them independently.) Other ideas are welcome.

 A general note on your use of `bp_activity_add()`. We definitely want to
 keep this out of the database class. Use a hook, and if you don't have the
 hook that you need in the place that you need it, create a new hook (like
 I did in my suggestion above).

 Thanks for your work on this!

-- 
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/4636#comment:7>
BuddyPress <http://buddypress.org/>
BuddyPress