[buddypress-trac] [BuddyPress Trac] #6789: XProfile: do not store serialized arrays for multi-value profile field data
noreply at wordpress.org
Tue Feb 27 20:23:43 UTC 2018
#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:
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 .
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
- 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
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/>
More information about the buddypress-trac