WP_User constructor not working as expected if called with 2 args

[francescolaffi]

WP_User constructor can be called with only userid, with only username or with both of them, the bug I found if it's when calling it with only username.

When calling with only username the constructor work only if the user_name is placed in the first arg, but it could not work if the first arg is leaved empty and username is placed in second arg.
e.g.:
I have the user with username 'foobar' and user id 14
new WP_User(14) works
new WP_User(14,'foobar') works
new WP_User('foobar') works
new WP_User(0,'foobar') works
new WP_User(null,'foobar') doesn't work
new WP_User(false,'foobar') doesn't work

Probably the constructor is intended to be used only with the username in the first arg if the userid is not known (and its not really clear in the phpdoc that it should not be called with null,$username ), but the problem is that I found it called with null in the first argoument and the username in the second argoument in the wp3 core (i.e. is_site_admin doesn't work if called with the username as arg because of this bug, but of course I haven't checked all the references to the constructor in the core).

I see 2 possible solutions:
1) make WP_User constructor works if called with first argoument null and username in second argoument.
2) decide this is the way it should work, clearify it's doc and search all the improperly calls of in the code and correct them.

My 2 cents: go with 1) because so the constructor is more flexible and it's only a matter of adding a condition to an if statement, instead 2) means checking a lot of code (it's possible that the result it's that only is_site_admin improperly uses WP_User constructor, and btw is_site_admin doesn't actually need the WP_User obj, it could just get userdata with get_userdatabylogin ).

I'm going to attach the 1-line diff for the 1st solution

[francescolaffi]

diff for 1st solution

[francescolaffi]

Sorry for wrong formatting, here is the example in the second paragraph with correct line ends:

e.g.:
I have the user with username 'foobar' and user id 14
new WP_User(14) works
new WP_User(14,'foobar') works
new WP_User('foobar') works
new WP_User(0,'foobar') works
new WP_User(null,'foobar') doesn't work
new WP_User(false,'foobar') doesn't work

[hakre]

Replying to francescolaffi:

Sorry for wrong formatting, here is the example in the second paragraph with correct line ends: ...

Informative: the two (last) lines in your examples

new WP_User(null,'foobar') doesn't work
new WP_User(false,'foobar') doesn't work

which are tagged as "doesn't work" are not to be expected to work according to the parameters documentation:
* @param int|string $id User's ID or username

So passing array(), null, false, Exception() or whatever into there is not to be expected to work, right? (not that it isn't always valuable to properly sanitize a functions input, especially as this is related to user management).

---

Then you wrote

new WP_User(0,'foobar') works

what does this works mean? Is the user object with the userid 0 returned or the user of which is name is foobar?

[francescolaffi]

Replying to hakre:

So passing array(), null, false, Exception() or whatever into there is not to be expected to work, right? (not that it isn't always valuable to properly sanitize a functions input, especially as this is related to user management).

Yes, they are not expected to work, but probably it's not clear to everyone if it's used in that way in the wp3 core:

/wp-includes/ms-deprecated.php:51

$user = new WP_User( null, $user_login) ;

and this makes is_site_admin('username_of_a_super_admin') evalutate to false, of course it's is_site_admin's fault and not WP_User's.

I only think that making the constructor a bit more flexible with the input doesn't hurt. As long as I know there could be other points in the core where WP_User is called improperly.

but this is only IMHO and I'm not really in wp core development, do you think the best solution is the one I called '2)' in the ticket post?

Of course I haven't opened this ticked because I want to write WP_User(null,$username) in my code :)

Replying to hakre:

Then you wrote

new WP_User(0,'foobar') works

what does this works mean? Is the user object with the userid 0 returned or the user of which is name is foobar?

returns the user which name is 'foobar', makes sense because it can't exist an user with userid 0

[scribu]

While working on my GSoC project, I had to refactor the WP_User_Search class. Here's how the constructor looks currently (​r417):

	function WP_User_Search($query) {
		$this->query_vars = wp_parse_args($query, array(
			'usersearch' => '',
			'userpage' => 0,
			'role' => '',
			'orderby' => 'login',
			'order' => 'ASC',
			'users_per_page' => 50,
		));
	
		...
	}

I still think a get_users() function would be nice.

[scribu]

Please ignore my previous comment.

[hakre]

My try now to approach this. For the case the WP_User::WP_User() function should parse the input in a strict way.

[hakre]

Variant

[hakre]

That does not work (yet)

[francescolaffi]

make is_site_admin work, no changes to WP_User [ solution 2) in the first post]

[nacin]

We need to fix is_site_admin(). is_super_admin('admin') returns true, is_site_admin('admin') returns false, that's really bad for MU back compat.

I like adding empty() to WP_User.

[francescolaffi]

Replying to nacin:

We need to fix is_site_admin(). is_super_admin('admin') returns true, is_site_admin('admin') returns false, that's really bad for MU back compat.

yep and in particular situation it can also do bad jokes, i.e. in an user deletion action the is_site_admin check to prevent admins deletion failed, so the only admin got deleted, luckily it was only a test install

I like adding empty() to WP_User.

That was my first choice but the problem seems more complicated than that.
hakre noticed and tried to solve another problem of WP_User:
what happen if an user has an user_login that is a numeric value and the WP_User is called with user_login in the first argoument?

I'm doing some tests and this could be also a security problem:
i.e. if someone register with user_name '1', what happens if in a particular situation instead of doing
new WP_User($user_id) I do new WP_User($user_name)?
if the user has $user_name '1' instead of it I get the user with id 1, think what happen if if then I check has_cap on it!!
with this new fact my opinion is that it should not be possible at all calling WP_User($username) but only WP_User($user_id) or WP_User(null,$username)

about is_site_admin.diff :
I think this should be applied also if problem is solved in WP_User, actually is_site_admin doesn't need the whole WP_User because it doesn't access to caps, so using get_userdatabylogin means a smaller obj and probably one less query

[ryan]

(In [15328]) Fix is_site_admin() when passing a username. Props francescolaffi. fixes #14046

[ryan]

(In [15329]) Fix is_site_admin() when passing a username. Props francescolaffi. fixes #14046 for 3.0.1