Right now, we still use DBI directly to create the $dbh handle inside of checksetup. Instead, we should create a Bugzilla::DB object with the new DB-compatibility layer. Later, checksetup will be modified to fully support DB-independence. But this change here blocks all that work, so it needs to go in first.

OK, here is a patch to do this. It requires the DBCompat patch, of course (bug 237862). It's pretty simple, and I think it's a nicer way to do things, anyhow. :-)

(In reply to comment #1) > Created an attachment (id=173621) [edit] > Make checksetup use Bugzilla::DB and move checksetup's errors there (use -p1) > > OK, here is a patch to do this. It requires the DBCompat patch, of course (bug > 237862). It's pretty simple, and I think it's a nicer way to do things, anyhow. > :-) I haven't tested this, so I am just guessing here :-), but I think this will not work. Note that checksetup.pl is constructing the DSN from different variables ($my_db_driver <> $db_driver; $my_db_host <> $db_host; $my_db_port <> $db_port etc.). DB.pm is using Config.pm, which in turn is using localconfig file, which, by the time checksetup is run for the first time, does not have to exists yet. You need some other way how to get the parameters from checksetup.pl to DB.pm, without using localconfig. I would probably go for adding parameters to Bugzilla->dbh(). If specified, they will override the localconfig parameters (and will be specified only from checksetup.pl).

(In reply to comment #2) > I haven't tested this, so I am just guessing here :-), but I think this will not > work. Note that checksetup.pl is constructing the DSN from different variables > ($my_db_driver <> $db_driver; $my_db_host <> $db_host; $my_db_port <> $db_port > etc.). > DB.pm is using Config.pm, which in turn is using localconfig file, which, by the > time checksetup is run for the first time, does not have to exists yet. Notice that checksetup is also getting all those files from localconfig. At tihs point in checksetup, localconfig is guaranteed to exist, so we're safe to use Config.pm. The only thing that I can think of that I might actually have to change is doing a specific "require" on Bugzilla::DB. But I'll probably just end up doing that when the globals.pl require is removed.

(In reply to comment #3) > (In reply to comment #2) > > I haven't tested this, so I am just guessing here :-), but I think this will not > > work. Note that checksetup.pl is constructing the DSN from different variables > > ($my_db_driver <> $db_driver; $my_db_host <> $db_host; $my_db_port <> $db_port > > etc.). > > DB.pm is using Config.pm, which in turn is using localconfig file, which, by the > > time checksetup is run for the first time, does not have to exists yet. > > Notice that checksetup is also getting all those files from localconfig. At > tihs point in checksetup, localconfig is guaranteed to exist, so we're safe to > use Config.pm. > > The only thing that I can think of that I might actually have to change is > doing a specific "require" on Bugzilla::DB. But I'll probably just end up doing > that when the globals.pl require is removed. Well, you are right :-). The my_ variables were there just to prevent compiler errors, but as we are not using these parameters here anymore (they are used in Bugzilla::DB), compiler is happy :-). Maybe you can now remove $my_db_host, $my_db_port, $my_db_user, $my_db_sock and $my_db_pass variables, as they are not used anymore...

OK! I removed the no-longer-used $my variables from checksetup. I also discovered that checksetup would fail if it had to create the database, with my previous patch. I've fixed that by allowing a method of connect_main that doesn't connect to a database. It's documented that it should only be used for creating the database. :-)

(In reply to comment #5) > Created an attachment (id=173738) [edit] > Version 2 (use -p1) > > OK! I removed the no-longer-used $my variables from checksetup. > > I also discovered that checksetup would fail if it had to create the database, > with my previous patch. > > I've fixed that by allowing a method of connect_main that doesn't connect to a > database. It's documented that it should only be used for creating the > database. :-) Nice, good catch. Two comments: - Nit: shouldn't the parameter be a constant? ;-). - This will not work on Postgres. AFAIK you can't connect to a server and not to any DB. With Pg you are usually solving this by conecting to the system database, template1. We need to handle this in the DB specific code, but I assume this can be done by checking if the name is empty, and substituing "template1" in the Pg.pm module...

(In reply to comment #6) > - Nit: shouldn't the parameter be a constant? ;-). Hahahhaa. Yeah, I thought about that. I only use it in ONE place, though, ever. I thought it would be a waste of space to make it a constant. :-) > - This will not work on Postgres. AFAIK you can't connect to a server and not > to any DB. With Pg you are usually solving this by conecting to the system > database, template1. We need to handle this in the DB specific code, but I > assume this can be done by checking if the name is empty, and substituing > "template1" in the Pg.pm module... Oh, right... the problem is that the "bugs" user might not have access to the "template1" or "template0" database. (I'd probably use "template0," since that would more closely approximate the behavior of MySQL, where I can't do anything when I'm not connected to a DB [template0 is usually read-only].) However, DBI might handle the situation for us, also. So we'll have to see, in an actual test...

*** Bug 281364 has been marked as a duplicate of this bug. ***

One more idea came over my mind: maybe you could call directly _connect function from the checksetup module, that way you can pass any parameters you want (it will be $my_ version again I guess). It would make the code maybe a bit cleaner than using the optional parameter which shouldn't be used otherwise. That also means _connect would no longer be private, but if properly documented that is not an issue. Take this just as an idea, not a review point, and ignore it if you please :-).

(In reply to comment #9) > One more idea came over my mind: maybe you could call directly _connect function > from the checksetup module, that way you can pass any parameters you want (it > will be $my_ version again I guess). It would make the code maybe a bit cleaner > than using the optional parameter which shouldn't be used otherwise. That also > means _connect would no longer be private, but if properly documented that is > not an issue. Yeah. I thought about that, too. I figured that _connect should stay private. That way I could take those localconfig variables out of checksetup, which I very much wanted to be able to do. The fewer localconfig variables that I need in checksetup, the happier I am.

Oops, this actually blocks checked-in patches in a fashion that we didn't realize -- you can't call the Bugzilla::DB functions on $dbh because it's still DBI, not Bugzilla::DB. I KNEW there was a reason that I originally marked this as a blocker to checksetup changes.

Comment on attachment 173738 [details] [diff] [review] Version 2 (use -p1) Change the connect_main(1); to something more like connect_main("dont actually connect here"); or something more explanatory than "1" and r=joel

Cool. OK, I integrated joel's suggestion and another good suggestion from glob, which was to also display $DBI::errstr if we fail connect.

Removed connection string, because DBI is now giving us the error message quite clearly.

Comment on attachment 174694 [details] [diff] [review] v2.1 r=glob

Checking in checksetup.pl; /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v <-- checksetup.pl new revision: 1.346; previous revision: 1.345 done Checking in Bugzilla/DB.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v <-- DB.pm new revision: 1.21; previous revision: 1.20 done