JUser Again (getInstance)

[Paladin]

If JUser can't find the requested user in the db, it hits the
following code:

if (!$id = JUserHelper::getUserId($id)) {
JError::raiseWarning('SOME_ERROR_CODE', 'JUser::_load: User '.
$id.' does not exist');
$retval = false;
return $retval;
}

Look closely at the code. Even disregarding the omnipresent
"SOME_ERROR_CODE', it's not possible for this error message to give
you useful information, beyond that it couldn't find whatever the user
id was it was supposed to be looking for. It *can't* tell you what
user id it was looking for, because the requested ID is overwritten by
the return from the search call, so it will *always* be zero at this
point, no matter what the original ID was.

I'm assuming this isn't intentional, so two recommendations, based on
different assumptions about the error message:

1) You don't care if the ID isn't given, or even think perhaps that
giving the ID might give away some security info: Change the error
message to reflect this by dropping out the $id var.

2) You want the error message to remain as is but actually be
informative: Either store the original ID in a separate variable and
connect that var to the error message or store the return in a temp
variable, only transferring it to $id after you know its valid.

Parenthetical note: Using double quotes gets rid of the two
concatenation operations. As for performance, see http://phpbench.com
(last performance comparison chart).

Future Reference:

You want things like this reported here or in tracker? Or do you want
me to just sit on this stuff and submit a patch file here to fix them
at the same time as I commit the test file? (If the latter, and you
don't want option 2, tell me now, because that's the one I'll choose
if left to my own devices.)

[Louis Landry]

Hi Paladin,

That should definitely be more informative.

On the single vs. double quotes remark: http://blog.libssh2.org/index.php?/archives/28-How-long-is-a-piece-of-string.html 

That is what I have used as a benchmark for how to treat strings.  I also find it easier to read generally.

I think for me personally it would be easier to handle the patches in batches then lots of very smallish ones one at a time, but ultimately whatever serves you better would be fine.

- Louis

--
Development Coordinator
Joomla! ... because open source matters.
http://www.joomla.org

[Paladin]

On Nov 23, 11:20 am, Louis Landry <louis.lan...@joomla.org> wrote:

> On the single vs. double quotes remark:http://blog.libssh2.org/index.php?/archives/28-How-long-is-a-piece-of...

>
> That is what I have used as a benchmark for how to treat strings.  I also
> find it easier to read generally.

Thanks for the pointer. Easier to read is good. It's probably a matter
of acclimatization on that point. I used to squick at the braces
required version ("{$this->orThat}") but was working on an assignment
where that was the in-house style, so wrote it. After a while I
decided it actually was easier to read that than the concatenated
version, because I could read the whole thing as a string, and not
break out of string to concatenate and then back into the string
again. I got used to reading that, so it now seems easier to me.

> I think for me personally it would be easier to handle the patches in
> batches then lots of very smallish ones one at a time, but ultimately
> whatever serves you better would be fine.

Full file of patches is probably easier, otherwise I need to maintain
multiple versions to do the diffs on to create the patches. But the
question then becomes should I raise the questions or just make the
changes and post the patch file?