Closed Bug 100094 Opened 23 years ago Closed 23 years ago

General template handling code.


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




Bugzilla 2.16


(Reporter: CodeMachine, Assigned: bbaetz)




(1 file, 2 obsolete files)

We need to move some of Myk's template handling code from the attachment manager
to, to reuse common code.
Priority: -- → P2
Summary: → General template handling code.
Target Milestone: --- → Bugzilla 2.16
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.
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
Bug 103778 does this.
Depends on: 103778
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.
Assignee: justdave → bbaetz
Keywords: patch, review
Attached patch patch (obsolete) — Splinter Review
Comment on attachment 68108 [details] [diff] [review]

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.
Attachment #68108 - Flags: review-
Attached patch new patch (obsolete) — Splinter Review
I thought use vars didn't work in .pm files, but then I noticed was
using it. I needed to have the use above the package statement, not below it...

lsearch moved to, and removed from plases which used it
Attachment #68108 - Attachment is obsolete: true
Err, ignore the COMPILE_DIR addition. Thats for a separate bug.
Blocks: 97832
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", although you still need it for modules 

>Index: createaccount.cgi

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

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


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

Leave this for bug 97832.
Attached patch v3Splinter Review
I never realised that use vars did that. Cool.
Attachment #68112 - Attachment is obsolete: true
Blocks template precompilation (bug 97832), which is a 2.16 blocker, so moving
to 2.16
Target Milestone: Bugzilla 2.18 → Bugzilla 2.16
Blocks: 119657
Comment on attachment 68128 [details] [diff] [review]

We need to make sure we don't insert any more instances of this.
Attachment #68128 - Flags: review+
Comment on attachment 68128 [details] [diff] [review]


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

Seems to work fine.
Attachment #68128 - Flags: review+
Yes, RELATIVE was intentionally removed - see the end of comment #3

Checked in.
Closed: 23 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.