This appears to be a type comparison issue. Check around line 223 of /ibraries/joomla/factory.php:

if ($current->id != $id)

$current->id will always be an integer. When we pass a string to $id, we're comparing an integer to a string. In PHP, the == operator (or !=) will convert a string to a number before doing the comparison. A non-numeric string gets converted to zero which in this case is equivalent. So, we will always pass back the session user instead of fetching a user by username.

For security reasons, I probably wouldn't do any error output if 'someuser' isn't found.

I wouldn't output the error :) But partially I would like the docs to work as they say they do and mainly - I had a list of potential usernames and I wanted to see which tied up with usernames on my site and which didn't. That was my overall aim for this code.

Moved the commits over onto the CMS side

Good catch, I never noticed this inconsistent behaviour. I decided to try a few things myself and found a few minor issues with the patch. One of these is that the ID is only checked in the JFactory::getUser() method from the session. I would propose something along the lines of: commit f586732 (embedded below - I don't know how to correctly link) with commit 4db79a5 you proposed.

$instance = self::getSession()->get('user');

if (is_null($id))
{
 if (!($instance instanceof JUser))
 {
  $instance = JUser::getInstance();
 }
}
elseif (!$instance->id)
{
 $instance = JUser::getInstance($id);
}
 elseif (is_numeric($id) ? ($instance->id != $id) : ($instance->username != $id))
{
 $instance = JUser::getInstance($id);
}

return $instance;

Something I also witnessed is the inconsistency of a "blank" JUser object which related to whether JUser::load() with an invalid ID is used - though I believe that doesn't belong in this pull.

@wilsonge what is the status of this please?

hell. this was my first ever pr to joomla :P there's all kinds of b/c issues with this but it's also completely different to what the docs say.......

I really have no clue what to do with it. @mbabker / @dbhurley / @Bakual / any other PLT dudes dunno if you guys have any comments?

My first thought is probably to close the PR until it's ready for merging.
:)

Congrats on finding your first! You've done a lot of great things since.
On Jul 23, 2014 8:17 AM, "George Wilson" notifications@github.com wrote:

hell. this was my first ever pr to joomla :P there's all kinds of b/c
issues with this but it's also completely different to what the docs
say.......

I really have no clue what to do with it. @mbabker
https://github.com/mbabker / @dbhurley https://github.com/dbhurley
dunno if you guys have any comments?

—
Reply to this email directly or view it on GitHub
#811 (comment).

It is ready for merging. So the PR now follows what the docs says. And what I'd say is more intuitively correct. But technically that comes with a b/c incompatibility..... (which is currently why the test fails - the test is really easy to fix if people are happy with it but point stands)

Travis yells at your first PR
The B/C would be that you now return an empty JUser instead of the wrong "false". There is another case a bit later in code where an empty JUser is returned. So any extension using this probably already can deal with an empty JUser object and thus nothing should break in theory.
Is there another B/C?

Not that I'm aware of - expect you pass in an ID and it will actually return the user object now you requested now :P

btw travis wasn't being used when my first pr was made :P

btw travis wasn't being used when my first pr was made

I'm aware of that

@test: After applying the PR I get the information about the user correctly.
You may blame the J!Tracker Application at http://issues.joomla.org/ for transmitting this comment.

@test all good! I get the information about the users correctly.

This comment was created with the J!Tracker Application at http://issues.joomla.org/.

thanks @b2z moving to RTC

This comment was created with the J!Tracker Application at http://issues.joomla.org/.

First ever PR to Joomla RTC. I can now stop :P :P

I can now stop

Don't you dare!

More I am thinking about it more my mind is blown

No because it's a or statement. the entire thing is true if one side or the other is true. It's only with an and statement that that's the case

Ok shame on me... So if ID is a string then the entire thing is true and we do not compare ids at all?

Why was my PR https://github.com/beat/joomla-cms/compare/wilsonge:userjfact...patch-6?quick_pull=1 ignored ? anything wrong with it ?

The is_string test above is not right, as it ignores numeric strings that should be handled as ids for B/C.

This is security-relevant stuff, so we should not take it lightly.

Beat I didn't ignore you PR at all. If you look I commented on it after you pasted the link above.

@wilsonge thanks for the reply. Missed it as it got hidden by github together with my line-comment after the line got changed.

Is this RTC ? Travis failed?

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/811.

Travis fixed

OK setting to RTC then

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/811.

Can you make it a PR towards staging please?

Merged into staging. Thanks!

And with that closed the oldest still open PR

I quit! My work here is done :P

I would be very surprised if you did!

Shouldn't there be any check before checking $instance->id in elseif (is_string($id) || $instance->id !== $id) ?

#3104