Max Kanat-Alexander Assignee
	Description • 18 years ago

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.

	Max Kanat-Alexander 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.

	Myk Melez [:myk] [@mykmelez]
	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?

	Frédéric Buclin
	Comment 3 • 18 years ago

Keeping this patch in our radar. Max, I will let you approve it.

	Max Kanat-Alexander 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

	Max Kanat-Alexander Assignee
	Comment 5 • 18 years ago

Here's the version I checked in. (Carrying forward r+)