Closed Bug 163291 Opened 22 years ago Closed 22 years ago

Move utility funcs into a module


(Bugzilla :: Bugzilla-General, defect)

Not set



Bugzilla 2.18


(Reporter: bbaetz, Assigned: bbaetz)




(1 file)

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| and then |use Bugzilla::Foo|, which is a real pain,
and its ugly]

Its also cleaner/easier to maintain
Blocks: 163290
Attached patch patchSplinter Review
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 and 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

To be moved to, 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.
gerv, review?
Keywords: patch, review
Target Milestone: --- → Bugzilla 2.18
Given your criteria, why weren't the following moved to


Also, in and 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
   Crypt - unclear where this goes; may be the only user of it
   ModTime - only used by GetVersionTable, which will go into soonish
   quoteUrls - this will become a FILTER when I do
   DiffStrings - I have no clue where this belongs; it will depend on how the 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]

*I'm* convinced.

This seems like a no-brainer and bbeatz tested w/ a running installation, so
Attachment #95731 - Flags: review+
OK, checked in.
Closed: 22 years ago
Resolution: --- → FIXED
Comment on attachment 95731 [details] [diff] [review]

>+# Contributor(s): Terry Weissman <>
>+#                 Dan Mosedale <>
>+#                 Jake <>
>+#                 Bradley Baetz <>
>+#                 Christopher Aillon <>
>+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.

Nope, you need blank lines between =foo commands

The generated manpages don't have that, though - try |perldoc Bugzilla/|

> 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.
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.