`update_metadata()` double-unslashes meta key when using `add_metadata()`

[jdgrimes]

When update_metadata() detects that the metadata doesn't exist in the database yet, it calls add_metadata() instead of continuing on with the "update" itself. Because the $meta_key and $meta_value are unslashed by both the update_metadata() and add_metadata() functions, the values saved in the database will be incorrect. This was fixed for the $meta_value in #17343. However, the $meta_key is still unslashed twice.

It isn't possible to work around this by just slashing the value twice before passing it to update_metadata(), because if the value already existed in the database (and so add_metadata() wouldn't be used) the value would only be unslashed once.

So there is no way to pass the data to update_metadata() in such a way that it won't end up being either too slashed or too unslashed part of the time.

I discovered this because I was saving some metadata for some objects whose slugs I had designed to\be\namespaced in some cases. The slug (slashed once with wp_slash()) was being used as the meta key, which worked except when using update_metadata() when the meta key didn't yet exist in the database—then all the slashes were ripped out.

// Assumes that the meta key 'test\slashing' doesn't exist.
update_post_meta( $post_id, wp_slash( 'test\slashing' ), 'value' );

// get_metadata() doesn't unslash the key, so we don't need to slash it as above.
$value = get_post_meta( $post_id, 'test\slashing', true );

// $value will be '', should be 'value'.

// Now $value will be 'value'.
$value = get_post_meta( $post_id, 'testslashing', true );

Related: #27421

[boonebgorges]

I think you've actually understated the problem. Not only do keys get double-slashed when update_metadata() falls back on add_metadata(), but also when update_metadata() does its "has anything changed?" check by calling get_metadata().

Until such time as the slashing mess is cleared up, these functions should be reworked so that two copies of $meta_key are kept internally: one that is unslashed (this is the one used for database queries), and one that is not-unslashed (this is the one used to pass to other meta API functions). This change should be supported by copious tests.

[boonebgorges]

Actually, scratch my comment about get_metadata() above - that function expects unslashed keys.

[boonebgorges]

See 35795.diff​ for what I have in mind. I'm a bit cross-eyed from the number of slashes required to make the unit test work as expected, but I think it's correct.

[jdgrimes]

Same as previous, but using wp_slash()

[jdgrimes]

Replying to boonebgorges:

See 35795.diff​ for what I have in mind. I'm a bit cross-eyed from the number of slashes required to make the unit test work as expected, but I think it's correct.

Thanks for looking into this @boonebgorges. I've uploaded 35795.2.diff​ to use wp_slash() in the tests instead of placing the double slashes in the strings themselves, because I find that easier to follow.

[boonebgorges]

In 36509:

Don't double-unslash meta key when update_metadata() falls back on add_metadata().

Props jdgrimes.
Fixes #35795.

[jdgrimes]

I wonder if this fix should be backported to prior versions? I'm not sure how that decision is made, but I thought I'd mention it.

[boonebgorges]

Replying to jdgrimes:

I wonder if this fix should be backported to prior versions? I'm not sure how that decision is made, but I thought I'd mention it.

We backport to the current stable branch for regressions or security fixes, and to earlier branches for security fixes only. This bug doesn't fall into either category.

[jdgrimes]

Replying to boonebgorges:

Replying to jdgrimes:

I wonder if this fix should be backported to prior versions? I'm not sure how that decision is made, but I thought I'd mention it.

We backport to the current stable branch for regressions or security fixes, and to earlier branches for security fixes only. This bug doesn't fall into either category.

OK. Thanks for clarifying that for me.