Opened 12 years ago
Closed 4 years ago
#24106 closed defect (bug) (fixed)
Simplify wp_slash()
Reported by: | TobiasBg | Owned by: | whyisjake |
---|---|---|---|
Milestone: | 5.5 | Priority: | normal |
Severity: | normal | Version: | 3.6 |
Component: | Formatting | Keywords: | has-patch needs-unit-tests dev-feedback |
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)
Change History (18)
#4
@
11 years ago
24106.2.patch includes a basic test for wp_slash()
(adapted from Tests_Formatting_StripSlashesDeep
).
#5
follow-up:
↓ 10
@
11 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 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.
#6
@
11 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 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.
#7
@
11 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
@
11 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.
#9
@
11 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
@
11 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'sstripslashes()
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.
#12
@
7 years 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 ?
Simplify wp_slash()