DBID_to_name() can probably be replaced by Bugzilla::User->new($user_id)->name. We just have to make sure we don't break the code when we passed an unknown or invalid user ID.

(In reply to comment #0) > DBID_to_name() can probably be replaced by Bugzilla::User->new($user_id)->name. > We just have to make sure we don't break the code when we passed an unknown or > invalid user ID. Shouldn't that be "new Bugzilla::User($user_id)->login" instead of ->name?

(In reply to comment #1) > Shouldn't that be "new Bugzilla::User($user_id)->login" instead of ->name? err... ->login instead of ->name, yes. But "new Bugzilla::User($user_id)->login" won't work, you have to write "Bugzilla::User->new($user_id)->login".

The patch assumes the one about get_product_* to be checked in first (else you have a conflict in globals.pl). Note that I use a user cache almost everywhere.

Hum, I have to admit that my patch doesn't make sure the user exists, i.e. $user->login may fail if the user account has been deleted. On the other hand, we should fine a more efficient way to write: $foo = $user ? $user->login : '';

I fixed all the places where a deleted user account could be a problem. The ones that are left untouched have already been validated.

Comment on attachment 226083 [details] [diff] [review] patch, v2 + # still valid before looking for there login name. Else we would crash. "there" is incorrect in this context. + # Avoid extraneaous SQL queries. Google says: "Did you mean: extraneous"

Comment on attachment 226083 [details] [diff] [review] patch, v2 >Index: process_bug.cgi > # from buglist.cgi, or just the one bug when called from > # show_bug.cgi). > # >+# We are going to handle several user objects, maybe several times >+# the same ones. It worths a cache. >+my %users; "Maybe several times the same ones" doesn't make a lot of sense. Also: "It's worth" Also, premature optimization is the root of all evil. Instead of all this either/or stuff, have you considered making a login_name exported function from Bugzilla::User that just does DBID_to_name without the fancy cache, and returns the same thing that DBID_to_name did? We'll eventually be replacing most of those calls with User obects anyhow, but right now it's silly to create a User object just to get the login name out of it, and look at all the code complexity it creates!

Comment on attachment 226164 [details] [diff] [review] patch, v3 I'd feel better with the function rejecting values like "01" or "0001" (numbers with leading zeros), as the previous version of the function did, but it seems this is not critical and it works because we're not inserting into DB IDs received from user input. +Returns the login name of the user account for the given user ID. If no +valid user ID is given or has no entry in the profiles table, we return +an emtpy string. "emtpy" => "empty" "or has" => "or the user has" (fixable on checkin)

Checking in editproducts.cgi; /cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v <-- editproducts.cgi new revision: 1.123; previous revision: 1.122 done Checking in globals.pl; /cvsroot/mozilla/webtools/bugzilla/globals.pl,v <-- globals.pl new revision: 1.371; previous revision: 1.370 done Checking in process_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v <-- process_bug.cgi new revision: 1.327; previous revision: 1.326 done Checking in Bugzilla/BugMail.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v <-- BugMail.pm new revision: 1.79; previous revision: 1.78 done Checking in Bugzilla/User.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v <-- User.pm new revision: 1.112; previous revision: 1.111 done Checking in contrib/bug_email.pl; /cvsroot/mozilla/webtools/bugzilla/contrib/bug_email.pl,v <-- bug_email.pl new revision: 1.41; previous revision: 1.40 done