Closed Bug 303688 Opened 19 years ago Closed 18 years ago

Eliminate deprecated Bugzilla::DB routines from attachment.cgi

Categories

(Bugzilla :: Bugzilla-General, enhancement)

Product:
Bugzilla
Component:
Bugzilla-General
Version:
2.21
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 3.0

People

(Reporter: wicked, Assigned: gregaryh)

References

Details

Attachments

(1 file, 3 obsolete files)

V4
25.48 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review 
These lines need to rewritten to use DBI:

attachment.cgi:153:    SendSQL("SELECT bug_id, isprivate FROM attachments WHERE
attach_id = $attach_id");
attachment.cgi:154:    MoreSQLData()
attachment.cgi:158:    (my $bugid, my $isprivate) = FetchSQLData();
attachment.cgi:206:    SendSQL("SELECT attach_id FROM attachments WHERE " .
attachment.cgi:209:    FetchSQLData()
attachment.cgi:217:    SendSQL("SELECT product_id
attachment.cgi:222:    my $productid = FetchOneColumn();
attachment.cgi:231:    SendSQL("SELECT product_id
attachment.cgi:234:    my $productid = FetchOneColumn();
attachment.cgi:403:    SendSQL("SELECT bug_id, isobsolete, description
attachment.cgi:407:    MoreSQLData()
attachment.cgi:410:    my ($bugid, $isobsolete, $description) = FetchSQLData();
attachment.cgi:477:    SendSQL("SELECT mimetype, filename, thedata FROM
attachments " .
attachment.cgi:479:    my ($contenttype, $filename, $thedata) = FetchSQLData();
attachment.cgi:601:  SendSQL("SELECT bug_id, description, ispatch, thedata FROM
attachments WHERE attach_id = $id");
attachment.cgi:602:  my ($bugid, $description, $ispatch, $thedata) = FetchSQLData();
attachment.cgi:731:  SendSQL("SELECT bug_id, description, ispatch, thedata FROM
attachments " .
attachment.cgi:733:  my ($bugid, $description, $ispatch, $thedata) = FetchSQLData();
attachment.cgi:761:      SendSQL("SELECT attach_id, description FROM attachments
WHERE bug_id = $bugid AND ispatch = 1 ORDER BY creation_ts DESC");
attachment.cgi:763:      while (my ($other_id, $other_desc) = FetchSQLData()) {
attachment.cgi:800:    SendSQL("SELECT attach_id, " .
attachment.cgi:807:  while (MoreSQLData())
attachment.cgi:812:     $a{'datasize'}) = FetchSQLData();
attachment.cgi:822:  SendSQL("SELECT short_desc, assigned_to FROM bugs " .
attachment.cgi:824:  my ($bugsummary, $assignee_id) = FetchSQLData();
attachment.cgi:854:  SendSQL("SELECT attach_id, description, isprivate
attachment.cgi:860:  while ( MoreSQLData() ) {
attachment.cgi:862:    ($a{'id'}, $a{'description'}, $a{'isprivate'}) =
FetchSQLData();
attachment.cgi:869:  SendSQL("SELECT short_desc, assigned_to FROM bugs
attachment.cgi:871:  my ($bugsummary, $assignee_id) = FetchSQLData();
attachment.cgi:880:  SendSQL("SELECT product_id, component_id FROM bugs
attachment.cgi:882:  my ($product_id, $component_id) = FetchSQLData();
attachment.cgi:941:    my $sql_filename = SqlQuote($filename);
attachment.cgi:942:    my $description = SqlQuote($cgi->param('description'));
attachment.cgi:943:    my $contenttype = SqlQuote($cgi->param('contenttype'));
attachment.cgi:948:  my $sql_timestamp = SqlQuote($timestamp);
attachment.cgi:1008:      SendSQL("UPDATE attachments SET isobsolete = 1 " .
attachment.cgi:1010:      SendSQL("INSERT INTO bugs_activity (bug_id, attach_id,
who, bug_when,
attachment.cgi:1024:      SendSQL("SELECT " . join(", ", @fields) . " " .
attachment.cgi:1030:      my @oldvalues = FetchSQLData();
attachment.cgi:1036:      @oldvalues = map(SqlQuote($_), @oldvalues);
attachment.cgi:1037:      @newvalues = map(SqlQuote($_), @newvalues);
attachment.cgi:1040:      SendSQL("UPDATE bugs SET delta_ts = $sql_timestamp, " .
attachment.cgi:1052:              SendSQL("INSERT INTO bugs_activity " .
attachment.cgi:1090:  SendSQL("SELECT description, mimetype, filename, bug_id,
ispatch, isobsolete, isprivate, LENGTH(thedata)
attachment.cgi:1092:  my ($description, $contenttype, $filename, $bugid,
$ispatch, $isobsolete, $isprivate, $datasize) = FetchSQLData();
attachment.cgi:1098:  SendSQL("SELECT attach_id FROM attachments WHERE bug_id =
$bugid ORDER BY attach_id");
attachment.cgi:1100:  push(@bugattachments, FetchSQLData()) while (MoreSQLData());
attachment.cgi:1101:  SendSQL("SELECT short_desc FROM bugs WHERE bug_id = $bugid");
attachment.cgi:1102:  my ($bugsummary) = FetchSQLData();
attachment.cgi:1105:  SendSQL("SELECT product_id, component_id FROM bugs WHERE
bug_id = $bugid");
attachment.cgi:1106:  my ($product_id, $component_id) = FetchSQLData();
attachment.cgi:1192:  SendSQL("SELECT description, mimetype, filename, ispatch,
isobsolete, isprivate
attachment.cgi:1195:      $oldisobsolete, $oldisprivate) = FetchSQLData();
attachment.cgi:1198:  my $quoteddescription = SqlQuote($cgi->param('description'));
attachment.cgi:1199:  my $quotedcontenttype = SqlQuote($cgi->param('contenttype'));
attachment.cgi:1200:  my $quotedfilename = SqlQuote($cgi->param('filename'));
attachment.cgi:1203:  SendSQL("SELECT NOW()");
attachment.cgi:1204:  my $timestamp = FetchOneColumn();
attachment.cgi:1214:  SendSQL("UPDATE  attachments
attachment.cgi:1225:  my $sql_timestamp = SqlQuote($timestamp);
attachment.cgi:1227:    my $quotedolddescription = SqlQuote($olddescription);
attachment.cgi:1229:    SendSQL("INSERT INTO bugs_activity (bug_id, attach_id,
who, bug_when,
attachment.cgi:1235:    my $quotedoldcontenttype = SqlQuote($oldcontenttype);
attachment.cgi:1237:    SendSQL("INSERT INTO bugs_activity (bug_id, attach_id,
who, bug_when,
attachment.cgi:1243:    my $quotedoldfilename = SqlQuote($oldfilename);
attachment.cgi:1245:    SendSQL("INSERT INTO bugs_activity (bug_id, attach_id,
who, bug_when,
attachment.cgi:1252:    SendSQL("INSERT INTO bugs_activity (bug_id, attach_id,
who, bug_when,
attachment.cgi:1259:    SendSQL("INSERT INTO bugs_activity (bug_id, attach_id,
who, bug_when,
attachment.cgi:1266:    SendSQL("INSERT INTO bugs_activity (bug_id, attach_id,
who, bug_when,
Assignee: general → LpSolit
Target Milestone: --- → Bugzilla 2.22
Target Milestone: Bugzilla 2.22 → Bugzilla 2.24
Assignee: LpSolit → ghendricks
Attached patch Patch v1 (obsolete) — — Splinter Review 

        Attachment #213757 -
        Flags: review?(mkanat)

    
    

    
  
Comment on attachment 213757 [details] [diff] [review]
Patch v1

>@@ -734,10 +745,12 @@
>   my $context = validateContext();
> 
>   # Get patch data
>-  SendSQL("SELECT bug_id, description, ispatch, thedata FROM attachments " .
>+  my $sth = $dbh->prepare(
>+          "SELECT bug_id, description, ispatch, thedata FROM attachments " .
>           "INNER JOIN attach_data ON id = attach_id " .
>-          "WHERE attach_id = $attach_id");
>-  my ($bugid, $description, $ispatch, $thedata) = FetchSQLData();
>+          "WHERE attach_id = ?");
>+  $sth->execute($attach_id);
>+  my ($bugid, $description, $ispatch, $thedata) = $sth->fetchrow_array;

  Nit: Why don't you just do this with selectrow_array like you do everywhere else?

>+      $sth = $dbh->prepare("SELECT attach_id, description 
>+                            FROM attachments 
>+                            WHERE bug_id = ? 
>+                            AND ispatch = 1 
>+                            ORDER BY creation_ts DESC");

  Nit: And same here.

>@@ -803,22 +821,23 @@
>+    my $sth = $dbh->prepare_cached("SELECT attach_id, " .

  Don't use prepare_cached, it's not mod_perl safe for us.

>             $dbh->sql_date_format('creation_ts', '%Y.%m.%d %H:%i') . ",
>             mimetype, description, ispatch, isobsolete, isprivate, 
>             LENGTH(thedata)
>             FROM attachments 
>             INNER JOIN attach_data
>             ON attach_id = id
>-            WHERE bug_id = $bugid $privacy 
>+            WHERE bug_id = ? $privacy 
>             ORDER BY attach_id");
>+  $sth->execute($bugid);
>   my @attachments; # the attachments array
>-  while (MoreSQLData())
>+  while (my @row = $sth->fetchrow_array)
>   {
>     my %a; # the attachment hash
>     ($a{'attachid'}, $a{'date'}, $a{'contenttype'},
>      $a{'description'}, $a{'ispatch'}, $a{'isobsolete'}, $a{'isprivate'},
>-     $a{'datasize'}) = FetchSQLData();
>+     $a{'datasize'}) = @row;

  You can just create all of this with fetchrow_hashref, if you use "AS" to name the columns properly.

>@@ -828,9 +847,9 @@
>+  my ($bugsummary, $assignee_id) = $dbh->selectrow_array(
>+          "SELECT short_desc, assigned_to FROM bugs " .
>+          "WHERE bug_id = ?", undef, $bugid);

  This is good, by the way -- much better than creating a temporary @row variable.

>@@ -859,24 +878,26 @@
>+  my $sth = $dbh->prepare_cached(

  prepare_cached, again.

>+  while ( my @row = $sth->fetchrow_array ) {
>     my %a; # the attachment hash
>-    ($a{'id'}, $a{'description'}, $a{'isprivate'}) = FetchSQLData();
>+    ($a{'id'}, $a{'description'}, $a{'isprivate'}) = @row;

  And another time that you could use fetchrow_hashref.

>@@ -904,7 +926,6 @@
> # Insert a new attachment into the database.
> sub insert
> {
>-    my $dbh = Bugzilla->dbh;

  Why'd you do this? subs *should* have their own $dbh.

>@@ -963,22 +986,21 @@
>+  my ($sql_timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()"); 
>+  trick_taint($sql_timestamp);

  You don't need to trick_taint that, it came out of the database, so it's not tainted.

>+      VALUES (?,?,?,?,?,?,?,?,?)");
>+  $sth->execute($bugid, $sql_timestamp, $filename,
>+              $description, $contenttype, $cgi->param('ispatch'),
>+              $isurl, $isprivate, $userid);

  Does that work? $cgi->param('ispatch') is almost definitely tainted. It looks like it worked before, but... oh, I suppose we called detaint_natural on it earlier, yes?

>-      @oldvalues = map(SqlQuote($_), @oldvalues);
>-      @newvalues = map(SqlQuote($_), @newvalues);
>+      @oldvalues = map($dbh->quote($_), @oldvalues);
>+      @newvalues = map($dbh->quote($_), @newvalues);

  SqlQuote also detaints things, but $dbh->quote doesn't.

>       # Update the bug record. Note that this doesn't involve login_name.
>-      SendSQL("UPDATE bugs SET delta_ts = $sql_timestamp, " . 
>+      $dbh->do("UPDATE bugs SET delta_ts = $sql_timestamp, " . 

  Was $sql_timestamp already sql-quoted? It shouldn't be; we should just pass it in through a placeholder.

>+  $dbh->do("UPDATE  attachments 
>+            SET     description = ?,
>+                    mimetype    = ?,
>+                    filename    = ?,
>+                    ispatch     = ?,
>+                    isobsolete  = ?,
>+                    isprivate   = ?
>+            WHERE   attach_id   = ?",
>+            undef, $description, $contenttype, $filename,
>+            $cgi->param('ispatch'), $cgi->param('isobsolete'), 
>+            $cgi->param('isprivate'), $attach_id);

  Are all those $cgi->param things untainted??

>-             VALUES ($bugid, $attach_id, $userid, $sql_timestamp, $fieldid,
>-                     $oldispatch, " . $cgi->param('ispatch') . ")");
>+              VALUES (?,?,?,?,?,?,?)",
>+              undef, $bugid, $attach_id, $userid, $sql_timestamp, $fieldid,
>+                     $oldispatch, $cgi->param('ispatch'));

  And here, too?

        Attachment #213757 -
        Flags: review?(mkanat) → review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v2 (obsolete)
        — 
          — Splinter Review
      

    


  
(In reply to comment #2)

> >             $dbh->sql_date_format('creation_ts', '%Y.%m.%d %H:%i') . ",
> >             mimetype, description, ispatch, isobsolete, isprivate, 
> >             LENGTH(thedata)
> >             FROM attachments 
> >             INNER JOIN attach_data
> >             ON attach_id = id
> >-            WHERE bug_id = $bugid $privacy 
> >+            WHERE bug_id = ? $privacy 
> >             ORDER BY attach_id");
> >+  $sth->execute($bugid);
> >   my @attachments; # the attachments array
> >-  while (MoreSQLData())
> >+  while (my @row = $sth->fetchrow_array)
> >   {
> >     my %a; # the attachment hash
> >     ($a{'attachid'}, $a{'date'}, $a{'contenttype'},
> >      $a{'description'}, $a{'ispatch'}, $a{'isobsolete'}, $a{'isprivate'},
> >-     $a{'datasize'}) = FetchSQLData();
> >+     $a{'datasize'}) = @row;
> 
>   You can just create all of this with fetchrow_hashref, if you use "AS" to
> name the columns properly.

Oh but that is so much more work and I am LAZY! But for you? sure ;-)

> >@@ -963,22 +986,21 @@
> >+  my ($sql_timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()"); 
> >+  trick_taint($sql_timestamp);
> 
>   You don't need to trick_taint that, it came out of the database, so it's not
> tainted.
> 
> >+      VALUES (?,?,?,?,?,?,?,?,?)");
> >+  $sth->execute($bugid, $sql_timestamp, $filename,
> >+              $description, $contenttype, $cgi->param('ispatch'),
> >+              $isurl, $isprivate, $userid);
> 
>   Does that work? $cgi->param('ispatch') is almost definitely tainted. It looks
> like it worked before, but... oh, I suppose we called detaint_natural on it
> earlier, yes?

That is all handled in validateIsPatch

> 
> >-      @oldvalues = map(SqlQuote($_), @oldvalues);
> >-      @newvalues = map(SqlQuote($_), @newvalues);
> >+      @oldvalues = map($dbh->quote($_), @oldvalues);
> >+      @newvalues = map($dbh->quote($_), @newvalues);
> 
>   SqlQuote also detaints things, but $dbh->quote doesn't.

Yes, I am aware, that is why I have added a bunch of trick_taints in this patch ;-)
However, in this case @oldvalues is all out of the DB (and as per your comment above we don't need to worry about those) and @newvalues are
@newvalues = ($userid, "ASSIGNED", "", 1, Bugzilla->user->login) of which only $userid would be questionable but as we see further up it is Bugzilla->user->id.
So I don't think we need to worry about them.

> >+  $dbh->do("UPDATE  attachments 
> >+            SET     description = ?,
> >+                    mimetype    = ?,
> >+                    filename    = ?,
> >+                    ispatch     = ?,
> >+                    isobsolete  = ?,
> >+                    isprivate   = ?
> >+            WHERE   attach_id   = ?",
> >+            undef, $description, $contenttype, $filename,
> >+            $cgi->param('ispatch'), $cgi->param('isobsolete'), 
> >+            $cgi->param('isprivate'), $attach_id);
> 
>   Are all those $cgi->param things untainted??
> 
> >-             VALUES ($bugid, $attach_id, $userid, $sql_timestamp, $fieldid,
> >-                     $oldispatch, " . $cgi->param('ispatch') . ")");
> >+              VALUES (?,?,?,?,?,?,?)",
> >+              undef, $bugid, $attach_id, $userid, $sql_timestamp, $fieldid,
> >+                     $oldispatch, $cgi->param('ispatch'));
> 
>   And here, too?
> 

Yes, in thier appropriate subroutines.

        Attachment #213757 -
        Attachment is obsolete: true

        Attachment #213820 -
        Flags: review?(mkanat)

    
    

    
  
Comment on attachment 213820 [details] [diff] [review]
Patch v2

All routines which use $dbh should define $dbh in the routines themselves. Else if we move these routines elsewhere, we will probably break them because $dbh will be undefined.



>@@ -151,12 +151,15 @@
>+    my @row = $dbh->selectrow_array("SELECT bug_id, isprivate 
>+                                     FROM attachments 
>+                                     WHERE attach_id = ?",
>+                                     undef, $attach_id);
>+    ThrowUserError("invalid_attach_id", { attach_id => $attach_id }) 
>+        unless @row;
> 
>     # Make sure the user is authorized to access this attachment's bug.
>+    (my $bugid, my $isprivate) = @row;

Much better is to write:

my (bugid, $isprivate) = $dbh->selectrow_array(...); and get rid of @row.


>@@ -204,23 +207,27 @@
>+    my $ref = $dbh->selectrow_arrayref("SELECT attach_id FROM attachments 
>+                                        WHERE attach_id = ? 
>+                                        AND submitter_id = ?",
>+                                        undef, $attach_id, Bugzilla->user->id);

We have never used selectrow_arrayref(), which doesn't make sense here. We want the attachment ID, so use selectrow_array(). Moreover, as soon as you have more than one parameters, use parens to group these parameters => "undef, ($attach_id, Bugzilla->user->id)".


>+   ThrowUserError("illegal_attachment_edit",
>+                        { attach_id => $attach_id })
>+                        unless $ref;

This is arbitrary indentation... Write:

$ref || ThrowUserError("illegal_attachment_edit", { attach_id => $attach_id });

We have enough place to write it on one line.


>@@ -401,14 +409,13 @@
>+    my @row = $dbh->selectrow_array("SELECT bug_id, isobsolete, description 
>+             FROM attachments WHERE attach_id = ?", undef, $attachid);
> 
>     # Make sure the attachment exists in the database.
>+    ThrowUserError("invalid_attach_id", $vars) unless @row;
> 
>+    my ($bugid, $isobsolete, $description) = @row;

Here again, get rid of @row, as above.


>@@ -475,10 +482,12 @@
>+    my $sth = $dbh->prepare(
>+            "SELECT mimetype, filename, thedata FROM attachments " .
>             "INNER JOIN attach_data ON id = attach_id " .
>+            "WHERE attach_id = ?");
>+    $sth->execute($attach_id);
>+    my ($contenttype, $filename, $thedata) = $sth->fetchrow_array;

Use selectrow_array() instead of prepare(), execute(), fetchrow_array().


>@@ -600,12 +609,14 @@
>+  my $sth = $dbh->prepare(
>+          "SELECT bug_id, description, ispatch, thedata " . 
>           "FROM attachments " .
>           "INNER JOIN attach_data " .
>           "ON id = attach_id " .
>+          "WHERE attach_id = ?");
>+  $sth->execute($id);
>+  my ($bugid, $description, $ispatch, $thedata) = $sth->fetchrow_array;

Use selectrow_array() here too.


>@@ -764,9 +775,14 @@
>+      my $sth = $dbh->prepare("SELECT attach_id, description 
>+                            FROM attachments 
>+                            WHERE bug_id = ? 
>+                            AND ispatch = 1 
>+                            ORDER BY creation_ts DESC");

Please align the SQL query vertically. This applies at some other places too.


>+      $sth->execute($bugid);
>       my $select_next_patch = 0;
>+      while (my ($other_id, $other_desc) = $sth->fetchrow_array) {

Moreover, use selectall_arrayref() and then do a loop over the array you get.


>@@ -803,34 +819,32 @@
>+    my $sth = $dbh->prepare("SELECT attach_id AS attachid, " .

>+  $sth->execute($bugid);
>   my @attachments; # the attachments array
>+  while (my $a = $sth->fetchrow_hashref)

You should use selectall_arrayref() combined with {'Slice' => {}} instead.


>@@ -859,24 +873,24 @@
>+  my $sth = $dbh->prepare(
>+          "SELECT attach_id AS id, description, isprivate

>+  $sth->execute($bugid);
>   my @attachments; # the attachments array
>+  while ( my $a = $sth->fetchrow_hashref ) {
> 
>     # Add the hash representing the attachment to the array of attachments.
>+    push @attachments, $a;
>   }

Same comment here. This way, you don't need to push() anything. The array is already built.


>@@ -925,16 +939,18 @@
>         $filename = validateFilename();
>+        trick_taint($filename);

>+        $contenttype = $cgi->param('contenttype');
>+        trick_taint($contenttype);

In both cases, justify why you detaint these values.


>@@ -963,22 +979,21 @@
>+    my $description = $cgi->param('description');
>+    trick_taint($description);

Same comment here.


>+  my ($sql_timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()"); 

There is no reason to call it $sql_something, which were used when using SqlQuote(). It's a timestamp, call it $timestamp.


>   my $sth = $dbh->prepare("INSERT INTO attachments
>       (bug_id, creation_ts, filename, description,
>        mimetype, ispatch, isurl, isprivate, submitter_id) 
>+      VALUES (?,?,?,?,?,?,?,?,?)");
>+  $sth->execute($bugid, $sql_timestamp, $filename,
>+              $description, $contenttype, $cgi->param('ispatch'),
>+              $isurl, $isprivate, $userid);

First of all, use $dbh->do(). Secondly, add a whitespace after commas, as we usually do. Third point: group parameters in parens (we are supposed to pass an array). These comments apply in several other places too.


>@@ -1050,23 +1065,23 @@
>+      my @oldvalues = $dbh->selectrow_array(
>+              "SELECT " . join(", ", @fields) . " " .
>               "FROM bugs " .
>               "INNER JOIN profiles " .
>               "ON profiles.userid = bugs.assigned_to " .
>+              "WHERE bugs.bug_id = ?", undef, $bugid);

I definitely don't understand the indentation you use with SQL queries. 8 whitespaces here.


>+      $dbh->do("UPDATE bugs SET delta_ts = $sql_timestamp, " . 
>               join(", ", map("$fields[$_] = $newvalues[$_]", (0..3))) . 
>               " WHERE bug_id = $bugid");

Nit: You could as well use placeholders here too.


>@@ -1201,83 +1217,93 @@
>+  my @row = $dbh->selectrow_array(
>+      "SELECT description, mimetype, filename, ispatch, isobsolete, isprivate
>+       FROM attachments WHERE attach_id = ?", undef, $attach_id);
>   my ($olddescription, $oldcontenttype, $oldfilename, $oldispatch,
>+      $oldisobsolete, $oldisprivate) = @row;

Get rid of @row.


>+  my $description = $cgi->param('description');
>+  trick_taint($description);
>+  my $contenttype = $cgi->param('contenttype');
>+  trick_taint($contenttype);
>+  my $filename = $cgi->param('filename');
>+  trick_taint($filename);

group all these trick_taint() below a comment explaining why you are detaining these values.


>+  my ($sql_timestamp) = $dbh->selectrow_array("SELECT NOW()");

Same comment as before, don't write $sql_foo.


>+  $dbh->do("UPDATE  attachments 
>+            SET     description = ?,
>+                    mimetype    = ?,
>+                    filename    = ?,
>+                    ispatch     = ?,
>+                    isobsolete  = ?,
>+                    isprivate   = ?
>+            WHERE   attach_id   = ?",
>+            undef, $description, $contenttype, $filename,
>+            $cgi->param('ispatch'), $cgi->param('isobsolete'), 
>+            $cgi->param('isprivate'), $attach_id);

Parens.


>+    $dbh->do("INSERT INTO bugs_activity (bug_id, attach_id, who, bug_when,
>                                         fieldid, removed, added)
>+              VALUES (?,?,?,?,?,?,?)",
>+              undef, $bugid, $attach_id, $userid, $sql_timestamp, $fieldid,
>+                     $olddescription, $description);

As above, whitespaces after commas, and parens. This comment applies in other places too.

        Attachment #213820 -
        Flags: review?(mkanat) → review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v3 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #213820 -
        Attachment is obsolete: true

        Attachment #216464 -
        Flags: review?(mkanat)

    
    

    
  
Comment on attachment 216464 [details] [diff] [review]
v3

> sub validateID
> {
>+    my $dbh = Bugzilla->dbh;
>     my $param = @_ ? $_[0] : 'id';

Nit: you should first get parameters passed to the routine @_ and then defined $dbh. That's the way we do it in the Bugzilla code.


> sub validateCanEdit
> {
>+    my $dbh = Bugzilla->dbh;
>     my ($attach_id) = (@_);

Same comment here and in other routines too.


>+      my $sth = $dbh->prepare("SELECT attach_id, description 
>+                               FROM attachments 
>+                               WHERE bug_id = ? 
>+                               AND ispatch = 1 
>+                               ORDER BY creation_ts DESC");
>+      $sth->execute($bugid);
>       my $select_next_patch = 0;
>+      while (my ($other_id, $other_desc) = $sth->fetchrow_array) {

Nit: you could use selectall_arrayref() and do a loop over the list of attachments being returned.


>+  foreach my $a (@{$attachments})

Nit: you can write @$attachments.


>     # Add the hash representing the attachment to the array of attachments.
>+    push @attachments, $a;

No need to push $a. $a has already been updated.


>+      Bugzilla::Flag::CancelRequests($bugid, $obsolete_id, $timestamp);

Flag::update_activity(), called by Flag::CancelRequests(), requires a quoted timestamp (this will be fixed in another bug). So $dbh->quote($timestamp) is required here till Flag.pm is fixed.


>+      $dbh->do("UPDATE bugs SET delta_ts = $timestamp, " . 
>               join(", ", map("$fields[$_] = $newvalues[$_]", (0..3))) . 
>+              " WHERE bug_id = ?", undef, $bugid);

Use a placeholder for the timestamp.


>       for (my $i = 0; $i < 4; $i++) {
>           if ($oldvalues[$i] ne $newvalues[$i]) {
>               my $fieldid = get_field_id($fields[$i]);
>+              $dbh->do("INSERT INTO bugs_activity " .
>+                       "(bug_id, who, bug_when, fieldid, removed, added) " .
>+                       "VALUES (?,?,?,?,?,?)", 
>+                       undef, $bugid, $userid, $timestamp, 
>+                       $fieldid, $oldvalues[$i], $newvalues[$i]);
>           }
>       }

Prepare the SQL query outside the FOR loop. Moreover, enclose the list of values in parens.


>+  # we can trick taint until the cows come home thanks to placeholders

Hum... s/trick taint/detaint/ and s/cows come home//.


>+    # trick_taint is fun, even if your parents say otherwise.
>+    # place holders are your friends
>+    trick_taint($olddescription);

Is it a test to make sure the reviewer reads the patch carefully? ;) Anyway, as $olddescription comes from the DB, it doesn't need to be detainted.


>+    trick_taint($oldcontenttype);
>+    trick_taint($oldfilename);

Same comment here. All values coming from the DB do not need to be detainted.

        Attachment #216464 -
        Flags: review?(mkanat) → review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        V4
        — 
          — Splinter Review
      

    


  
(In reply to comment #6)
> >+      my $sth = $dbh->prepare("SELECT attach_id, description 
> >+                               FROM attachments 
> >+                               WHERE bug_id = ? 
> >+                               AND ispatch = 1 
> >+                               ORDER BY creation_ts DESC");
> >+      $sth->execute($bugid);
> >       my $select_next_patch = 0;
> >+      while (my ($other_id, $other_desc) = $sth->fetchrow_array) {
> 
> Nit: you could use selectall_arrayref() and do a loop over the list of
> attachments being returned.

This feels too invasive to me. Unless this is more efficient I would prefer to keep it simple. This way was the the least invasive.

 
> 
> >+  foreach my $a (@{$attachments})
> 
> Nit: you can write @$attachments.
> 
I know, but I like how the braces make it stand out. It helps me see it easier.

        Attachment #216464 -
        Attachment is obsolete: true

        Attachment #218188 -
        Flags: review?(LpSolit)

    
    

    
  
Comment on attachment 218188 [details] [diff] [review]
V4

>+  my $sth = $dbh->do(

Nit: $dbh->do() won't return anything useful. $sth is optional.


The patch works correctly. r=LpSolit

        Attachment #218188 -
        Flags: review?(LpSolit) → review+

    
  
Status: NEW → ASSIGNED
Flags: approval?
Flags: approval? → approval+

    
    

    
  
Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.106; previous revision: 1.105
done

Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED

    
  
Blocks: 340139

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: