Closed Bug 238870 Opened 16 years ago Closed 15 years ago

remove %FORM from attachment.cgi

Categories

(Bugzilla :: Bugzilla-General, defect)

2.17.7
defect
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: justdave, Assigned: wicked)

References

Details

Attachments

(2 files, 5 obsolete files)

This one is going to be downright nasty.

I'm not going to bother to put a grep list in this one.  Suffice it to say
there's a LOT. :)
Blocks: 236
No longer blocks: 225818
Blocks: 236678
No longer blocks: 236
"Do I see a volunteer emerging from the crowd?"
:)
Assignee: nobody → bugzilla
What do people want to happen about the following lines in attachment.cgi? (around line 160)

>    Bugzilla::Flag::validate(\%::FORM, $bugid);
>    Bugzilla::FlagType::validate(\%::FORM, $bugid, $cgi->param('id'));

Should the interface to the validate() routines change (to take a $cgi object maybe?), or is that to remain 
the same, and a fake hash created for the calls?

This also occurs in process_bug.cgi

Status: NEW → ASSIGNED
erf...  forgot about that part...   the FlagType stuff needs to be rewritten to
deal with $cgi->param instead of a form hash I think.  Myk?  Opinion?

We never had to mess with this part for Zippy because they weren't using flags.
My "off the top of my head" suggestion is to pass $cgi itself instead of an
equivalent to \%::FORM, and teach Flagtype.pm how to look at the $cgi object to
get what it needs.
*** Bug 240241 has been marked as a duplicate of this bug. ***
Attached patch does this work (attachment.cgi) (obsolete) — Splinter Review
Attachment #145874 - Flags: review?(justdave)
Comment on attachment 145874 [details] [diff] [review]
does this work (attachment.cgi)

Looks good. I don't like the lines that are over 80 chars due to your patch
(they were under 80 chars before applying it).

Also, this patch so doesn't solve the current bug. The purpose here is things
like $::FORM and $::COOKIE, and your patch deals with Bugzilla_login and stuff.

Otherwise looks good assuming you tested it and stuff. r=vlad.
Attachment #145874 - Flags: review+
Comment on attachment 145874 [details] [diff] [review]
does this work (attachment.cgi)

The comment at the top doesn't make any sense...  We're not setting
Bugzilla->user->id, it's already set.  Probably better to just remove this
comment instead of editing it.

There's several places here where you're doing DBID_to_name($userid), which
causes unnecessary database activity.  The user's name is already in
Bugzilla->user->login, just use that.

Also, this patch doesn't address the entire scope of this bug...  if you want
to fix the issues I raised and reupload it, go for it (I gather you objected to
fixing the entire bug), but whoever ends up fixing the rest of this should just
incorporate this into their patch (feel free to credit timeless).
Attachment #145874 - Flags: review?(justdave) → review-
This patch does not include the changes in attachment#145874 [details] [diff] [review] -- I'll add that
in a later patch.

This patch removes the $::FORM and COOKIE code from attachment.cgi. It adds
some paramaters and return values to some of the routines, so that values are
no longer updated in the $cgi object. I have tested this a lot, but the whole
patch viewing and diffing, and flags side of things is a bit new to me, so
there may be untested bits in there.

This changes the interface to the flags code, so a patch for process_bug.cgi
will be needed alongside this one. I hope to attach that within the next day or
2
Attachment #147354 - Flags: review?
Comment on attachment 147354 [details] [diff] [review]
remove $::FORM and COOKIE from attachment.cgi

Please attach the process_bug et al. changes into the same patch. Testing this
without them is going to be practically impossible anyway.

Marking r- because of the above and bitrot.
Attachment #147354 - Flags: review? → review-
Here is the combined patch. I've done a lot of testing, but it changes quite a
lot of code, so it could do with a lot more, as it is such an important part of
the system -- especially in the area of patch viewing/diffing, as I don't have
all the patch viewer/interdiff support installed
Attachment #147354 - Attachment is obsolete: true
Attachment #149538 - Flags: review?
Comment on attachment 149538 [details] [diff] [review]
Combined %::FORM removal from process_bug, attachment, hidden-fields, and flags (-w)

This patch is massive (and bitrotten, but that's not a surprise). I don't think
we're going to be able to review this 100+ k of mostly mechanical changes and
then spot the errors within. Thanks for all the work, though - it's a great
step ahead to this material on the table now.

It seems we have two major alternatives. Either we check this monstrosity in
very soon after 2.18 branches so that trunk can get enough testing even if
(when) something will slip the review OR split this into quite a few smaller
bugs and get them with proper reviews. I'd go for heavy splitting, and I'm
talking about really small parts like "make flag/flagtype::validate use $cgi".
If the patch size exceeds 20k, it becomes really hard to keep from bitrotting
and reviewing becomes pretty unreliable.

GavinS, what do you think?



Also, please clearly indicate if a patch you've added is a -w version. This one
is; it can't be checked in. Although it's a good idea to give us a -w version,
please attach the normal one as well.

The following comments are the result of a cursory runthrough of the patch. I
didn't have the stomach to go through all of the logic without being able to
test.

>Index: attachment.cgi
>===================================================================

>+# Returns, a list, the first item is the validated, detainted
>+# attachment id, the 2nd item is the bugid corresponding to the
>+# attachment, or will fail appropriately

Start with something like "Returns a list, where the first item...", "... and
the 2nd item ...". Use a full stop before "or will fail"; describe when the
failure occurs (something really simple like 'when the attachment id is not
valid). How about describing the optional parameter as well?

s/bugid/bug id/

>     # Validate the value of the "id" form field, which must contain an
>     # integer that is the ID of an existing attachment.
>+    my $attach_id = $cgi->param($param);

Fix this comment; since the param name can now be taken as a parameter (sigh),
it's no longer always id?

>+    # If we were to update the CGI env value to the detainted one, in
>+    # case it is used later on, we would do it here, but, I don't
>+    # think it is used anymore
>+#    $cgi->param($param,$attach_id);
>+    

I simply cannot grok what's going on here; let's remove this part of code and
see if something breaks. If it does, let's talk about the fix later on when we
understand what we're doing.

>+# Returns the validated (or default-value-ed) format
> sub validateFormat

It wouldn't hurt if you described the operation a bit more clearly here. What
does this sub really do?

>+# Returns validated, detainted context, or fails appropriately
> sub validateContext

Again; what is "context"? (I know it's not your terminology really, but it's
easier to open this up now). What is "appropriately"?

>+        
>+        # If we were to add back the detainted value into the CGI env,
>+        # we would do it here, but it should not be needed any more
>+#        $cgi->param('context',$context);

As commented earlier with a similar case.

>-    SendSQL("SELECT mimetype, filename, thedata FROM attachments WHERE attach_id = $::FORM{'id'}");
>+    SendSQL("SELECT mimetype, filename, thedata FROM attachments WHERE attach_id = 
>+            $attach_id");

Nit: I'd rather see a wrap occur before the WHERE.

>     $flag_type->{'flags'} = Bugzilla::Flag::match({ 'type_id'   => $flag_type->{'id'}, 
>-                                                    'attach_id' => $::FORM{'id'} });
>+                                                        'attach_id' => $attach_id });

All these flag matches will have bitrotten because of my patch to bug 223878;
be sure to have the is_active => 1 condition in all of these after you fix the
bitrots :)


>Index: process_bug.cgi
>===================================================================

>+# 3) If we are processing just the one id, then it is stored in @idlist for later processing

Make sure no lines exceed 80 chars in length.

>Index: Bugzilla/Flag.pm
>===================================================================

>-        # and it won't break if $::MFORM does not define them.
>+        # and it won't break if the CGI object does not know about them

Nit: full stop missing...
Attachment #149538 - Attachment description: Combined %::FORM removal from process_bug, attachment, hidden-fields, and flags → Combined %::FORM removal from process_bug, attachment, hidden-fields, and flags (-w)
Attachment #149538 - Flags: review? → review-
Target Milestone: --- → Bugzilla 2.20
This patch should probably also block the process_bug bug 238876, if we're going
to do them together. I can clean up COOKIE in a jiffy in a separate bug, too.
Should I?
Doing COOKIE separately in bug 252378.
No longer blocks: 236678
Summary: remove %FORM and %COOKIE from attachment.cgi → remove %FORM from attachment.cgi
Blocks: 225818
No longer blocks: 204042
I've got a patch that does some restructuring of attachment.cgi that does some
partial cleanup of FORM -- leaves in shims, but in general it's nice (uses Bug
where possible, etc). It's not too high-risk because it's not a severe whack
(possible because attachment is really a number of tasks together).

Gavin, would you like to review my bits and then maybe work from there, or what?
(In reply to comment #16)
> I've got a patch that does some restructuring of attachment.cgi that does some
> partial cleanup of FORM -- leaves in shims, but in general it's nice (uses Bug
> where possible, etc). It's not too high-risk because it's not a severe whack
> (possible because attachment is really a number of tasks together).
> 
> Gavin, would you like to review my bits and then maybe work from there, or what?

Yes, sure. I hope to re-visit this after a bit more admin templatisation, and
finally answer Jounis review comments and question about splitting this patch up
into more manageable chunks -- which I'm not sure is going to be too easy... 
Depends on: 254379
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Hey Gavin. Did you want to do just the attachment.cgi piece of this, for 2.20?
We can probably handle the other ones in the other bugs, I suspect.

I think the admin templatization is pretty well along, now. :-)

Otherwise, did one of you MINB folk want to take this one? :-)
(In reply to comment #18)
> Hey Gavin. Did you want to do just the attachment.cgi piece of this, for 2.20?
> We can probably handle the other ones in the other bugs, I suspect.

How do we propose to do all the remaining $::FORM removals then?

I seem to remember that because of the hidden-fields template, anything which
uses it, or the user matching code, all sort of seem to have to be changed at
the same time, unless you write a wrapper from $cgi <-> $::FORM during the
transition period...
(In reply to comment #18)
> Hey Gavin. Did you want to do just the attachment.cgi piece of this, for 2.20?
> We can probably handle the other ones in the other bugs, I suspect.

I will try and update the attachment.cgi bit sometime this week...
Blocks: 254379
No longer depends on: 254379
I unbitrotted the combined patch from attachment 149538 [details] [diff] [review], addressed jouni's
review notes from comment #13 and integrated timeless's userid changes from
attchment 145874. Here is the result of that work in a combined patch. Code
passes runtests.sh but was not otherwise extensively tested.

I'll try to chop this monster into reviewable pieces and attach them to
appropriate bugs. Or is there an eager reviewer willing to review this monster?
:)
Assignee: bugzilla → wicked
Attachment #145874 - Attachment is obsolete: true
Attachment #149538 - Attachment is obsolete: true
Depends on: 238878
This patch removes FORM hash only from attachment.cgi and is based on the
combined patch by GavinS. I did test it briefly and basic attachment functions
seem to work. Dependent bugs are needed to fully remove FORMs from
attachment.cgi.
Attachment #178866 - Flags: review?(myk)
Comment on attachment 178866 [details] [diff] [review]
Remove FORMs from attachment.cgi, V1

> elsif ($action eq "interdiff")
> {
>-  validateID('oldid');
>-  validateID('newid');
>-  validateFormat("html", "raw");
>-  validateContext();
>-  interdiff();
>+    my ($old_id, $old_bugid) = validateID('oldid');
>+    my ($new_id, $new_bugid) = validateID('newid');
>+    interdiff($old_id,$new_id,validateFormat('html', 'raw'), validateContext());

I haven't done a full review yet, but an important note is that besides making
the function calls unnecessarily complicated, changing the validation routines
like this so they return the validated values and are called directly from the
function calls' parameter lists keeps validation in the top-level switch.  It
should move into the functions, however, per the comment at the top of the
script, since the functions do everything else, and moving validation there too
makes the code simpler and easier to grok.
Attachment #178866 - Flags: review?(myk) → review-
Yes, my Reviewer. I moved validation routine calls to inside the function subs.
This patch is now diffed against the patch in dependent bug #238878 so it's
needed to apply this patch cleanly.
Attachment #178866 - Attachment is obsolete: true
Attachment #179503 - Flags: review?(myk)
Comment on attachment 179503 [details] [diff] [review]
Remove FORMs from attachment.cgi, V2

Attachments are working fine, except for requestees. But this problem is not
new, see bug 289002.

myk, have you completed your review?
Attachment #179503 - Flags: review+
Already requesting approval, assuming myk will review this patch on time. :)
Flags: approval?
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
Flags: approval? → approval+
Comment on attachment 179503 [details] [diff] [review]
Remove FORMs from attachment.cgi, V2

>-                      " VALUES ($::FORM{'bugid'}, $::userid, " . 
>-                      "$sql_timestamp " . 
>+                      "VALUES ($bugid, $userid, " . $sql_timestamp .
>                       ", $fieldid, $oldvalues[$i], $newvalues[$i])");

Nit: interpolation of $sql_timestamp, which is what the original code did,
would be cleaner and simpler, i.e.:

>+                      "VALUES ($bugid, $userid, $sql_timestamp " .


>+  # Get the user's login name since the AppendComment and header functions
>+  # need it.
>+  my $who = Bugzilla->user->login;

Nit: this complicates the code more than it simplifies it given that $who is
only used twice in the subsequent code.

Otherwise, great fixes. r=myk
Attachment #179503 - Flags: review?(myk) → review+
Attachment #179503 - Attachment is obsolete: true
Attachment #179993 - Flags: review+
Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.82; previous revision: 1.81
done
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Blocks: 290229
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.