280124 - Move InsertNewUser to Bugzilla::User

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, enhancement)

Description

[Max Kanat-Alexander]

This is another globals.pl function that's got to go. It will also get renamed
to match the Bugzilla::User style, to insert_new_user, and it will have SendSQL
calls changed to DBI calls.

Comment 1

[Max Kanat-Alexander]

This patch will modify code also modified by the (not-yet-checked-in) patch on
bug 278792, so it depends on that patch.

Comment 2

[Max Kanat-Alexander]

Attachment: Move and fix up InsertNewUser (use -p1)

OK, here's the patch. At the moment, it depends on the patch from bug 278792.
(Meaning, you have to install that one first.)

It's pretty simple. I tested that it works in createaccount.cgi, and I tested
that LDAP.pm still compiles correctly. (The code is almost identical, so there
shouldn't be any problems with LDAP.pm.)

This isn't a CVS diff, because I had to diff against an installation with bug
278792 installed.

Comment 3

[Max Kanat-Alexander]

Attachment: Version 1.1 (use -p1)

This one's just updated because the Crypt() patch changed.

Comment 4

[Vlad Dascalu]

Comment on attachment 172774 [details] [diff] [review]
Version 1.1 (use -p1)

I wonder if it would have been better to EXPORT it by default instead of
EXPORT_OK it.

Also, another "crazy" idea would have been to have a constructor for the User
type, and to create a new user object based on those 2 params ($username and
$userRealName) instead of calling an arbitrary "static" function. All subs in
Bugzilla::User are actually, AFAIK, methods for the user type, in the sense
that they receive as the first argument a user object and they do my $self =
shift. Probably it would have made sense to analyse if insert_new_user can
actually be made something similar with the _create sub from Bugzilla::User.
Although there is a difference between those two (_create makes the user object
based on already existing DB entry and insert_new_user makes a new user only
from those 2 fields only), maybe there are enough similarilies also that allow
this one to be written based on OO style.

Anyway, for Mod_perl support, this rocks! r=vladd

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> I wonder if it would have been better to EXPORT it by default instead of
> EXPORT_OK it.

  Well, the perldoc for Exporter recommends to always use EXPORT_OK instead of
EXPORT, if you can. So that's why I did that.

> Also, another "crazy" idea would have been to have a constructor for the User
> type, and to create a new user object based on those 2 params ($username and
> $userRealName) instead of calling an arbitrary "static" function.

  Yeah, I fully agree. :-) When I designed an OO system with Bugzilla-like
features, in the past, I had this interface on all my objects:

  $object = Class.read($id) -- Returns an object from the database, given its id.

  $object = Class.create($name, $blah, $blah...) -- Creates a new object, in the
database, and returns the created object.

  $object = Class.new($name, $blah, $blah...) -- Constructs an object. Normally
only called by create() or read().

Comment 6

[Shane H. W. Travis]

Checking in createaccount.cgi;
/cvsroot/mozilla/webtools/bugzilla/createaccount.cgi,v  <--  createaccount.cgi
new revision: 1.35; previous revision: 1.34
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.292; previous revision: 1.291
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.33; previous revision: 1.32
done
Checking in Bugzilla/Auth/Verify/LDAP.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify/LDAP.pm,v  <--  LDAP.pm
new revision: 1.4; previous revision: 1.3
done

Comment 7

[:glob ✱]

Attachment: fix checksetup breakage

> # It's safe to use Bugzilla::Auth here because parameters have now been
> # defined.
> use Bugzilla::Auth;

ah, no.  "use" is evaluated at compile time, as as Bugzilla::Auth accesses
param('user_verify_class') in a BEGIN block, that also happens at compile time.


net result - new installs can't run checksetup: "No value for param
user_verify_class"

Comment 8

[:glob ✱]

Comment on attachment 173072 [details] [diff] [review]
fix checksetup breakage

oops.  bug 280573

Comment 9

[Frédéric Buclin]

Comment on attachment 173072 [details] [diff] [review]
fix checksetup breakage

Byron, there is already a bug for that, with exactly the same patch submitted,
see bug 280573.

As you seem to agree with mkanat's patch, you could maybe grant review there?
;)