Closed Bug 238878 Opened 16 years ago Closed 15 years ago

Make hidden-fields template, User Matching and Flags use direct CGI instead of [% form.foo %]

Categories

(Bugzilla :: Bugzilla-General, defect, P2)

2.17.7
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: justdave, Assigned: wicked)

References

Details

Attachments

(1 file, 2 obsolete files)

This is not going to be as easy as it sounds...

The hidden-fields template itself should be really easy to fix.  However, the
User-Matching code heavily manipulates the form hash after it's put into the
template vars hash, so the User-Matching stuff in Bugzilla/User.pm is going to
need a major overhaul.

Both the user matching and the hidden-fields template will need to be done as
part of the same patch, because they are too inter-related to do separately. 
There are two other bugs I'm marking dependent on this one -- those don't need
to be on the same patch, they won't break when this is checked in.  Those
basically amount to cleanup.  But they will break if you check those in before
this one.

I've already done this once, but I did it for hire and I haven't gotten an IPA
release yet.  If I get that IPA release in the near future I plan to take this.
 But there's no guarantee I'll get it.
Blocks: 210663
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
I'll also attach patches for bug#238870 and bug#238876 in a bit. The patch for
process_bug.cgi (bug#238876) will use this patch, and the patch for
attachment.cgi (bug#238870) will share the Flag and FlagType changes with the
patch for process_bug.cgi, so I'll also attach a patch with all these changes
together for actual testing tonight or tomorrow.
Attachment #147178 - Flags: review?
Comment on attachment 147178 [details] [diff] [review]
use $cgi in user matching and hidden fields code

Looks good on a first sight. You might want to tweak the comment where
"Anyone?" and the first person appear, but that's a nit I guess :).

However you're not consistent regarding the use of "CGI". It comes from Common
Gateway Interface, so it's an abbreviation and it must be in the upper case
when specified in the comments.
Attachment #147178 - Flags: review? → review-
Comment on attachment 147178 [details] [diff] [review]
use $cgi in user matching and hidden fields code

>Index: template/en/default/global/hidden-fields.html.tmpl

>+     FOREACH. I would like to count the number of items, instead of this
>+     check for an empty parameter but can't get
>+     Template Toolkit to evaluate $cgi->param(field) in a list context,
>+     so I can count the items. Anyone?
>+  %]
>+  [% IF cgi.param(field).match('^\s*$') %]
>+    <input type="hidden" name="[% field FILTER html %]"
>+           value="[% cgi.param(field) FILTER html FILTER html_linebreak %]">
>+  [% ELSE %]
>+    [% FOREACH mvalue = cgi.param(field) %]
>+      <input type="hidden" name="[% field FILTER html %]"
>              value="[% mvalue FILTER html FILTER html_linebreak %]">
>     [% END %]
>-  [% ELSE %]
>-    <input type="hidden" name="[% field.key FILTER html %]"
>-           value="[% field.value FILTER html FILTER html_linebreak %]">
>   [% END %]

.splice(0) on the end of a list will coerce it to always be in list context.

do a FOREACH item = cgi.param(field).slice(0)

Then you don't need the IF/ELSE either.
I typoed one of those... you want .slice(0), not .splice(0).
Uses slice(0), and hopefully made the CGI comments more consist
Attachment #147178 - Attachment is obsolete: true
For the record, you got all the "gotchas" covered that we ran into at my old job
when we converted that.  I won't mark review+ because I haven't tested this. 
Looks good from visual inspection though.
Attachment #147265 - Flags: review?
For the latest patch (and subsequent activity), see bug#238870, which contains a combined patch for 
process_bug, attachment, hidden fields and flags
Comment on attachment 147265 [details] [diff] [review]
use $cgi in user matching and hidden fields code, take 2

marking patch obsolete per comment 7.  This is now included in the patch on the
parent bug.
Attachment #147265 - Attachment is obsolete: true
Attachment #147265 - Flags: review?
I'll take this and move it back from the parent bug so that I can handle bug 238877.
Assignee: bugzilla → mkanat
Status: ASSIGNED → NEW
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.20
(Repeating my comment from bug#238870, as I don't think Max was CC'd on that)

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...
Yeah, I usually end up writing transition code in situations like this.
The other option is that we do this bug, and then we do all the other ones, and
we hold checkin for all of them until they're all complete.
I'm sorry, I just don't think this is going to make 2.20. At the least, I don't
have time to do it. Gavin, if you think you can do it, feel free to take it...
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
The Bugzilla/Flag.pm and Bugzilla/FlagType.pm will also be changed to use CGI
object instead of in this bug. See bug 238870 comment 5. This needs to happen
same time as User Matching is changed because Flags assume the user fields have
already been checked.
Assignee: mkanat → wicked
Blocks: 238870
Summary: Make hidden-fields template and User Matching use direct CGI instead of [% form.foo %] → Make hidden-fields template, User Matching and Flags use direct CGI instead of [% form.foo %]
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
This converts the FORM hash to CGI object in hidden-fields template, User
Matching and Flags and is based on patch from GavinS. I briefly tested this to
work.

I did left little bit of compatibility code on Bugzilla/User.pm so that changes
to parameters are copied back to FORM hash. At least process_bug.cgi excepts
this routine to delimit fields with multiple values with spaces. This code can
be removed after everything is converted to CGI.
Attachment #178860 - Flags: review?(mkanat)
Comment on attachment 178860 [details] [diff] [review]
FORM conversion, V1

Architecturally, this looks OK, from a quick overview.

However, it's a lot of Flags code, and I don't really understand Flags code too
much.

But LpSolit does... :-)
Attachment #178860 - Flags: review?(mkanat) → review?(LpSolit)
Blocks: 238875
Status: NEW → ASSIGNED
Blocks: 284301
Comment on attachment 178860 [details] [diff] [review]
FORM conversion, V1

>--- attachment.cgi	18 Mar 2005 03:23:54 -0000	1.77
>+++ attachment.cgi	28 Mar 2005 23:31:23 -0000
>@@ -126,10 +126,11 @@
>-  Bugzilla::User::match_field({ '^requestee(_type)?-(\d+)$' => 
>-                                    { 'type' => 'single' } });
>-  Bugzilla::Flag::validate(\%::FORM, $bugid);
>-  Bugzilla::FlagType::validate(\%::FORM, $bugid, $::FORM{'id'});
>+  Bugzilla::User::match_field($cgi, {
>+      '^requestee(_type)?-(\d+)$' => { 'type' => 'single' }
>+  });

Nit: why do you put }); on a new line??


>--- template/en/default/global/hidden-fields.html.tmpl	18 Jan 2004 18:39:28 -0000	1.8
>+++ template/en/default/global/hidden-fields.html.tmpl	28 Mar 2005 23:31:24 -0000
>@@ -20,23 +20,22 @@
>+[% FOREACH field = cgi.param() %]
>+  [% NEXT IF exclude && field.search(exclude) %]
>+  [%# The '.slice(0)' bit is here to force the 'param(field)' to be evaluated
>+      in a list context, so we can avoid extra code checking for single valued or
>+      empty fields %]
>+  [% FOREACH mvalue = cgi.param(field).slice(0) %]
>+    <input type="hidden" name="[% field FILTER html %]"
>              value="[% mvalue FILTER html FILTER html_linebreak %]">
>     [% END %]
> [% END %]

Nit: The identation of the [% END %] is incorrect.


This is a good job! r=LpSolit

Note to checker-in: This patch has to checked in first! Next one will be bug
287947.
Attachment #178860 - Flags: review?(LpSolit) → review+
Flags: approval?
Blocks: 287947
(In reply to comment #17)
> Nit: why do you put }); on a new line??

Because that way it doesn't need to be adjusted when/if more fields need to
matched. Also the block looks better in my eyes when lines are symmetrical and
there is a clear ending line. :)
Flags: approval? → approval+
Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.81; previous revision: 1.80
done
Checking in editwhines.cgi;
/cvsroot/mozilla/webtools/bugzilla/editwhines.cgi,v  <--  editwhines.cgi
new revision: 1.5; previous revision: 1.4
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.110; previous revision: 1.109
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.246; previous revision: 1.245
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.35; previous revision: 1.34
done
Checking in Bugzilla/FlagType.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v  <--  FlagType.pm
new revision: 1.14; previous revision: 1.13
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.52; previous revision: 1.51
done
Checking in template/en/default/global/confirm-user-match.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/confirm-user-match.html.tmpl,v
 <--  confirm-user-match.html.tmpl
new revision: 1.11; previous revision: 1.10
done
Checking in template/en/default/global/hidden-fields.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/hidden-fields.html.tmpl,v
 <--  hidden-fields.html.tmpl
new revision: 1.9; previous revision: 1.8
done
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Blocks: 315339
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.