303705 - Eliminate deprecated Bugzilla::DB routines from editkeywords.cgi

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Teemu Mannermaa (:wicked)]

These lines need to rewritten to use DBI:

editkeywords.cgi:72:    SendSQL("SELECT keyworddefs.id, keyworddefs.name,
keyworddefs.description,
editkeywords.cgi:80:    while (MoreSQLData()) {
editkeywords.cgi:81:        my ($id, $name, $description, $bugs) = FetchSQLData();
editkeywords.cgi:123:    SendSQL("SELECT id FROM keyworddefs WHERE name = " .
SqlQuote($name));
editkeywords.cgi:125:    if (FetchOneColumn()) {
editkeywords.cgi:136:    SendSQL("SELECT id FROM keyworddefs ORDER BY id");
editkeywords.cgi:140:    while (MoreSQLData()) {
editkeywords.cgi:141:        my $oldid = FetchOneColumn();
editkeywords.cgi:149:    SendSQL("INSERT INTO keyworddefs (id, name,
description) VALUES ($newid, " .
editkeywords.cgi:150:            SqlQuote($name) . "," .
editkeywords.cgi:151:            SqlQuote($description) . ")");
editkeywords.cgi:179:    SendSQL("SELECT name,description
editkeywords.cgi:182:    my ($name, $description) = FetchSQLData();
editkeywords.cgi:188:    SendSQL("SELECT count(*)
editkeywords.cgi:192:    $bugs = FetchOneColumn() if MoreSQLData();
editkeywords.cgi:222:    SendSQL("SELECT id FROM keyworddefs WHERE name = " .
SqlQuote($name));
editkeywords.cgi:224:    my $tmp = FetchOneColumn();
editkeywords.cgi:231:    SendSQL("UPDATE keyworddefs SET name = " .
SqlQuote($name) .
editkeywords.cgi:232:            ", description = " . SqlQuote($description) .
editkeywords.cgi:253:    SendSQL("SELECT name FROM keyworddefs WHERE id=$id");
editkeywords.cgi:254:    my $name = FetchOneColumn();
editkeywords.cgi:257:        SendSQL("SELECT count(*)
editkeywords.cgi:261:        my $bugs = FetchOneColumn();
editkeywords.cgi:278:    SendSQL("DELETE FROM keywords WHERE keywordid = $id");
editkeywords.cgi:279:    SendSQL("DELETE FROM keyworddefs WHERE id = $id");

Comment 1

[Frédéric Buclin]

Attachment: patch, v1

I also did some cleanup. ;)

Comment 2

[Frédéric Buclin]

Attachment: patch, v2

./runtests.pl didn't complain that '$keyword' was already in use in the
'foreach' loop. I fixed that anyway.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 193759 [details] [diff] [review]
patch, v2

>+    my $keyword_list =
>+      $dbh->selectall_arrayref('SELECT id, name, description,

  Nit: When there are multiple tables, I think it is actually nice to have each
SELECT field given a table name.

>+                                       COUNT(keywords.bug_id)
>+                                  FROM keyworddefs
>+                                  LEFT JOIN keywords

  Nit: I prefer the following spacing:

      FROM keyworddefs
 LEFT JOIN keywords

  Or just keep it like it was before (FROM keyworddefs LEFT JOIN keywords).

>-    while (MoreSQLData()) {
>-        my ($id, $name, $description, $bugs) = FetchSQLData();
>+    foreach (@$keyword_list) {

  You could use the Slice parameter of one of those selectall_ functions to
actually just create this array without this loop at all. I do it somewhere in
one of the modules... just grep for "Slice".

>+    my $id = $dbh->selectrow_array('SELECT id FROM keyworddefs
>+                                    WHERE name = ?', undef, $name);

  Nit: Generally it's nice to do "my ($id)" here, since technically
selectrow_array returns an array.

>+    my $bugs = $dbh->selectrow_array('SELECT COUNT(*) FROM keywords
>+                                      WHERE keywordid = ?',
>+                                      undef, $id) || 0;

  I'm fairly sure that COUNT(*) will return 0 if the WHERE condition fails, not
NULL. So you probably don't need the "|| 0" there.


  Otherwise this generally *looks* very nice. :-) I still have to test it, but
I'll r- it for now in case you want to fix any of the nits before I get around
to testing it. (That way I can test the patch with any nits fixed that you want
to fix.)

Comment 4

[Frédéric Buclin]

Attachment: patch, v3

Comment 5

[Frédéric Buclin]

Comment on attachment 194104 [details] [diff] [review]
patch, v3

not sure if mkanat has time for reviews these days; asking jouni.

Comment 6

[Jouni Heikniemi]

Comment on attachment 194104 [details] [diff] [review]
patch, v3

>Index: editkeywords.cgi
>===================================================================

>     Validate($name, $description);
...
>+    trick_taint($name);
>+    trick_taint($description);

Why this - why can't Validate handle detainting? Having a trick_taint apart
from the actual validating code is a bad idea. The same problem reoccurs later
in the file.

>+    my $ids = $dbh->selectcol_arrayref('SELECT id FROM keyworddefs ORDER BY id');

Nit: "$existing_ids" or something equivalent would be more readable.


I'm not r-ing this because of the trick_taint issues, but please try to fix
them prior to checkin. It seems to work and reads out OK, thus r=jouni.

Comment 7

[Dave Miller [:justdave]]

Nit: I'd prefer when you use trick_taint that you put a comment next to it
explaining why it's safe (such as that the only place it's used is as a
placeholder replacement).

But looks good otherwise

Comment 8

[Frédéric Buclin]

Attachment: patch checked in

I fixed both jouni and justdave comments.

Comment 9

[Frédéric Buclin]

Checking in editkeywords.cgi;
/cvsroot/mozilla/webtools/bugzilla/editkeywords.cgi,v  <--  editkeywords.cgi
new revision: 1.31; previous revision: 1.30
done