[buddypress-trac] [BuddyPress Trac] #5220: Overhaul implementation of xprofile field types

[buddypress-trac]

#5220: Overhaul implementation of xprofile field types
------------------------------------------+-----------------------
 Reporter:  DJPaul                        |       Owner:  DJPaul
     Type:  enhancement                   |      Status:  assigned
 Priority:  normal                        |   Milestone:  2.0
Component:  XProfile                      |     Version:
 Severity:  normal                        |  Resolution:
 Keywords:  has-patch dev-feedback early  |
------------------------------------------+-----------------------

Comment (by r-a-y):

 Finally got a chance to test this and like Boone and imath said, there's
 some really great stuff here!  Way easier for devs to add new xprofile
 types!

 I came across a problem with the new `'number'` field not being able to
 accept 0 as a value.

 The majority of the problem lies during the saving of the xprofile data
 and not Paul's code.

 To fix this problem:

 - I've altered `BP_XProfile_Field_Type_Number` so it accepts null values
 - The xprofile screen function was setting
 [https://buddypress.trac.wordpress.org/browser/tags/1.9.2/bp-xprofile/bp-
 xprofile-screens.php#L107 empty values as an array].  This led to the
 value being saved as a serialized array instead of integer zero.  To fix
 this, I moved this weird behavior out of the screen function and into
 `xprofile_set_field_data()` where the field type is being called.  There,
 I do an explicit field type check for either a checkbox or a
 multiselectbox, then set the value as an array if it matches these
 conditions.  I did this based on the PHP comments.
 - Finally, in `BP_XProfile_ProfileData::save()`, I removed the `empty()`
 check while leaving the `strlen()` check.  There are probably reasons why
 an empty check was there in the first place, but this was causing problems
 with saving integer zero.  This can cause other unintended problems,
 however if we don't like this, we can also change the empty() check to
 `$this->value != ''`.

 In my mind, we should move most of the logic from
 `xprofile_set_field_data()` to `BP_XProfile_ProfileData::save()` since
 Paul's `BP_XProfile_Field_Type` class has a validation routine that is
 actually useful before saving xprofile field data.  We should then remove
 `BP_XProfile_ProfileData::save()`'s stringent requirements on updating an
 existing field and removing an existing field and let Paul's class handle
 validation exclusively.

 Note: The included unit test will pass even without 04b.patch.  This is
 because I can't emulate what happens during the screen function's form
 submission routine before it gets passed to `xprofile_set_field_data()`.

 ----

 Some other things I encountered that are not regressions, but are bugs.

 - Date field cannot reset back to nothing
 - There are no inline error messages if validation fails for a specific
 field.  Unfortunately, after we save the fields when editing the profile,
 we use `bp_core_redirect()` so we can't use hooks.  I tried working out a
 patch that addressed this, but didn't have enough time to work out the
 kinks.  Either way, this should be placed in another ticket anyway and not
 this one.

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