[buddypress-trac] [BuddyPress Trac] #5919: Xprofile field meta values not deleted with field

[buddypress-trac]

#5919: Xprofile field meta values not deleted with field
--------------------------+------------------
 Reporter:  tometzky      |       Owner:
     Type:  defect (bug)  |      Status:  new
 Priority:  high          |   Milestone:  2.2
Component:  XProfile      |     Version:  2.1
 Severity:  major         |  Resolution:
 Keywords:  needs-patch   |
--------------------------+------------------
Changes (by boonebgorges):

 * keywords:  needs-patch good-first-bug => needs-patch
 * priority:  low => high
 * severity:  minor => major
 * milestone:  Future Release => 2.2


Comment:

 tometzky - Thanks for the constructive feedback.

 > When there's no meta values to delete, then this function returns a
 value which is ==false. It should return a ==true value.

 WP's `delete_metadata()` returns `false` when there is no meta to delete.
 We are following this convention.

 > But this causes that there can be no "highly targeted substring
 replacements", as relevant strings are moving targets.

 The string replacement was designed to apply very narrowly to a small
 number of queries. Your examples show that it's not focused enough - thank
 you very much for bringing these sorts of situations to our attention.

 > And all this so Wordpress functions like update_meta could be used on
 different table than wp_postmeta or wp_usermeta. When simply using
 $wpdb->insert() and $wpdb->update() would be much, much simpler and
 robust.

 See https://buddypress.trac.wordpress.org/ticket/4551 for some background
 on the decision to move to the WP functions. Short version: previously we
 had our own versions of these functions that did not use the WP functions.
 It resulted in a huge amount of highly fractured code, inconsistency,
 incomplete implementation of caching, and other problems. Making the move
 to the WP API was a trade-off: we got rid of the vast majority of this
 code (a good thing) but had to do a couple of oddball workarounds to get
 it to work (the SQL filtering you refer to). So, I agree with you that
 this is not ideal - and this ticket is making me consider how we can
 propose some upstream changes that will allow us to phase out our hacks -
 but I'm not as confident as you that the old way was much simpler and more
 robust. I would be glad to have a continued discussion about the wisdom of
 the path we've taken, but we are stuck with the current system at least in
 the short term, so I would like to come up with a fix for the problems
 you've raised that doesn't require refactoring the entire codebase.

 So, down to the task at hand. Given that you don't like what we're doing
 here in the first place, you might want to close your eyes for the rest of
 this update :) I've written a patch that modifies our SQL filter
 callbacks. Instead of blanket calls to `str_replace()`, it does the
 following:

 * Uses a regular expression to locate all quoted content. This will
 include all content (meta_key, meta_value) provided by the user.
 * Replaces each instance with a placeholder.
 * Does the existing `str_replace()` logic.
 * Swaps the placeholders out with the original quoted content.

 Unit tests are included to demonstrate how various kinds of quoted content
 is blocked from causing the problems that tometzky raises.

 I'm going to hijack this ticket for the purposes of fixing this issue.
 Once it's solved, we can get back to the original problem: deleting
 xprofilemeta when the field is deleted.

 Thanks again for your help, tometzky.

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