WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#16974 closed defect (bug) (wontfix)

absint to return 0 for values with non-numeric characters

Reported by: sc0ttkclark Owned by:
Milestone: Priority: normal
Severity: minor Version: 3.1
Component: General Keywords: has-patch
Focuses: Cc:

Description

Currently, if you put in a string into absint which contains non-numeric characters after numeric characters it will return the number before the non-numeric characters as the number, which I believe isn't the valid return for absint($maybeint)

The solution I am proposing is to check if $maybeint isn't numeric, and if not then return 0;

See the attached patch.

Attachments (3)

16974.patch (373 bytes) - added by sc0ttkclark 3 years ago.
16974.2.patch (362 bytes) - added by sc0ttkclark 3 years ago.
16974.3.patch (364 bytes) - added by sc0ttkclark 3 years ago.

Download all attachments as: .zip

Change History (26)

sc0ttkclark3 years ago

comment:1 sc0ttkclark3 years ago

Oops, sorry for the tab difference, my editor is set to use spaces vs tabs.

comment:2 sc0ttkclark3 years ago

  • Summary changed from absint to return 0 for values that with non-numeric characters to absint to return 0 for values with non-numeric characters

sc0ttkclark3 years ago

comment:3 aaroncampbell3 years ago

We talked a little on IRC. What he's trying to fix is that absint( '123abc' ) returns 123 but absint( 'abc123' ) returns 0.

His original issue came from the fact that http://wordpress.org/news/?p=100 is the same as http://wordpress.org/news/?p=100test but not the same as http://wordpress.org/news/?p=test100

comment:4 scribu3 years ago

I always assumed that the definition for absint was return abs( (int) $maybeint ) );

Apparently not.

comment:5 scribu3 years ago

The original commit for absint() also mentioned casting: [6222]

comment:6 sc0ttkclark3 years ago

@scribu - It is, I just believe that if $maybeint contains non-numeric characters it should return zero to avoid canonical issues with the values it deals with. Otherwise, a significant number of absint usages in WP Core would need additional checks to prevent the usage of non-numeric characters in their values.

comment:7 scribu3 years ago

Yeah, it seems intval() is equivalent to using (int), so nevermind.

Note that is_numeric() will also match floats and other representations.

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

comment:8 sc0ttkclark3 years ago

Went the direction if is_numeric vs is_int so that it would avoid issues with strings not being handles as numbers (is_int requires an integer)

comment:9 scribu3 years ago

Yeah, there's no built-in function for strict casting. Might have to resort to a simple regex, since most cases where absint() is used is for validating a database ID.

comment:10 duck_3 years ago

The logic of your if statement appears to be the wrong way around, ! is_numeric surely.

comment:11 sc0ttkclark3 years ago

Absolutely correct @duck_, My wife and I just had a baby a week ago and have suffered the resulting #fail on a simple piece of code :)

Patching now..

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

sc0ttkclark3 years ago

comment:12 westi3 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

I don't see a valid reason for changing this.

In fact I see more valid reasons for keeping what we have.

I would much rather that: http://wordpress.org/news/?p=100test still worked than didn't work and I don't want to possibly break a whole load of plugin / theme code just to chage this function to work how you think it should work rather than how it works.

PHP is by design a loosely typed language and WP itself lives within this loosely typed world.

The whole point of absint is to make sure that the thing you are treating as an integer really is treated as an integer not a string so that when you later use this value you get what you expect.

comment:13 follow-up: sc0ttkclark3 years ago

@westi - Why would you want ?p=100test to work? And what plugin / theme code would break from this? If anyone can give me an example, I'd love to see it because I just don't think there are any. Maybe that's me being naive, but I haven't heard anyone provide any use-cases for absint usage within core or plugins/themes which the result of absint should allow for non-numeric characters.

comment:14 in reply to: ↑ 13 westi3 years ago

Replying to sc0ttkclark:

@westi - Why would you want ?p=100test to work?

Because urls end up getting bunched up against other text by accident in comments, emails etc.

comment:15 nacin3 years ago

Strong +1 for the wontfix.

Imagine a ?p= URL that was made clickable, but the trailing whitespace was lost. http://wordpress.org/news/?p=100and would work. So would http://wordpress.org/news/?p=100), in systems where the ) becomes clickable.

That's just one specific instance. I am also for wontfix on the theory basis. absint() doesn't perform validation, it performs sanitization. We're using core PHP functions here, so this is expected behavior. I see no reason to modify this.

comment:16 nacin3 years ago

A change like this has huge potential cost, and offers no practical benefit.

comment:17 aaroncampbell3 years ago

That's the way I was leading when talking to him in IRC as well. I think absint() is just used in too many places to make a change like this.

I'd instead recommend that if a site wants to handle things differently they grab p and filter it before it gets processed normally.

comment:18 follow-up: sc0ttkclark3 years ago

Gotcha, I see what you mean here though I believe http://wordpress.org/news/?p=100and itself is a bug and should be redirected to normalize 'p' in the query (and other places). absint hides issues like this, which is what I'm against.

I'll let it go, will put my own solution together for usage in my work.

comment:19 sc0ttkclark3 years ago

Was thinking more (one more try), perhaps absint could have an additional variable, so it'd be like:

function absint ($maybeint, $strict = false) {
    if ( $strict && ! is_numeric ( trim ( $maybeint ) ) ) {
        return 0;
    }
    return abs( intval( $maybeint ) );
}

This seems like a perfect solution, which wouldn't run the is_numeric / trim unless told to do so by the usage of absint($value,true);

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

comment:20 nacin3 years ago

At that point, why even use absint()? Or only use absint() after using is_numeric(). Core won't use this, and you're not required to leverage absint.

comment:21 sc0ttkclark3 years ago

You're the boss

comment:22 in reply to: ↑ 18 ; follow-up: scribu3 years ago

Replying to sc0ttkclark:

Gotcha, I see what you mean here though I believe http://wordpress.org/news/?p=100and itself is a bug and should be redirected to normalize 'p' in the query (and other places).

I'm pretty sure there's a canonical redirect in place for this case. If not, there should be.

comment:23 in reply to: ↑ 22 sc0ttkclark3 years ago

Replying to scribu:

Replying to sc0ttkclark:

Gotcha, I see what you mean here though I believe http://wordpress.org/news/?p=100and itself is a bug and should be redirected to normalize 'p' in the query (and other places).

I'm pretty sure there's a canonical redirect in place for this case. If not, there should be.

Only redirects if you are using non-p permalinks. Like in the case of this site:

pcgamingcorner.com/wordpress/?p=820and

Note: See TracTickets for help on using tickets.