100094 - General template handling code.

Status: RESOLVED FIXED

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

Description

[Matthew Tuck [:CodeMachine]]

We need to move some of Myk's template handling code from the attachment manager
to CGI.pl, to reuse common code.

Comment 1

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Bug 103778 does this.

Comment 3

[Bradley Baetz (:bbaetz)]

The generic stuff is in, and I converted existing templates to use the generic
code (then I found this existing bug).

Taking. I'd like this for 2.16, since it simplifies stuff a lot, and avoids the
overhead of creating another template object for each script. It also makes
template precompilation really easy, and I think that we want that for 2.16
since its meant to be a huge perf win.

This should give identical results - the only differences will be for templates
which don't currently have PRE_CHOMP/TRIM enabled. I don't think that this will
cause any real difference, since we only have html templates ATM, and those
don't really care.

The patch I'll attach does:

my $template = $::template;
my $vars = $::vars;

in each file (I got this trick from gerv's enter_bug.cgi), on the basis that the
$:: form is ugly, and one day we'll require perl 5.6 and can use 'our' and then
just get rid of those lines. It should also make moving those vars into a
package easier. I also had to fudge some of the use vars stuff because of this.

I also removed RELATIVE from the template options, since thats only needed for
template files beginning with '.'. Our templates are all relative to
INCLUDE_PATH, not cwd, so we don't need it.

Comment 4

[Bradley Baetz (:bbaetz)]

Attachment: patch

Comment 5

[Matthew Tuck [:CodeMachine]]

Comment on attachment 68108 [details] [diff] [review]
patch

This looks reasonably good, but there are a few small problems.

Firstly, lsearch should be moved to the general code as it is general and used
in multiple places.

Also, you introduced a sillyness sub - everywhere else we use use vars now.

I can't comment on RELATIVE removal, I'd like to see Myk approve that.

Comment 6

[Bradley Baetz (:bbaetz)]

Attachment: new patch

I thought use vars didn't work in .pm files, but then I noticed Bug.pm was
using it. I needed to have the use above the package statement, not below it...


lsearch moved to globals.pl, and removed from plases which used it

Comment 7

[Bradley Baetz (:bbaetz)]

Err, ignore the COMPILE_DIR addition. Thats for a separate bug.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 68112 [details] [diff] [review]
new patch

Looks good, works well.  Minor issues:

>+my $template = $::template;
>+my $vars = $::vars;

Once you "use vars" you no longer need to do this for CGI scripts 
that "require CGI.pl", although you still need it for modules 
like Attachment.pm.


>Index: createaccount.cgi

>+$template = $::template;
>+$vars = $::vars;

Needs "my", but might as well just remove it entirely.


>Index: globals.pl

>+    # Enable compiled templates
>+    COMPILE_DIR => 'data/templates', 

Leave this for bug 97832.

Comment 9

[Bradley Baetz (:bbaetz)]

Attachment: v3

I never realised that use vars did that. Cool.

Comment 10

[Bradley Baetz (:bbaetz)]

Blocks template precompilation (bug 97832), which is a 2.16 blocker, so moving
to 2.16

Comment 11

[Matthew Tuck [:CodeMachine]]

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

We need to make sure we don't insert any more instances of this.

Comment 12

[Andreas Franke (gone)]

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

r=afranke.

I noticed that RELATIVE has been removed on purpose. I have applied the patch
locally, and the only conflicts I got were due to another patch I have
installed.

Seems to work fine.

Comment 13

[Bradley Baetz (:bbaetz)]

Yes, RELATIVE was intentionally removed - see the end of comment #3

Checked in.