Closed
Bug 282132
Opened 20 years ago
Closed 19 years ago
BugMail.pm: Eliminate deprecated Bugzilla::DB routines
Categories
(Bugzilla :: Bugzilla-General, enhancement, P3)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.22
People
(Reporter: mkanat, Assigned: bugzilla-mozilla)
References
Details
Attachments
(1 file, 3 obsolete files)
9.41 KB,
patch
|
wicked
:
review+
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
OK! This one was a bit more work, but I tested it, and it all works nicely. :-)
Reporter | ||
Updated•20 years ago
|
Attachment #174240 -
Flags: review?
Reporter | ||
Comment 2•20 years ago
|
||
Actually, this one is slightly improved.
Attachment #174240 -
Attachment is obsolete: true
Attachment #174241 -
Flags: review?
Reporter | ||
Updated•20 years ago
|
Attachment #174240 -
Flags: review?
Reporter | ||
Updated•19 years ago
|
Attachment #174241 -
Flags: review? → review?(wurblzap)
Reporter | ||
Updated•19 years ago
|
Priority: -- → P3
Comment 3•19 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•19 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•19 years ago
|
||
mkanat: This bug seems doable. Can I steal it from you?
Assignee | ||
Comment 7•19 years ago
|
||
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 8•19 years ago
|
||
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•19 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
Comment 10•19 years ago
|
||
(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•19 years ago
|
||
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 12•19 years ago
|
||
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+
Updated•19 years ago
|
Flags: approval?
Comment 13•19 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.
Updated•19 years ago
|
Flags: approval? → approval+
Comment 14•19 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
Closed: 19 years ago
Resolution: --- → FIXED
Comment 15•19 years ago
|
||
(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•19 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.
Description
•