Closed
Bug 282132
Opened 20 years ago
Closed 20 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•20 years ago
|
Attachment #174241 -
Flags: review? → review?(wurblzap)
Reporter | ||
Updated•20 years ago
|
Priority: -- → P3
![]() |
||
Comment 3•20 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•20 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•20 years ago
|
||
mkanat: This bug seems doable. Can I steal it from you?
Assignee | ||
Comment 7•20 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•20 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•20 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•20 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•20 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•20 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•20 years ago
|
Flags: approval?
![]() |
||
Comment 13•20 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•20 years ago
|
Flags: approval? → approval+
![]() |
||
Comment 14•20 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: 20 years ago
Resolution: --- → FIXED
Comment 15•20 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•20 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
•