WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#18026 closed defect (bug) (fixed)

stripslashes_deep() casts booleans to strings

Reported by: DemoIgor Owned by: nacin
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.2
Component: Formatting Keywords: has-patch commit
Focuses: Cc:

Description

older
a:1:{s:13:"administrator";b:1;}
newer (PHP v5.3)
a:1:{s:13:"administrator";s:1:"1";}

Function stripslashes_deep() change bool "true" >to> string "1"

Attachments (2)

18026.diff (738 bytes) - added by kawauso 4 years ago.
is_string() on stripslashes()
stripslashes.diff (362 bytes) - added by knutsp 4 years ago.
Only stripslash strings (and arrays, objects)

Download all attachments as: .zip

Change History (16)

comment:1 @knutsp4 years ago

  • Cc knut@… added

What is the bug that needs to fixed? If this is some kind of enhancement, why do you want to do this?

comment:2 @nacin4 years ago

I've noticed this. I think the report is that stripslashes_deep() is now moving (bool) true to (string) "1". I'd call that a bug.

comment:3 @scribu4 years ago

  • Summary changed from Change value type in wp_capabilities or any custom user field to stripslashes_deep() casts booleans to strings

Confirmed:

var_dump( stripslashes_deep( array( 'foo' => true, 'bar' => false ) ) );

// Result: array(2) { ["foo"]=> string(1) "1" ["bar"]=> string(0) "" }

comment:4 @knutsp4 years ago

I see. That makes sense.

I found this comment on http://php.net/manual/en/function.stripslashes.php


2) both stripslashes and str_replace functions always return strings, so:

  • TRUE will become a string "1",
  • FALSE will become an empty string,
  • integers and floats will become strings,
  • NULL will become an empty string.

On the other hand you only need to process strings, so use the is_string function to check;


If so, then the function could be exptended a bit like this:

function stripslashes_deep($value) {
    $value = is_array($value) ?
        array_map('stripslashes_deep', $value) :
        (is_string($value)?
            stripslashes($value):
            $value);
 
    return $value;
 }

But I don't like the $value = $value so I would prefer using if blocks instead of nested ternal if's.

Test:

var_dump( stripslashes_deep( array( 'foo' => true, 'bar' => false, 'string' => 'str\ing', 'array' => array( 'foo1' => true, 'bar1' => false, 'arr1' => 'arr\1', 'arr2' => 'arr\2') ) ) );

Result:
array(4) { foo?=> bool(true) bar?=> bool(false) string?=> string(6) "string" array?=> array(4) { foo1?=> bool(true) bar1?=> bool(false) arr1?=> string(4) "arr1" arr2?=> string(4) "arr2" } }

@kawauso4 years ago

is_string() on stripslashes()

comment:5 @kawauso4 years ago

  • Keywords has-patch added

@knutsp4 years ago

Only stripslash strings (and arrays, objects)

comment:6 follow-up: @knutsp4 years ago

Ok, I'll do another easy one :-)

Lessons for a newbee:

  • Always do a last check if a patch has already been uploded
  • Name the .diff as the <ticket_number>.diff
  • Remember the documentation @headers

Question: If I am working on a patch, even a small one, should I accept it or reassign it to me?

comment:7 in reply to: ↑ 6 @SergeyBiryukov4 years ago

Replying to knutsp:

Question: If I am working on a patch, even a small one, should I accept it or reassign it to me?

According to Codex, it's up to you:

A WordPress developer (which could be you) decides to fix the bug. The developer may choose to take responsibility for the bug by clicking on the Accept ticket option near the bottom of the ticket, though this is not required. Then the developer figures out how to fix the bug, creates one or more patch files, and uploads them to Trac.

http://codex.wordpress.org/Reporting_Bugs

Generally the owner of a ticket is either a person who supplies the patch or the one who oversees the ticket (i.e. a committer).

comment:8 @coffee2code3 years ago

  • Version set to 3.2

Re-verified bug. Tested 18026.diff and it still applies cleanly as of r20598.

Added unit tests (see [UT698]) for stripslashes_deep() in general, including a test reflecting this bug. Test relating to this bug passes after aforementioned patch is applied.

Setting version to 3.2 which was current as of the original bug submission (though the bug harkens back to the introduction of stripslashes_deep() in 2.0).

Last edited 3 years ago by coffee2code (previous) (diff)

comment:9 @SergeyBiryukov3 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to Future Release

comment:10 @nacin3 years ago

  • Component changed from General to Formatting
  • Milestone changed from Future Release to 3.5

18026.diff and [UT698] both look good.

comment:11 follow-up: @nacin3 years ago

Since we use stripslashes_deep() quite a bit in core, I'm trying to think of the potential side effects. For one, an update_option() of two equal serialized values (that are no longer equal due to the change here) would force an update. I don't see this as a big problem, but something to keep in mind. A deploy to WP.com for instance might cause a spike in writes.

comment:12 in reply to: ↑ 11 @westi3 years ago

Replying to nacin:

Since we use stripslashes_deep() quite a bit in core, I'm trying to think of the potential side effects. For one, an update_option() of two equal serialized values (that are no longer equal due to the change here) would force an update. I don't see this as a big problem, but something to keep in mind. A deploy to WP.com for instance might cause a spike in writes.

This patch looks good to me too.

The possibly wide reaching effect is a bit of a worry but no one should be updating options from most requests anyway so I don't think it would have too much of an effect.

comment:14 @nacin3 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In [21292]:

Only run stripslashes() in stripslashes_deep() for strings, not other scalar values. props Kawauso, knutsp. props coffee2code for [UT698]. fixes #18026.

Note: See TracTickets for help on using tickets.