282132 - BugMail.pm: Eliminate deprecated Bugzilla::DB routines

Status: RESOLVED FIXED

Description

[Max Kanat-Alexander]

Time to tackle BugMail! I figured this would be a good one to do since it's
frequently-used code (that is, in terms of end-user usage).

Comment 1

[Max Kanat-Alexander]

Created attachment 174240 [details] [diff] [review]
Change BugMail SendSQL calls to Bugzilla::DB calls

OK! This one was a bit more work, but I tested it, and it all works nicely. :-)

Comment 2

[Max Kanat-Alexander]

Created attachment 174241 [details] [diff] [review]
v2

Actually, this one is slightly improved.

Comment 3

[Frédéric Buclin]

Comment on attachment 174241 [details] [diff] [review]
v2

4 out of 12 hunks FAILED -- saving rejects to file Bugzilla/BugMail.pm.rej

Oops! :(

mkanat, update this patch and ask me to review it! We still have a chance to
get it for 2.20.

Comment 4

[Max Kanat-Alexander]

I tried to un-bitrot it the other day, and it just was too much work for 2.20, I
think. BugMail has changed significantly, and will change significantly again
when emailprefs goes in.

Comment 5

[Olav Vitters]

mkanat: This bug seems doable. Can I steal it from you?

Comment 6

[Max Kanat-Alexander]

Sure, go ahead! :-)

Comment 7

[Olav Vitters]

Created attachment 201046 [details] [diff] [review]
Patch v3

Slightly based on patch v2 by mkanat, this is why I'm calling it v3.

Comments:
I like the hashref to fill %value. However, wasn't too sure about the best way to make the SQL statement 'layout' look nice (I put lastdiffed etc on another row because max 80 cols).

Wasn't sure if I should change everything to $values{start}, $values{end} or the $start, $end. Decided minimal code changes where best.

Also changed some $dbh SQL statements that didn't use '?' (I think it belongs to this bug).

Comment 8

[Teemu Mannermaa (:wicked)]

Comment on attachment 201046 [details] [diff] [review]
Patch v3

>Index: Bugzilla/BugMail.pm
>===================================================================
>+    my $sth = $dbh->prepare('SELECT name, description, mailhead 
>+                             FROM fielddefs ORDER BY sortkey');
>+    $sth->execute();
>+    while (my ($field, $description, $mailhead) = $sth->fetchrow_array()) {

Instead of prepare/execute/fetchrow_array, please combine them in one selectall_arrayref call and use a foreach loop. For an example, see editproducts.cgi line 536.

>+    my %values = %{$dbh->selectrow_hashref(
>+        'SELECT ' . join(',', @::log_columns) . ',
>+                lastdiffed AS start, NOW() AS end
>+           FROM bugs WHERE bug_id = ?',
>+        undef, $id)};

Hmm, I don't get this %{} around the DBI call. What's it for, is it really necessary? Same comment for few @{} that are used later in the patch.

>+    my ($start, $end) = ($values{start}, $values{end});


>+          ORDER BY bugs_activity.bug_when",
>+           undef, @args)};

Nit: I think this undef line could be moved to previous line.

>+            $diffpart->{'isprivate'} = $dbh->selectrow_array(
>+                'SELECT isprivate FROM attachments WHERE attach_id = ?',
>+                undef, ($attachid));

You should surround $diffpart->{'isprivate'} with () because selectrow_array always returns an array. I think this was discussed on the mailing list a while ago.

>+    $sth = $dbh->prepare(
>+    $sth->execute(@args);
>+    while (my ($depbug, $summary, $what, $old, $new) = $sth->fetchrow_array()) {

Again, combine these with one selectrow_arrayref call.

>+    my $voters = $dbh->selectcol_arrayref(
>+        "SELECT who FROM votes WHERE bug_id = ?",
>+        undef, ($id));

Nit: I think here too the undef line could be combined with previous line.

Comment 9

[Olav Vitters]

(In reply to comment #8)
> >+    my %values = %{$dbh->selectrow_hashref(
> >+        'SELECT ' . join(',', @::log_columns) . ',
> >+                lastdiffed AS start, NOW() AS end
> >+           FROM bugs WHERE bug_id = ?',
> >+        undef, $id)};
> 
> Hmm, I don't get this %{} around the DBI call. What's it for, is it really
> necessary? Same comment for few @{} that are used later in the patch.

Those DBI calls return references. The code currently assumes normal hashes and arrays (not references). I use %{} and @{} to deref these so the current code stays the same.

Should I not do %{} / @{} and just change the code? These are small changes except for %values. Changing %values{foo} to $values->{foo} will mean a lot of code changes.

> >+    my ($start, $end) = ($values{start}, $values{end});

Keeping $start and $end is ok? I can change every $start and $end usage to $values{start} and $values{end}.

Comment 10

[Teemu Mannermaa (:wicked)]

(In reply to comment #9)
> Those DBI calls return references. The code currently assumes normal hashes and
> arrays (not references). I use %{} and @{} to deref these so the current code
> stays the same.

Ah, makes sense now.

> Should I not do %{} / @{} and just change the code? These are small changes
> except for %values. Changing %values{foo} to $values->{foo} will mean a lot
> of code changes.

I think it would be better to make the small changes but keep the dereference for %values.

> > >+    my ($start, $end) = ($values{start}, $values{end});
> Keeping $start and $end is ok? I can change every $start and $end usage to
> $values{start} and $values{end}.

I think it's better to keep $start and $end variables.

Comment 11

[Olav Vitters]

Created attachment 201258 [details] [diff] [review]
Patch v4: Fix comments from review

Changes:
 * No more prepare stuff
 * Removed @{} except for %values + changed code that relied on it.
 * Put a few 'undef, $foo);' on the previous line
 * Use ($foo) with selectrow_array

Comment 12

[Teemu Mannermaa (:wicked)]

Comment on attachment 201258 [details] [diff] [review]
Patch v4: Fix comments from review

Tested to work with both changed and new bugmail. Also flagmail.

Comment 13

[Frédéric Buclin]

(In reply to comment #8)
> You should surround $diffpart->{'isprivate'} with () because selectrow_array
> always returns an array. I think this was discussed on the mailing list a while
> ago.

What was discussed is that selectrow_array() returns either an array *or* a scalar depending on the context. I personally never add parens in this case, for instance.

Comment 14

[Frédéric Buclin]

Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.54; previous revision: 1.53
done

Comment 15

[Teemu Mannermaa (:wicked)]

(In reply to comment #13)
> > You should surround $diffpart->{'isprivate'} with () because selectrow_array
> What was discussed is that selectrow_array() returns either an array *or* a
> scalar depending on the context. I personally never add parens in this case,
> for instance.

But you did use parens in bug 303703 that had similar construct. That line in editflagtypes.cgi was mostly the reason why I commented in my review about this.

Comment 16

[Max Kanat-Alexander]

> What was discussed is that selectrow_array() returns either an array *or* a
> scalar depending on the context. I personally never add parens in this case,
> for instance.

  Actually, it depends on the DBD. So you should add parens. Some DBDs may always return an array, causing us to have to do a lot of cleanup.