WordPress.org

Make WordPress Core

Opened 12 months ago

Last modified 2 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
Focuses: Cc:

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 12 months ago.
Simplify wp_slash()
24106.2.patch (2.1 KB) - added by SergeyBiryukov 5 months ago.
24106.test.php (1.3 KB) - added by SergeyBiryukov 5 months ago.
24106.wp_slash-unittest.diff (1.6 KB) - added by devesine 3 months ago.

Download all attachments as: .zip

Change History (14)

TobiasBg12 months ago

Simplify wp_slash()

comment:1 SergeyBiryukov12 months ago

  • Version set to trunk

comment:2 markoheijnen5 months ago

Isn't this something we can add in 3.8?

comment:3 SergeyBiryukov5 months ago

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

SergeyBiryukov5 months ago

comment:4 SergeyBiryukov5 months ago

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

SergeyBiryukov5 months ago

comment:5 follow-up: SergeyBiryukov5 months 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.

comment:6 ryan5 months 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 a slew of 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.

Last edited 5 months ago by ryan (previous) (diff)

comment:7 nacin5 months 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

comment:8 devesine3 months 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 3 months ago by devesine (previous) (diff)

comment:9 TobiasBg2 months 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.

comment:10 in reply to: ↑ 5 devesine2 months 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.

Note: See TracTickets for help on using tickets.