Closed Bug 145030 Opened 22 years ago Closed 22 years ago

verify-new-product template uses cgi.pm?

Categories

(Bugzilla :: Creating/Changing Bugs, defect)

Product:
Bugzilla
Component:
Creating/Changing Bugs
Version:
2.15
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 2.16

People

(Reporter: jouni, Assigned: myk)

Details

Attachments

(1 file, 3 obsolete files)

patch v3: options instead of values; more comments; formatting fixes
3.82 KB, patch
jouni
: review+
gerv
: review+
Details | Diff | Splinter Review 
The problem I encountered first is that the TT's use clause in
verify-new-product.html.tmpl seems to freeze in my Win32/IIS5 installation,
causing a CGI timeout. That's a problem in itself, one which has to be examined
closer if the following one isn't solved.

The more interesting question: why is cgi.pm used here, and if this is
intentional, should we require it in checksetup.pl?
I used CGI.pm in that template because it's an elegant way of generating HTML. 
It doesn't need to be required in checksetup.pl because it comes standard with
all recent Perl distributions.

We should be using CGI.pm methods more and more to replace our homegrown
equivalents in CGI.pl (and in lieu of TT snippets), but we could excise this
code on the 2.16 branch (putting a TT snippet or one of our homegrown functions
in its place) for the 2.16 release if the code is causing problems.
I think it would be wisest to use the old approach in 2.16. The USE statement in
TT definitely seems to have some off problems on my three different Windows test
platforms - all of them seem to freeze.

I agree about migrating to CGI.pm, but I think whole Bugzilla should aim to do
that in one step (hopefully not one patch, but within the span of one release -
2.18 or 2.20 then?).
What version of CGI.pm do you have? (and what perl version?)

Are you using IIS or apache/win32?
IIS5, W2k, ActiveState Perl 5.6.1. I can't remember the CGI.pm version right
now, but I can check it out later. 

The other two configurations are close to the same, but don't have the specs
here offhand. All are W2k/IIS boxes anyway. 
what happens if you change the USE CGI to USE CGI (-no_debug)

I'm not sure if that works in TT - you may have to play arround a bit.
A good guess there, bbaetz. The proper syntax is [% USE mycgi = CGI (no_debug)
%] and that does it, at least on my home test system. The CGI module version
number is 2.74.

Ok, since this apparently can be fixed this way, I'll attach a patch to add the
no_debug switch to the use clause. I'm still not happy with one part of Bugzilla
using CGI.pm when a lot of work has been done elsewhere to avoid using it, but
if Myk feels it's better to this way, then I just propose adding the no_debug flag.

Should IMO be resolved before 2.16, one way or another.
Target Milestone: --- → Bugzilla 2.16
Attached patch Patch (obsolete) — — Splinter Review 
Add no_debug to verify-new-product's use cgi call.
Are you sure its not -no_debug, rather than no_debug? Thats what the docs say to do.

If you upgrade CGI.pm, does that fix it? I know that I didn't need the -no_debug
stuff we added to the testing suite, somehow.
Keywords: patch, review
I wouldn't say we avoid using CGI.pm, I think the reasons we don't use it are
historical.  Converting to templates doesn't really mean we shouldn't use it, I
think it's a good idea personally.  We should document its usage if it starts
spreading through the code, however.
Even documenting the use isn't enough IMO. The shift from a custom CGI module to
the common cgi.pm should be done with some coordination (a single patch or at
least a group of patches reviewed together). It's not good for just some
templates to start using cgi.pm whenever their authors so wish. It leads to
obscure bugs such as this, where one seemingly innocent part of the system fails
because there are some unknown incompatibilities. 

Bbaetz, re comment 8: I'm sure it's no_debug without the hyphen, no matter how
illogical it sounds. I should try upgrading cgi.pm, but haven't had the time to
test this. I'll try to do this tonight.
Jouni: Other places in the code use -no_debug, though. Can you look at CGI.pm to
check?

My version has:

        $DEBUG=0,                next if /^[:-]no_?[Dd]ebug$/;

which seems to require the -.
Comment on attachment 84055 [details] [diff] [review]
Patch

Oh dear. I tried "USE mycgi = CGI (abcdefghijklmnop)" and it works fine.
Actually, the relevant thing seems to be SOME parameters for the CGI. It's not
relevant which ones they are. I upgraded CGI.pm to 2.81 but with no apparent
effect.

BUT: if I tap in "USE mycgi = CGI (-no_debug)", note the hyphen, the I get the
following: "file error - parse error: bug/process/verify-new-product.html.tmpl
line 36: unexpected token (-) [% USE mycgi = CGI (-no_debug) %]"

That makes me think it would be a TT problem. Any ideas on this?

Obsoleting my patch proposal because it has become misleading in the light of
last discoveries.
Attachment #84055 - Attachment is obsolete: true
Try taking it to the TT list (www.tt2.org).
So are we doing the quick hack here for 2.16?
OK, executive decision here...

get rid of the CGI.pm calls for 2.16.  Let's open a new bug about transitioning
our code to CGI.pm and eliminating CGI.pl later.

This likely ain't going to make 2.16rc2 because it's probably too big a job, but
we should have this in by the 2.16 GM (or rc3 if we end up doing one).
Keywords: patch, review
Ok, moving towards CGI.pm is now bug 147833.

I'll mail the TT list about the problems with USE cgi and parameters on Win32.
But that will have to wait until I get the time to test that a bit more thoroughly.
Shouldn't this include a minor version number bump?
Comment on attachment 85525 [details] [diff] [review]
patch v1: select-menu.html.tmpl replaces CGI.pm calls

r=gerv. Does TT not already provide some way of determining the type of
variables?

Gerv
Attachment #85525 - Flags: review+
>Shouldn't this include a minor version number bump?

No, since the interface for the template isn't changing.

>Does TT not already provide some way of determining the type of variables?

Not that I can find.  Using array and hash references in scalar context returns
a scalar representation of the reference starting with the words ARRAY and HASH,
respectively, but I know no good clean way of extracting that reference.
This patch grabs the variable type of the "values" variable without adding
additional pseudo-methods.
Comment on attachment 85643 [details] [diff] [review]
patch v2: same fix, but without adding "type" pseudo-methods

>Index: template/en/default/global/select-menu.html.tmpl
>===================================================================
>+    [% FOREACH value = values %]
>+      <option value="[% value.value FILTER html %]" 
>+        [% " selected" IF value.value == default %]>
>+        [% value.key FILTER html %]
>+      </option>
>+    [% END %]

This "value.value" thing is quite horrible. Could that "values"
be "choices" or "options" (per the HTML terminology)? A select
element is not given values but choices or options IMO. Choosing
between those two terms is a matter of taste - I'm slightly in
favor of choices but would be fine with options as well. :-)

Anyway, this works fine on Linux and W2k. Fix that values thing
above, and r=jouni.
Comment on attachment 85643 [details] [diff] [review]
patch v2: same fix, but without adding "type" pseudo-methods

Makes more sense :-)

Gerv
Attachment #85643 - Flags: review+
Regarding issue mentioned in comment 12: 

I believe there is some TT-internal problem with using the CGI.pm. I wrote about
this to the TT list and Randal Schwartz replied saying he had a similar problem.
See his post:

http://www.template-toolkit.org/pipermail/templates/2002-May/003161.html

We should consider this when moving on to using CGI plugin from TT.

(I'll copy this over to bug 147833 as well)
"values" changed to "options", commented the funky block that lets me determine
whether the reference is to an array or a hash, and consistentized the
formatting.
Attachment #85525 - Attachment is obsolete: true
Attachment #85643 - Attachment is obsolete: true
Comment on attachment 85658 [details] [diff] [review]
patch v3: options instead of values; more comments; formatting fixes

r=jouni
Attachment #85658 - Flags: review+
Comment on attachment 85658 [details] [diff] [review]
patch v3: options instead of values; more comments; formatting fixes

r=gerv.

Gerv
Attachment #85658 - Flags: review+
patch is Myk's...  looks ready for checkin.
Keywords: patch, review
Checked into the trunk and 2.16 branch.  Resolving fixed.

RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/select-menu.html.tmpl,v
done
Checking in template/en/default/global/select-menu.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/select-menu.html.tmpl,v
 <--  select-menu.html.tmpl
initial revision: 1.1
done
Checking in template/en/default/bug/process/verify-new-product.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/process/verify-new-product.html.tmpl,v
 <--  verify-new-product.html.tmpl
new revision: 1.5; previous revision: 1.4
done

Checking in template/en/default/global/select-menu.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/select-menu.html.tmpl,v
 <--  select-menu.html.tmpl
new revision: 1.1.2.1; previous revision: 1.1
done
Checking in template/en/default/bug/process/verify-new-product.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/process/verify-new-product.html.tmpl,v
 <--  verify-new-product.html.tmpl
new revision: 1.3.2.2; previous revision: 1.3.2.1
done
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: