Using DBI methods

[Bradley Baetz]

This got kind of long, but do read it all carefully.

In bug 163290, I'm moving the various DB stuff into a separate file, 
Bugzilla/DB.pm.

For the moment, this is being transferred fairly literally into an OO 
infrastructure.

However, it seems to me that it may be worth being able to use the DBI
methods directly. |perldoc DBI| lists those, for the interested.

Advantages:

1) Can use stuff like:
  my ($a, $b) = $dbh->selectrow_array("SELECT a,b FROM foo WHERE 
                                       prim_key=7");

or selectall_arrayref, and so on

which is nicer/cleaner/faster/easier to read than our sendsql+fetch stuff

2) We get to use DBI's error handling + exceptions - currently we only 
check for errors in one place, which is bad.

Note that all of these can reasonably easily be added to the current 
setup, but it starts to become less worthwhile/more complicated/etc as we 
go on

3) Can be faster - by using the routines as written in C in the DBI/DBD
  - unlikely to be really noticable since we don't tend to move large 
volumes of data to/from the db, but every bit helps.

4) No more state stack
  - don't have to worry about forgetting to push/pop the state stack -
simply get a new statement handle when you need one from teh globally 
available dbh when needed.

Disadvantages:

1) New syntax required
  - We can (and will) keep the old syntax arround for backwards 
compatability, though, for the reasonably future.
  - Mixing FetchOneColumn() and fetchrow_arrayref (for example) on ehte 
same statement handle won't work, because of the internal buffer 
MoreSQLData needs to determine emptyness.

2) Its slightly messier
  - Need to keep statment objects arround explicitly, rather than 
implicitly

3) We need to up the required DBI version
  - The currently required version dates to July 1999 (1.13). To use the 
HandleError stuff to do custom SQL error messages, we need 1.21 (Feb 2002)
  - Its also needed to subclass DBI, which is what we need for taint 
checking, and supporting the old (current) syntax (speaking of which, I 
should email the dbi list about the taint stuff...)
  - It has a lot of the cross-db stuff for transactions which we will need 
at some point
  - DBI-1.21 comes with debian stable and RH7.2/7.3 at least. Its not that 
hard to install it though, anyway

  - If DBI does add a 'TaintInOnly' mode, then we'll require that at some 
point, although probably not right away (or even at all for 2.18)

4) we need to up the required perl version to 5.005_03
  - that DBI version requires 5.005_03. We've been talking about it for a 
while, and 5.8 is now out, so requring the latest 5.005 release shouldn't 
be an issue, although I know that afranke will disagree...
  - Do note that the latest DBI version has a note that it may not support
5.005 for much longer. This may become an issue for us at some point in
the future.

What do people think about the above?

'Feature' removal:

I also plan to remove the following two features:

a) SQL log should indent the log as statements are pushed to the stack
  - If we no longer have a stack and have sth's which can be finshed in 
    an order different to when they were started, this is a pain to do.
  - The results don't really look any better, because very very very few of
    our SQL statements fit into 80 columns, let alone 80-8*depth. so it
    wraps anyway
  - More importantly, this doesn't actually work. It took me 15 minutes to 
    work out why not, and it hasn't worked since it was checked in almost 
    two years ago. Why? Because the case in |SavedSQLStates| is wrong.
    Why didn't this trigger a 'used only once' warning? Because it got
    added to the silliness stuff in a warning fix-up (bug 72862) :)

b) In theory, Param("shutdownhtml") will currently work even for scripts 
which don't |require 'CGI.pl'|.

Pre-templatisation/2.16, the check was in PutHeader, which was too 
late (and not called for non-CGIs anyway) Now, its in CGI.pl's toplevel 
stuff.

The current behaviour of having MoreSQLData() return 0 is a pain to 
re-implement, plus its buggy, and may cause stuff to happen which 
shouldn't (eg what if one of these calls is trying to regenerate the 
versioncache, or its a security check, or something?)

(I just checked, and regenerating the versioncache with shutdownhtml set 
does result in one which is empty.)

Eventually this will happen somewhere apart from CGI.pl, but for the
moment... This will cause different behaviour in non-cgi stuff, but I
consider an error (which Bugzilla::DB::connect will generate) to be 
preferable than generating bogus data/empty versioncache/etc

I'd be incredibly surprised if any of those programs work with shutdown 
html anyway - collectstats, for example, dies when it can't read from the 
product table a product which is in the versioncache.

(I'm tossing up between |connect| returning undef, dieing, or throwing an 
exception, and leaning towards the exception, which will basically mean a 
die)

If you object to the removal of any of the above please let me know ASAP.

Bradley