WordPress.org

Make WordPress Core

Opened 7 years ago

Last modified 22 months ago

#24106 new enhancement

Simplify wp_slash()

Reported by: TobiasBg Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.6
Component: Formatting Keywords: has-patch needs-unit-tests dev-feedback
Focuses: Cc:
PR Number:

Description

[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.

Attachments (4)

24106.patch (1.0 KB) - added by TobiasBg 7 years ago.
Simplify wp_slash()
24106.2.patch (2.1 KB) - added by SergeyBiryukov 6 years ago.
24106.test.php (1.3 KB) - added by SergeyBiryukov 6 years ago.
24106.wp_slash-unittest.diff (1.6 KB) - added by devesine 6 years ago.

Download all attachments as: .zip

Change History (16)

@TobiasBg
7 years ago

Simplify wp_slash()

#1 @SergeyBiryukov
7 years ago

  • Version set to trunk

#2 @markoheijnen
6 years ago

Isn't this something we can add in 3.8?

#3 @SergeyBiryukov
6 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 3.8

#4 @SergeyBiryukov
6 years ago

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

#5 follow-up: @SergeyBiryukov
6 years ago

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 arrayNested arrays
7.121s — unpatched19.524s — unpatched
9.754s — 24106.2.patch26.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.

#6 @ryan
6 years ago

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.

Version 0, edited 6 years ago by ryan (next)

#7 @nacin
6 years ago

  • Keywords commit removed
  • Milestone changed from 3.8 to Future Release

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

#8 @devesine
6 years ago

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.

Last edited 6 years ago by devesine (previous) (diff)

#9 @TobiasBg
6 years ago

  • Keywords needs-unit-tests added

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.

#10 in reply to: ↑ 5 @devesine
6 years ago

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.

#11 @chriscct7
4 years ago

  • Keywords dev-feedback added

#12 @jrf
22 months ago

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 ?

Note: See TracTickets for help on using tickets.