Simplify wp_slash()

[TobiasBg]

[23416] added the function wp_slash() for the slashing sanitization in #21767.

​According to ryan, it has been modelled after $wpdb->prepare(), and therefore uses a custom foreach loop with an if-check in it.
I suggest to instead model it after stripslashes_deep() and urlencode_deep() to simplify the function and make it better readable.

The attached patch also makes it clearer that this function works in a recursive manner.

[TobiasBg]

Simplify wp_slash()

[markoheijnen]

Isn't this something we can add in 3.8?

[SergeyBiryukov]

24106.2.patch​ includes a basic test for wp_slash() (adapted from Tests_Formatting_StripSlashesDeep).

[SergeyBiryukov]

Wrote a quick script to test the performance of the suggested change with 1,000,000 iterations: 24106.test.php​.

The unpatched version appears to be about 25% faster:

Simple array	Nested arrays
7.121s — unpatched	19.524s — unpatched
9.754s — 24106.2.patch​	26.051s — 24106.2.patch​

Not sure if performance is crucial for this function though. stripslashes_deep() uses array_map() too​, so I guess it would make sense for wp_slash() to be consistent.

[ryan]

Some of this slash/unslash code was simply cut-and-pasted from old sources without alteration to avoid any chance of back compat problems. We promised to write the needed test cases when we finally got around to cleaning up the code. Cue the present and the slew. :-)

We need to inventory all of the different depth and object slashing behaviors our varied rogues gallery of slash/unslash functions exhibit and write unit tests for everything.

[nacin]

We need to inventory all of the different depth and object slashing behaviors our varied rogues gallery of slash/unslash functions exhibit and write unit tests for everything.

The unpatched version appears to be about 25% faster

[devesine]

Attached 24106.wp_slash-unittest.diff​ is a unit test for slashing behavior that includes datatype sanctity (compare the StripSlashesDeep test). As it stands, calling wp_slash followed by wp_unslash causes datatype loss.

This makes wp_slash rather unsuitable for use when preparing (for instance) postmeta to pass to add_metadata.

[TobiasBg]

devesine, I'm not sure what you mean with datatype loss here. wp_slash()/wp_unslash() (both the patched and the unpatched versions) will only work with strings and (nested) arrays of strings, which basically is a result of PHP's stripslashes() only accepting and returning a string.

[devesine]

Replying to TobiasBg:

devesine, I'm not sure what you mean with datatype loss here. wp_slash()/wp_unslash() (both the patched and the unpatched versions) will only work with strings and (nested) arrays of strings, which basically is a result of PHP's stripslashes() only accepting and returning a string.

wp_unslash() uses stripslashes_deep() (source​) , which handles strings, arrays, and objects (via get_object_vars()), and the tests for stripslashes_deep() (and by extension wp_unslash()) test for its ability to touch objects without turning them into arrays.

Unpatched wp_slash() only handles strings and arrays. The test I attached uses the same logic as the existing test for stripslashes_deep(), but on the slashing rather than the unslashing side of things.

wp_unslash() has a docblock that indicates it only works on strings or arrays of strings, but it is currently inaccurate.

[jrf]

Came across the code of wp_slash() today and just couldn't help but wonder why a recursive function wasn't properly implemented as recursive.

Searching led me to this ticket and #42195. /cc @johnbillion

I've prepared a patch which would fix both tickets and which unifies and adds to the unit tests uploaded in previous patches for this ticket by @SergeyBiryukov and @devesine.

Before I upload the new patch, I'd like to get some clarification on how objects should be handled.

In my opinion, object property values should never be slashed, but I saw some unit tests in the previous uploads which don't seem to agree with that.

Reaching a consensus on how objects should be handled by wp_slash() is a prerequisite for any patch dealing with this and #42195.

@pento Got an opinion on this ?

[whyisjake]

Bringing the changes here over with #42195.

[whyisjake]

In 48433:

Formatting: Prevent wp_slash from returning non-strings as strings.

If a bool/float/int is passed into wp_slash it will be coerced into a string.

This changes the behavior to only slash strings. At the same time, handles recursion a little nicer by calling array_map for arrays.

Fixes #42195, #24106.

Props johnbillion, andizer, jrf, ryotasakamoto, SergeyBiryukov, donmhico, TobiasBg, markoheijnen, ryan, nacin, devesine, whyisjake.