Code Improvement in get_userdata

[hakre]

Minor code improvement.

[scribu]

IMO, minor code improvements like this should be bundled with other patches.

The time wasted creating this ticket and committing the patch by a core dev outweighs it's usefulness.

[nacin]

Replying to scribu:

IMO, minor code improvements like this should be bundled with other patches.

+1

The time wasted creating this ticket and committing the patch by a core dev outweighs it's usefulness.

+10

This is a really, really silly patch. Not only that, but a == check on 0 should just be !$user_id instead.

[hakre]

I thought patch-files were the common way to do it, but I'm flexible. So back on tra(c)k, please make your choice:

[ ] A - Commit
[ ] B - Close as wontfix
[ ] C - Punt to future release
[ ] D - Commit with nacin's additional suggestion
[ ] E - Commit with nacin's suggestion and take into account 
    that it is possible to spare the absint() call for the 
    case of false.
[ ] F - Wait for an additional patch and then start another 
    discussion right away after.

I would prefer E to finally give this discussion some value. Nacin, please let me know if you can do the change of those ca. three lines of code on your own or you want a patch-file for it. I personally now prefer the first variant. You're the committer, just let me know which way you prefer it.

[hakre]

I finally must admit that those useless comments might have distracted me somehow and in kind of a bad influence.

Naturally my original patch is totally okay and nacin's suggestion to it are infact silly.

It's common practice in core code when it comes to a comparison against zero for a userid to do it with the numerical value and the standard php comparison operator ==. Using false or ! would be misleading.

So my new choice is A.

I hope some other comitter is able to concentrate on the actual work instead of trying to find an argumentation to no do things. It's especially a problem for me to see a really useless discussion here instead of a quick and straight ticket handling. It's not me who is wasting time in the end. Please deal with such minor changes more efficient next time to safe us all some time to concentrate on more important stuff. Thanks.

[nacin]

(In [14579]) Whitespace and standards in get_userdata. fixes #13317.

[hakre]

if you're in for ! (instead of 0 == ), spare the absint() call while userid is false

[hakre]

to give the discussion some value in collaborative work.

[nacin]

If the user_id is nonnumeric for some reason, that will absint() to 0 and thus compute to false. Moving the absint down would break that. I find ! $user_id easier to read. Yes, 0 == $user_id works, and perhaps is more clear with absint. I'm not inclined to change this again.

[hakre]

A non-numeric user id? I have not seen that but for requesting the admin user obect:

$admin_user = get_userdata( array( 'theee_admin();' ) );

This one at least works for 99% of all installs and I think we must keep it backwards compatible. I've seen that with some plugins.

I was making up my mind a bit what the difference between just using (int) abs( $val ) instead of absint( abs( intval( $val ) ) ) would be. I have not found any difference by now, so it might be of some value to remove absint() from code and replace it with that more effective variant.

Please let me know if you want a patch.

[Denis-de-Bernardy]

Reopening, since there seems like there's potential for security issues here...

[nacin]

Replying to Denis-de-Bernardy:

Reopening, since there seems like there's potential for security issues here...

Pardon me for having no idea what you're talking about.. but I have no idea what you're talking about.

[Denis-de-Bernardy]

What I'm meaning is that get_userdata(garbage) should not return an admin user on 99% of sites. It should fail and return false, zero, whatever. If, for any reason, a plugin uses the function improperly, it may very well introduce security risks. And nevermind that you can't see one. I'm sure you couldn't see any in the eval() calls used in permalinks either. But low and behold, they got exploited.

[hakre]

Testcases

[hakre]

Testresults

[hakre]

Finally I was able to run some tests to find out more. The Absint() function should be put on the deprecated list and replaced with a single function call:

abs( (int) $val );

because it just does the job, this behaves absolutely the same as absint( $val );. I've attached a detailed testfile and exemplary output results.

[nacin]

The Absint() function should be put on the deprecated list and replaced with a single function call

We use absint() everywhere -- it's a well-known shortcut for plugin developers that does abs( intval( $val ) ), or alternatively abs( (int) $val ). It's not going anywhere. We didn't need the test cases to prove that they're the same :-)

What I'm meaning is that get_userdata(garbage) should not return an admin user on 99% of sites. It should fail and return false, zero, whatever.

Garbage will almost always evaluate to false on absint(), which will mean get_userdata will return false. I'm still missing the point.

[Denis-de-Bernardy]

Replying to nacin:

Garbage will almost always evaluate to false on absint(), which will mean get_userdata will return false. I'm still missing the point.

That would mean you haven't read hakre's comment or tested the code in it further up.

[nacin]

I don't see why we need to protect against garbage inputs like array( 'theee_admin();' ). We're expecting an integer, so we absint() what we get. If we get an object, we'll throw a notice.

Are you looking for an is_int()? Should we is_int() check every variable before absint'ing it, when all we expect is an integer to begin with?

[hakre]

Replying to nacin:
You should not concentrate too much why to not make a change but why. In this case (and be lucky it got noticed) this function should not return the admin user object on error. so, you do not need to run all core up and down and insert checks, but just in places where it can actually make sense.

Any way, I'll add a patch for that one and for the general replacement of the absint() calls.

[hakre]

the absint() function is useless while casting is taken into account. therefore deprecate it and teach users a better way to use: abs( (int) $var );.

Related: #4762

I'll add another one that will get grip on needless intval() usage.

[nacin]

absint() isn't going anywhere.

Not only is it way more readable -- you'd have to sneak it into a larger patch for me to commit that -- but we also use it throughout core for array_map callbacks.

We want to encourage integer casting for security. Let's not make it more difficult. It is also easier to document and teach a function than casting or coercion.

Additionally, it is *not* a "better way." That is your opinion. Both work quite well.

You should not concentrate too much why to not make a change but why.

I am firmly and proudly in the 'why not' camp. Changes should be sensical and not based on a "just because we can" philosophy.

Finally, there is a difference between returning an admin user object on error and stuffing absolute garbage into functions.

[hakre]

Putting it into deprecated really got over the top. The usage as callback I have not checked, my fault.

But that does not make the overexagerated usage of absint() all over the place better.

I think you're in the 'why not' spread absint() all over the place "just because we can" camp. I do not share both points. Wether the why not nor the all over the place. You must really misread me.

I'll reopen, add (int) to where it is fit at least. But it might be even too much for you, because you use intval() just because you can, right? and yes, why not .... .

[hakre]

One must not use absint() or intval()

[hakre]

Replying to nacin:

Finally, there is a difference between returning an admin user object on error and stuffing absolute garbage into functions.

Yeah, totally right. I just wonder why - when the docblocks already document that $user_id is to be int already you run absint() on it anyway. I mean, it's already an integer, and if some adds garbage into this function, like a negative integer, the function is not expceted to return a user, right?

You should really reflect what you say here in the end.

Getting the admin when passing -1 to that function is not an equally well thing either. But, let's better not do that strict, this is only about user-management which is knowing to be an area, where to do things in a secure manner isn't further important.

Sorry for so much irony.

[hakre]

Gosh, there are really tons of the array_map('absint' calls. well, if I did knew that, I must say, why did no one thought of really making the job easier for the coders here by just allowing using absint on an array by design?

like so:

function absint( $maybeint ) {
	if ( is_array($maybeint) )
		return array_map('absint', $maybeint);				
	return abs( intval( $maybeint ) );
}

That would finally provide some use then for the wpquery subroutine. and it would make my live harder to remove it all over the place *gg*.

[nacin]

Absolutely not. The whole point of absint is to return an integer. That would break its existing functionality of forcing an integer return. Maybe introduce absint_map(), but we cannot do what you are suggesting.

I see no compelling reason to make any changes to get_userdata, absint, or intval. "One must not use absint() or intval()" -- that's your opinion and you're welcome to have it, but we're done wasting my time here. Closing again as fixed on [14579].

[westi]

If future can reporters please provide a more detailed ticket description hi-lighting the actual issue that they are trying to resolve.

It is very unproductive for everyone to spend the amount of time that has been invested in this ticket over a trivial coding style type discussion which is the meat of what is here.

[westi]

(In [14695]) Don't return data for user 1 when passed in junk like an array. See #13317.

[hakre]

Considering negative values as garbage as well. Unfixed for those. Please reopen and fix.

[hakre]

is_numeric() check should be replaced with is_scalar() instead to change to a var-type-check instead of parsing the string - while it's at it.

[hakre]

updated using is_scalar() instead of is_numeric()

[nacin]

is_numeric broke an instance of passing $post->post_author which was string "0" to get_userdata.

[nacin]

(In [14780]) Revert [14695]. We need to be a bit less strict. see #13317.

[nacin]

(In [14781]) False alarm, something else is going on here. Revert [14780], restore [14695]. see #13317.

[nacin]

For reference, handled in [14782].

[hakre]

no comment: "hakre — 11 days ago -- updated using is_scalar() instead of is_numeric()"