Closed Bug 280124 Opened 15 years ago Closed 15 years ago
New User to Bugzilla::User
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.
This one's just updated because the Crypt() patch changed.
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+
(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().
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
> # 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 on attachment 173072 [details] [diff] [review] fix checksetup breakage oops. bug 280573
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.