Move database-manipulation subroutines in checksetup to Bugzilla::DB and (eventual) subclasses

RESOLVED FIXED in Bugzilla 2.20

Status

()

enhancement
P1
normal
RESOLVED FIXED
15 years ago
7 years ago

People

(Reporter: mkanat, Assigned: mkanat)

Tracking

2.19.1
Bugzilla 2.20
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
Depends on: bz-dbcompat
Assignee: nobody → mkanat
Status: NEW → ASSIGNED
Keywords: meta
Keywords: meta
Depends on: 281360
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.
Attachment #173634 - Flags: review?
Posted patch Version 2 (obsolete) — Splinter Review
Oops. I realized that I'd forgotten to move two functions. :-)

Everything else about this patch is the same as comment 1.
Attachment #173634 - Attachment is obsolete: true
Attachment #173639 - Flags: review?
Posted patch Version 2.1 (obsolete) — Splinter Review
There was a little extra sillyness at the end of the patch that I had to
delete.
Attachment #173639 - Attachment is obsolete: true
Attachment #173640 - Flags: review?
Attachment #173634 - Flags: review?
Attachment #173639 - Flags: review?
Attachment #173640 - Flags: review? → review?(Nick.Barnes)
Priority: -- → P1
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.
Attachment #173640 - Flags: review?(Nick.Barnes)
Posted patch v3 (obsolete) — Splinter Review
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.
Attachment #173640 - Attachment is obsolete: true
Attachment #175516 - Flags: review?(bugzilla)
(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
Attachment #175516 - Flags: review?(bugzilla) → review+
Flags: approval?
Target Milestone: --- → Bugzilla 2.20
Flags: approval? → approval+
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
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Here's the version I checked-in, with the nits corrected.
Attachment #175516 - Attachment is obsolete: true
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.