Closed Bug 282132 Opened 20 years ago Closed 20 years ago

BugMail.pm: Eliminate deprecated Bugzilla::DB routines

Categories

(Bugzilla :: Bugzilla-General, enhancement, P3)

2.19.2
enhancement

Tracking

()

RESOLVED FIXED
Bugzilla 2.22

People

(Reporter: mkanat, Assigned: bugzilla-mozilla)

References

Details

Attachments

(1 file, 3 obsolete files)

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).
OK! This one was a bit more work, but I tested it, and it all works nicely. :-)
Attachment #174240 - Flags: review?
Attached patch v2 (obsolete) — Splinter Review
Actually, this one is slightly improved.
Attachment #174240 - Attachment is obsolete: true
Attachment #174241 - Flags: review?
Attachment #174240 - Flags: review?
Attachment #174241 - Flags: review? → review?(wurblzap)
Priority: -- → P3
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-
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
mkanat: This bug seems doable. Can I steal it from you?
Sure, go ahead! :-)
Assignee: mkanat → bugzilla-mozilla
Attached patch Patch v3 (obsolete) — Splinter Review
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-
(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.
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?
(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+
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
Closed: 20 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.
> 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.

Attachment

General

Created:
Updated:
Size: