Xprofile field meta values not deleted with field

[tometzky]

When you delete an Xprofile field with any meta values in wp_bp_xprofile_meta table, then these meta values are left in database.

Try this:

• create a new field;
• change default_visibility or allow_custom_visibility of this field;
• delete this field;
• default_visibility or allow_custom_visibility would be left in wp_bp_xprofile_meta table.

Please remove field meta values when you delete a field.

[boonebgorges]

Good catch. This is a pretty easy patch for someone who wants some juicy props.

[tometzky]

Yeah - it should be easy... But.

1.

When I added in BP_XProfile_Field::delete():

if ( !bp_xprofile_delete_meta( $this->id, 'field' ) ) return false;

Then it breaks when there's no meta defined.

2.

Then I tried to check if any metadata exists. And then I went on to read bp_xprofile_filter_meta_query() and bp_filter_metaid_column_name() functions. And I cried. They're blindly replacing substrings in SQL query. This is ​thedailywtf.com inspired coding.

[boonebgorges]

They're blindly replacing substrings in SQL query. This is thedailywtf.com inspired coding.

These are highly targeted substring replacements that are covered by extensive unit tests. The BP team has a close connection to the WP core team (I am a member of both) and will be aware of breaking changes. In any case, these filters have nothing to do with the case at hand, so perhaps we could set aside your distaste for the purposes of this ticket :)

Then it breaks when there's no meta defined.

Can you give more details about what breaks? Are you getting some sort of error? Here is the relevant unit test: ​https://buddypress.trac.wordpress.org/browser/tags/2.1/tests/phpunit/testcases/xprofile/functions.php#L243

[tometzky]

Replying to boonebgorges:

Can you give more details about what breaks? Are you getting some sort of error? Here is the relevant unit test: ​https://buddypress.trac.wordpress.org/browser/tags/2.1/tests/phpunit/testcases/xprofile/functions.php#L243

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

So the relevant unit test would be something like:

public function test_bp_xprofile_delete_meta_when_no_meta() {
  $this->assertTrue( bp_xprofile_delete_meta( 0x7FFFFFFF, 'field' ) );
}

These are highly targeted substring replacements that are covered by extensive unit tests.

I'm sorry, but I don't understand how they're highly targeted. Both Wordpress and Buddypress are very extensible programs - the both have many hooks that allow plugins or themes to modify their behavior. And that's why they're so popular. But this causes that there can be no "highly targeted substring replacements", as relevant strings are moving targets.

Please consider the following example. A function bp_xprofile_update_meta(), as I understand, should be used to add additional meta values to xprofile fields. This is used by my plugin to add two strings, controlled from dashboard, to any xprofile field.

But these string can very much contain for example a substring "WHERE ". Like a message "PLEASE GO SOMEWHERE ELSE". Trying to save this string as a meta value would generate SQL error:
WordPress database error: [You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'field' AND ELSE' WHERE object_type = 'field' AND object_id = '24' AND `meta_ke' at line 1]
UPDATE wp_bp_xprofile_meta SET meta_value = 'PLEASE GO SOMEWHERE object_type = 'field' AND ELSE' WHERE object_type = 'field' AND object_id = '24' AND meta_key = 'validate_with_regex_error_message'

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.

[boonebgorges]

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.

[tometzky]

Replying to boonebgorges:

• Uses a regular expression to locate all quoted content. This will include all content (meta_key, meta_value) provided by the user.

I'd rather use a function that would find a substring and replace only this one substring. I may assume that "WHERE ", that we need to replace, would be the first in "SELECT", and the last in "UPDATE ".

This regular expression is too ​write only for me. And it assumes that there's only one quoted text, which I'm not convinced that it is.

• Replaces each instance with a placeholder.

I'd rather use some random placeholder like uniqid('__QUOTE__').

[DJPaul]

If I remember correctly, we've only done this because our meta tables have a different schema than WP core's post/usermeta tables.

How about we create a ​MySQL view in our meta tables, and use the name of the column that WP is looking for? Then I think we can just avoid filtering the SQL statement at all.

[boonebgorges]

This regular expression is too write only for me.

Yes, it's a bit opaque. The idea is to find all instances of quoted text, because that's where all input from outside the system will appear. That regex is actually pretty simple: /'(.*+)'/ or something like that. The problem is that this quoted text might contain escaped quotes (...WHERE meta_value = 'I said \'hello\' to him'...). The extra complexity in the regular expression is to account for this. I've borrowed it from ​http://stackoverflow.com/questions/5695240/php-regex-to-ignore-escaped-quotes-within-quotes.

And it assumes that there's only one quoted text, which I'm not convinced that it is.

No, this accounts for all quoted text. See the use of preg_match_all().

I'd rather use some random placeholder like uniqid('QUOTE').

I believe this is unnecessary. The only important thing about the placeholder text is that it not match any of the substrings we're going to replace using str_replace(). In other words, when we translate

UPDATE wp_bp_xprofile_meta SET meta_value = 'PLEASE GO SOMEWHERE ELSE' WHERE object_id = '24' AND meta_key = 'validate_with_regex_error_message' 

into

UPDATE wp_bp_xprofile_meta SET meta_value = __QUOTE__ WHERE object_id = __QUOTE__ AND meta_key = __QUOTE__

we have done enough to assure that things like str_replace( 'WHERE', ... will not catch any false positives.

If I remember correctly, we've only done this because our meta tables have a different schema than WP core's post/usermeta tables.

This is true for bp_filter_metaid_column_name(), and it's worth looking into. We still need to manipulate the SQL for xprofile clauses in order to insert an object_type = 'field' clause.

[tometzky]

OK. It looks that it would work. Though I'd definitely use a variable instead of repeating 'QUOTE' and use strlen($quote) instead of 9.

But I'm still not convinced that this is all worth it. This was to avoid copying similar code from *_meta core functions, but:

• it isn't any simpler;
• it also repeats code - code in bp-core/bp-core-filters.php is copied to/from bp-xprofile/bp-xprofile-filters.php;
• simply using $wpdb->update() did not use significantly more code;
• it does not need to use Object Cache, as I don't see where it could be retrieved more than once per request anyway - so caching it just wastes memory; throwing out caching would simplify it even further.

[boonebgorges]

But I'm still not convinced that this is all worth it.

Perhaps not, but I disagree with your points. Object cache support is extremely important in a couple ways:

1. When querying for a single piece of object metadata, all of that object's metadata is loaded into the cache, so that subsequent calls to get_metadata() on the same pageload do not hit the database
2. When pulling up a list of items - such as a profile page, where many profile fields may be displayed - we prefetch metadata for all xprofile objects and store it in the object cache, so that we only run one database query instead of many
3. On sites that have a persistent cache backend, our cache support means that calls to get_metadata() almost never hit the database

Using WP's functions for this means that we get support for all of this. Previously, we supported none of it.

I do very much appreciate your thoughts on this issue, and if you'd like to continue the discussion and make some suggestions about future improvements, please open a separate ticket for it. In the meantime, I'm going to go with my suggested fix so that we can get on with the original problem of clearing field meta (remember that? :) ).

[boonebgorges]

In 9073:

Better detection for false positives in meta SQL filters.

Our wrappers for WP's _metadata() functions require some filtering of the SQL
string (to change a column name and, in the case of xprofile, to add an
'object_type' clause). Our str_replace() logic is too generous, creating the
possibility of matching quoted text, as when the meta value contains the string
'WHERE'.

This changeset modifies the filters so that quoted content is swapped out with
placeholders before we run our search-and-replace.

See #5919.
Props tometzky for feedback.

[Offereins]

The original issue was fixed in r11847, so marking as duplicate of #6658. For remaining issues, please open a new ticket.