163291 - Move utility funcs into a module

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Bradley Baetz (:bbaetz)]

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

Comment 1

[Bradley Baetz (:bbaetz)]

Attachment: patch

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.

Comment 2

[Bradley Baetz (:bbaetz)]

gerv, review?

Comment 3

[J. Paul Reed [:preed]]

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?

Comment 4

[Bradley Baetz (:bbaetz)]

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 5

[J. Paul Reed [:preed]]

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

Comment 6

[Bradley Baetz (:bbaetz)]

OK, checked in.

Comment 7

[Gervase Markham [:gerv]]

Comment on attachment 95731 [details] [diff] [review]
patch

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

Comment 8

[Bradley Baetz (:bbaetz)]

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.