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

Status: RESOLVED FIXED

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

Description

[Dave Miller [:justdave]]

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.

Comment 1

[GavinS]

Attachment: use $cgi in user matching and hidden fields code

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.

Comment 2

[Vlad Dascalu]

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.

Comment 3

[Dave Miller [:justdave]]

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.

Comment 4

[Dave Miller [:justdave]]

I typoed one of those... you want .slice(0), not .splice(0).

Comment 5

[GavinS]

Attachment: use $cgi in user matching and hidden fields code, take 2

Uses slice(0), and hopefully made the CGI comments more consist

Comment 6

[Dave Miller [:justdave]]

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.

Comment 7

[GavinS]

For the latest patch (and subsequent activity), see bug#238870, which contains a combined patch for 
process_bug, attachment, hidden fields and flags

Comment 8

[Dave Miller [:justdave]]

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.

Comment 9

[Max Kanat-Alexander]

I'll take this and move it back from the parent bug so that I can handle bug 238877.

Comment 10

[GavinS]

(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...

Comment 11

[Max Kanat-Alexander]

Yeah, I usually end up writing transition code in situations like this.

Comment 12

[Max Kanat-Alexander]

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.

Comment 13

[Max Kanat-Alexander]

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...

Comment 14

[Teemu Mannermaa (:wicked)]

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.

Comment 15

[Teemu Mannermaa (:wicked)]

Attachment: FORM conversion, V1

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.

Comment 16

[Max Kanat-Alexander]

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... :-)

Comment 17

[Frédéric Buclin]

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.

Comment 18

[Teemu Mannermaa (:wicked)]

(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. :)

Comment 19

[Frédéric Buclin]

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