[buddypress-trac] [BuddyPress Trac] #6789: XProfile: do not store serialized arrays for multi-value profile field data

[buddypress-trac]

#6789: XProfile: do not store serialized arrays for multi-value profile field data
------------------------------+-------------------------------------
 Reporter:  Offereins         |       Owner:
     Type:  enhancement       |      Status:  new
 Priority:  normal            |   Milestone:  Awaiting Contributions
Component:  Extended Profile  |     Version:
 Severity:  normal            |  Resolution:
 Keywords:                    |
------------------------------+-------------------------------------

Comment (by boonebgorges):

 @Offereins Thanks for your work on this. It is exciting to see it taking
 shape. Overall, the decisions you've made seem smart to me. The handling
 of empty/falsey fields tested well for me, and the
 `supports_multiple_values` flag feels correct.

 I'm attaching a new patch [attachment:6789.06.patch] that includes updates
 to documentation, as well as some changes to work after recent changes to
 the treatment of serialized data throughout BP. See [11692].

 I have some random feedback. In no particular order.

 - The serialization dances are complicated, perhaps too complicated. For
 example: Data coming from multiple-value fields is being passed through
 `array_map( 'maybe_unserialize' )` in multiple places. In what situation
 is it possible for a multiselect or checkbox field to store serializable
 data for its individual options? Certainly it's not possible in BP itself,
 and I wonder if it's even possible to do it now with a custom field type.
 Likewise: if a field does *not* support multiple values, what is the value
 in running it through `maybe_unserialize()`? Have you seen examples of
 custom field types that are storing structured data in the profile_data
 table in this way? (Related: we will need to perform a review to ensure
 that `maybe_unserialize()` calls are always symmetrical; for security
 reasons, we should never run any user-provided data through
 `maybe_unserialize()` that didn't go through `maybe_serialize()` on the
 way in.)
 - You've added some missing filter documentation in
 `BP_XProfile_Data::save()`. It's unrelated to the ticket at hand, so I'd
 go ahead and commit it separately. No need to wait for this ticket.
 - When saving multiple values in `BP_XProfile_Data::save()`, you call the
 `delete()` method. This will trigger the `xprofile_data_before_delete`
 hook, which feels incorrect to me. As much as I like the idea of not
 duplicating code, I think we should make a direct DB query here (or break
 the existing `DELETE` query into its own hookless method, which would be
 called from both places).
 - You suggested earlier to "use the last row's item id" for storing
 xprofile meta, but you're returning the *first* row's data ID from the
 `save()` method. There's no technical issue here, especially since your
 patch doesn't actually use xprofile data meta at all (nor does BP itself?
 though it would perhaps be nice to move 'visibility' here; see #6350). It
 might be wise to standardize here. The docs for the `save()` method should
 probably indicate this fact about return values as well.

 > Since multiple values previously were stored and returned as serialized
 arrays, should we now return non-serialized row data also as a
 (re)serialized array - to keep backpat in order? Then data would be stored
 in the new fashion, but returned as is done previously. Any ideas about
 that?

 Ugh - I assume here you mean the `data` property of
 `BP_XProfile_ProfileData` objects, because `xprofile_get_field_data()` and
 other places do the necessary unserialization. I guess the answer is yes,
 we should reserialize so as to maintain backward compatibility with this
 specific field. But it suggests to me that perhaps we could set the `data`
 property with the serialized value, but then stop using the property
 internally. Instead, introduce something like a `get_data()` method, which
 would return structured data, and then use `get_data()` throughout BP.
 What do you think of an idea like that?

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