Closed
Bug 374331
Opened 18 years ago
Closed 18 years ago
Bugzilla::Template should use template_include_path from Bugzilla::Install::Util
Categories
(Bugzilla :: User Interface, enhancement)
Tracking
()
RESOLVED
FIXED
Bugzilla 3.2
People
(Reporter: mkanat, Assigned: mkanat)
References
Details
Attachments
(1 file, 1 obsolete file)
8.61 KB,
patch
|
mkanat
:
review+
|
Details | Diff | Splinter Review |
I re-wrote getTemplateIncludePath to be a generic function, and moved it to Bugzilla::Install::Util, so that I could use it during installation time. I wrote it and expected that Bugzilla::Template would use it, but Bugzilla::Template is outside of my ownership, so I'm filing this separate bug to start using that code in Bugzilla::Template.
Assignee | ||
Comment 1•18 years ago
|
||
Okay, here we go. I suppose you'll want to look over Install::Util::template_include_path--that's the real bulk of the review, probably. :-) Note that this eliminates the need for defaultlanguage.
Comment 2•18 years ago
|
||
Comment on attachment 258913 [details] [diff] [review] v1 >+C<$project> has to do with installations that are using the C<$ENV{PROJECT}> >+variable to have different "views" on a single Bugzilla. Nit: $project is the only one unfamiliar to me, but it's not the only one that will be unfamiliar to a new Bugzilla programmer looking at this code, so you should probably explain what custom and default are, too (at least custom). >+Note that languages are sorted by the user's preference (as specified >+in their browser, usually), and extensions are sorted alphabetically. Nit: alphabetically -> alphanumerically? (installations might prepend numbers onto these directories to order them in a specific order). Otherwise this looks fine, applies mostly cleanly (with just one patch conflict that is trivial to resolve), passes a basic test, and the code in Utils.pm looks much better than the old code in Template.pm. One note: after applying the patch, getTemplateIncludePath gets calls many times per request, whereas before it was only called once per request. Will this affect performance?
Attachment #258913 -
Flags: review?(myk) → review+
Comment 3•18 years ago
|
||
Keeping this patch in our radar. Max, I will let you approve it.
Flags: approval?
Assignee | ||
Comment 4•18 years ago
|
||
Calling getTemplateIncludePath multiple times is OK, because the result is cached. I fixed the nits and I'll upload a copy of what I actually checked in. Checking in Bugzilla/Template.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v <-- Template.pm new revision: 1.72; previous revision: 1.71 done Checking in Bugzilla/Config/L10n.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/L10n.pm,v <-- L10n.pm new revision: 1.3; previous revision: 1.2 done Checking in Bugzilla/Install/Util.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Util.pm,v <-- Util.pm new revision: 1.5; previous revision: 1.4 done Checking in template/en/default/admin/params/l10n.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/l10n.html.tmpl,v <-- l10n.html.tmpl new revision: 1.2; previous revision: 1.1 done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Flags: approval? → approval+
Resolution: --- → FIXED
Assignee | ||
Comment 5•18 years ago
|
||
Here's the version I checked in. (Carrying forward r+)
Attachment #258913 -
Attachment is obsolete: true
Attachment #263869 -
Flags: review+
You need to log in
before you can comment on or make changes to this bug.
Description
•