Closed Bug 163291 Opened 18 years ago Closed 18 years ago
Move utility funcs into a module
We need to move stuff like is_tainted into a module. The reason for this is that then we can use them from other modules without having namespace issues + ordering isues wrt require: [since require only happens once, if you |require| into the global namespace then code in packages need to use &::func() all the time. If you |require| into the pakcage namespace, then nothing else can touch it. This means that all .cgis have to |require globals.pl| and then |use Bugzilla::Foo|, which is a real pain, and its ugly] Its also cleaner/easier to maintain
OK, this should be trivial. Things to note: a) I removed our html_quote filter from the Template stuff. The TT one is ok in the version we require, and I had to modify it anyway for scoping reasons. b) I POD-ified the docs. They're not perfect, but they're better than nothing c) I left calls to |use Bugzilla::Util| in globals.pl and CGI.pl so that existing code still works w/o needig the extra |use| call. This will be requried eventually, though d) I updated the various pm's not to use the &:: form for calls which were added to Utils.pm To be moved to Utils.pm, the function needed to not involve configuration params (eg Param, localconfig vars), the database (bug 163290 will move most of those), or anything which called the db - they should all go somewhere else.
Given your criteria, why weren't the following moved to Util.pm? In globals.pl: Crypt(); GenerateRandomPassword(); ModTime(); quoteUrls(); DiffStrings(); Also, in checksetup.pl and globals.pl you remove a function ref to html_quote in some of the templating code; I'm not a TT expert, so why was that done?
The filter was removed because of namnespacing issues, + we don't need it anyway. I managed to fix the namespacing stuff, though, so I guess that that could be part of a separate bug The real criteria were 'functions which I'd have to use &:: on in other modules' :) That said: a) Its easy to move them later b) GenerateRandomPassword - belongs in User.pm Crypt - unclear where this goes; User.pm may be the only user of it ModTime - only used by GetVersionTable, which will go into Bugzilla::Config.pm soonish quoteUrls - this will become a FILTER when I do Bugzilla::Template.pm DiffStrings - I have no clue where this belongs; it will depend on how the Bug.pm back/front end separation goes; currently attachment.cgi and proess_bug.cgi duplicate a bit of code c) Its easy to move them later
Comment on attachment 95731 [details] [diff] [review] patch *I'm* convinced. This seems like a no-brainer and bbeatz tested w/ a running installation, so 2xr=preed
Attachment #95731 - Flags: review+
OK, checked in.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment on attachment 95731 [details] [diff] [review] patch >+# Contributor(s): Terry Weissman <email@example.com> >+# Dan Mosedale <firstname.lastname@example.org> >+# Jake <email@example.com> >+# Bradley Baetz <firstname.lastname@example.org> >+# Christopher Aillon <email@example.com> >+ >+package Bugzilla::Util; >+ >+=head1 NAME >+ >+Bugzilla::Util - Generic utility functions for bugzilla >+ >+=head1 SYNOPSIS Your perldoc style has a lot of whitespace in it. We don't have a style guide for this (yet), but it would be nice (if it's possible; I don't know perldoc's whitespace rules) to use less. >+B<It is not intended as a general dumping group for something which >+people feel might be useful somewhere, someday>. Do not add methods to this Presumably you mean "other than in the place where it's used now." :-) >+B<WARNING!! Using this routine on data that really could be tainted defeats >+the purpose of taint mode. It should only be used on variables that have been sanity checked in some way and have been determined to be OK.> You have a very long line here. Gerv
Nope, you need blank lines between =foo commands The generated manpages don't have that, though - try |perldoc Bugzilla/Util.pm| > Presumably you mean "other than in the place where it's used now." :-) Not entirely - whilst max and min take arbitary arguments, the only user (the dependancy graph stuff) uses the two arg format only. Someone thought that it would be useful, I guess. Besides, the following sentence covers that part. I've fixed the long line in CVS now.
You need to log in before you can comment on or make changes to this bug.