checksetup has all sorts of cool functions like "DropIndexes," "GetFieldDef," and so on that tell us information about and modify the DB schema. Really, these should go into Bugzilla::DB instead, and then be overridden in the eventual subclasses of Bugzilla::DB::MySQL, Bugzilla::DB::Pg, etc. This will also help with Schema manipulation and making checksetup compatible across DBs.

OK, here we go! This looks like a big patch, but it's really a trivially simple change. It just involves a lot of renaming stuff. I tested the code by upgrading from a 2.14 database, and everything works fine. This code depends on both bug 237862 and bug 281360.

Oops. I realized that I'd forgotten to move two functions. :-) Everything else about this patch is the same as comment 1.

There was a little extra sillyness at the end of the patch that I had to delete.

I had a look at this, it looks really good, I really want this in. I found only two nits: The functions still have the same prototype (e.g. '$$$') with the same number of parameters, but they now take $self parameter, so they should take one more. But as we are calling them as methods and the prototype does not apply here (if I understand perl docs correctly), it does not matter. I think the protoypes should go completelly, but that can be part of the next patch, implementing db compatibility for these functions. As the patch bitrotted, I was not able to verify you found all places these functions were called. I found few which were not replaced at the bottom of checksetup, but that may be result of the bitrott. If you update the patch and make sure all calls are replaced, it's r+ from me (FWIW :-) ).

Comment on attachment 173640 [details] [diff] [review] Version 2.1 OK, I'll update the patch.

OK, here's the new version, that's updated to actual tip checksetup code. This will rot the instant that somebody touches checksetup, pretty much.

(In reply to comment #4) > I think the protoypes should go completelly, Why remove the prototypes? They give us useful error-checking.

Oh, by the way, I tested version 3 up there using my regression-test script against the most common old databases, and it definitely still works.

(In reply to comment #7) > (In reply to comment #4) > > I think the protoypes should go completelly, > > Why remove the prototypes? They give us useful error-checking. Citing perlsub documentation (http://perldoc.perldrunks.org/perlsub.html): ------------ The prototype affects only interpretation of new-style calls to the function, where new-style is defined as not using the & character. In other words, if you call it like a built-in function, then it behaves like a built-in function. If you call it like an old-fashioned subroutine, then it behaves like an old-fashioned subroutine. It naturally falls out from this rule that prototypes have no influence on subroutine references like \&foo or on indirect subroutine calls like &{$subref} or $subref->() . Method calls are not influenced by prototypes either, because the function to be called is indeterminate at compile time, since the exact code called depends on inheritance. ------------ Note the last paragraph. Which means it is at most for documentation only. And we already have a documentation in the pods...

Oh, OK. So subroutines still benefit from prototypes, but methods don't. So if we have methods, we can remove their prototypes. Darn. /me taps his foot impatiently for perl6... :-)

glob, if you could review this soon, it would help a lot. We only have 15 or 16 days until feature freeze.

i'm not a big fan at all of the bz_ prefix for all the methods. any particular reason for this?

It's the standard for the module. We extend DBI, so we have to have a way of not conflicting with their namespace.

BTW, that little block at the top is part of another patch that has already been checked-in. It can just be removed on checkin, if we get review+ on this.

Comment on attachment 175516 [details] [diff] [review] v3 ok, this all looks ok, mainly a search & replace job. >- name => 'Text::Autoformat', >- version => '0' >+ name => 'Text::Wrap', >+ version => '2001.0131' like you said, oops >+$dbh->bz_rename_field ('bugs_activity', 'when', 'bug_when'); nit: no space before ( this occurs several times >+# XXX - This shouldn't print stuff to stdout actually i like the fact that it outputs schema changes. >+=item C<bz_drop_field> >+ >+ Description: Removes a column from a database table. If the column >+ doesn't exist, we return witout doing anything. If we do nit: witout --> without >+=item C<bz_get_field_def> >+ >+ Description: Returns information about a column in a table in the database. >+ Params: $table = name of table containing the column (scalar) >+ $column = column you want info about (scalar) >+ Returns: An reference to an array containing information about the >+ field, with the following information in each array place: >+ 0 = column name >+ 1 = column type >+ 2 = 'YES' if the column can be NULL, empty string otherwise >+ 3 = The type of key, either 'MUL', 'UNI', 'PRI, or ''. >+ 4 = The default value >+ 5 = ? http://dev.mysql.com/doc/mysql/en/describe.html says it's an "extra" column. doesn't help much. r=glob with the nits fixed

Checking in checksetup.pl; /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v <-- checksetup.pl new revision: 1.357; previous revision: 1.356 done Checking in Bugzilla/DB.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v <-- DB.pm new revision: 1.23; previous revision: 1.22 done

Here's the version I checked-in, with the nits corrected.