Closed Bug 280124 Opened 15 years ago Closed 15 years ago

Move InsertNewUser to Bugzilla::User

Categories

(Bugzilla :: User Accounts, enhancement)

2.19.2
enhancement
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: mkanat, Assigned: mkanat)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
This patch will modify code also modified by the (not-yet-checked-in) patch on
bug 278792, so it depends on that patch.
Status: NEW → ASSIGNED
Depends on: 278792
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.
Attachment #172628 - Flags: review?
This one's just updated because the Crypt() patch changed.
Attachment #172628 - Attachment is obsolete: true
Attachment #172774 - Flags: review?
Attachment #172628 - Flags: review?
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
Attachment #172774 - Flags: review? → review+
Flags: approval?
Target Milestone: --- → Bugzilla 2.20
(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().
Blocks: 119485
Flags: approval? → approval+
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
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attached patch fix checksetup breakage (obsolete) — Splinter Review
> # 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"
Attachment #173072 - Flags: review?
Comment on attachment 173072 [details] [diff] [review]
fix checksetup breakage

oops.  bug 280573
Attachment #173072 - Flags: review?
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?
;)
Attachment #173072 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.