BugMail.pm: Eliminate deprecated Bugzilla::DB routines

RESOLVED FIXED in Bugzilla 2.22

Status

()

P3
enhancement
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: mkanat, Assigned: bugzilla-mozilla)

Tracking

2.19.2
Bugzilla 2.22
Bug Flags:
approval +

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

14 years ago
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).
(Reporter)

Comment 1

14 years ago
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. :-)
(Reporter)

Updated

14 years ago
Attachment #174240 - Flags: review?
(Reporter)

Comment 2

14 years ago
Created attachment 174241 [details] [diff] [review]
v2

Actually, this one is slightly improved.
Attachment #174240 - Attachment is obsolete: true
Attachment #174241 - Flags: review?
(Reporter)

Updated

14 years ago
Attachment #174240 - Flags: review?
(Reporter)

Updated

14 years ago
Attachment #174241 - Flags: review? → review?(wurblzap)
(Reporter)

Updated

14 years ago
Priority: -- → P3

Comment 3

14 years ago
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.
Attachment #174241 - Flags: review?(wurblzap) → review-
(Reporter)

Comment 4

14 years ago
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.
Target Milestone: --- → Bugzilla 2.22
(Assignee)

Comment 5

13 years ago
mkanat: This bug seems doable. Can I steal it from you?
(Reporter)

Comment 6

13 years ago
Sure, go ahead! :-)
Assignee: mkanat → bugzilla-mozilla
(Assignee)

Comment 7

13 years ago
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).
Attachment #174241 - Attachment is obsolete: true
Attachment #201046 - Flags: review?(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.
Attachment #201046 - Flags: review?(wicked) → review-
(Assignee)

Comment 9

13 years ago
(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}.
Status: NEW → ASSIGNED
(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.
(Assignee)

Comment 11

13 years ago
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
Attachment #201046 - Attachment is obsolete: true
Attachment #201258 - Flags: review?(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.
Attachment #201258 - Flags: review?(wicked) → review+
Flags: approval?

Comment 13

13 years ago
(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.
Flags: approval? → approval+

Comment 14

13 years ago
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.54; previous revision: 1.53
done
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(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.
(Reporter)

Comment 16

13 years 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.

  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.
You need to log in before you can comment on or make changes to this bug.