163829 - Move pref code into a separate package

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Bradley Baetz (:bbaetz)]

Need to move the Param function (and localconfig vars) to a separate package.
The versiontable stuff may wait until later; not sure about that. %::param may
still be global, though, at least for the moment

Comment 1

[Bradley Baetz (:bbaetz)]

Attachment: WIP

OK, heres a work in progress. See the XXX comments for what still needs doing.
(Except the mod_perl related stuff, which I've been marking as I go through for
stuff to do later)

Apart from the code movement, I had to do several things:

- namespacing issues meant that $::param in data/params was no good. Vars not
being global meant that the eval tricks played to write the perl code out
didn't quite work I ended up using Data::Dumper  instead.

- The params file is now loaded in a Safe instead of munging the symbol table
(for backwards compat) and using eval (I left that in, but commented out).
However, we should think about not executing the config file, and using
lib/Config.pm from mozbot instead, or something else along those lines

- checksetup.pl was changed to deal with the fact that it didn't have direct
access to the param array any more.

- the version is now not a param, but is instead in $Bugzilla::Config::VERSION.
The callers were updated. This will break performsubsts, but since the stuff
which would have used that is now a template I'm not sure if that matters. I
can always hack the Param sub to work, though, if people do think that its an
issue

- WriteParams uses File::Temp instead of data/params.$$ (I had to rewrite that
anyway, so...)

- DefParam is gone; defparams.pl has an array of anon hashes - see comments in
that file. (I may move that somewhere else; again, see the comments). As part
of this splitup, (and the removal of the global %::param hash), defaults can't
contain other params. Bugzilla::Config has an 'update' section which can set
that if you want; the quips stuff was moved to take advantage of that

- multi/single select stuff uses the value, not the number (for editing), and
has a separate defaults entry in the hash. This made lots of code simpler.

- editparams/doeditparams were changed to cope with not having direct access to
defaults and so on

- Probably toher stuff I've forgotten

Comments?

Comment 2

[Myk Melez [:myk] [@mykmelez]]

I haven't looked at the patch yet, but several comments on the design:

1. App::Config may be useful here.  It's required by the Template Toolkit, so
all Bugzilla installations should have it.

2. Whatever we do, it should still be possible to make a variety of
configuration changes via editparams.cgi.

3. I assume you are doing this already, but make sure your changes make
mod_perlification easier/possible.

Comment 3

[Bradley Baetz (:bbaetz)]

1) a non-executable file format can come later; this patch has enough in it

2) Nothing changed in that regard

3) again, nothing has changed, except for some XXX comments on trouble spots (ie
wher we load the prefs file)

Comment 4

[Bradley Baetz (:bbaetz)]

Attachment: v1

OK, I think that this is ready.

Remaining 'XXX' comments are for things which we may want to do in the future.

Comment 5

[Joel Peshkin]

Comment on attachment 96961 [details] [diff] [review]
v1

r=joel
inspected and tested.  Needs a quick 2xr by someone with a good vision about
how the modularity of the code should be maintained.

Comment 6

[J. Paul Reed [:preed]]

Comment on attachment 96961 [details] [diff] [review]
v1


Some relatively big nits here, but I leave all of them to your good judgment.
r2=preed

Oh, and BTW: thanks for cleaning up some of that horrid multi-select code. :-)

>+# XXXX - would be nice for doeditparams to 'know' about types s and m, and call
>+# check_multi without it having to be explicitly specified here

Nit: comments like these throughout this patch are useful, but it would be nice
to know a) who put them there, and b) which bug number the original patch was
attached to, so future BZ devs don't have to fire up bonsai to find out from
whence these comments came.

>-    $::FORM{$i} =~ s/\r\n?/\n/g;   # Get rid of windows/mac-style line endings.
>-    $::FORM{$i} =~ s/^\n$//;      # assume single linefeed is an empty string
>-    if ($::FORM{$i} ne Param($i)) {
>-        if (defined $::param_checker{$i}) {
>-            my $ref = $::param_checker{$i};
>-            my $ok = &$ref($::FORM{$i}, $i);
>+    if ($changed) {
>+        if (exists $i->{'checker'}) {

Nit: Since there is no DefParam() anymore, I don't see a place where you check
to ensure that 'checker' really is a code ref. Recommend (here or in
Bugzilla::Config in an appropriate spot):

if (exists $i->{'checker'}) {
   ThrowCodeError("Checker not a valid CODE ref") unless (ref($i->{'checker'})
eq 'CODE');
   ## ...

>+
>+    # Param conversion code goes here!
>+    # Note that this isn't particuarly 'clean' in terms of separating
>+    # the backend code (ie this) from the actual params.
>+    # We don't care about that, though
>+    # The alternate is to have checksetup do this conversion
>+    # XXX - should we do that for 'perf' reasons, anyway?
>+    # Its definately not noticable, but it may add up in theory

I would like to see this moved to checksetup, not so much for perf reasons, but
moreso because semantically, it seems like all the update/changing code is in
checksetup. It's kind of odd to find that code here in the module itself.

Maybe you could separate it out into a Bugzilla::Config::UpdateParams() and
have checksetup call that if you don't want to actually move the code out to
checksetup (this would also have the benefit of still centralizing the code
here and allowing it to be implementation independent of checksetups calling
it.)

>+sub Param ($) {
>+sub SetParam($$) {

I thought we had decided no function signatures.

I know you're just carrying this over, but if that's the case, why not drop it
while we're breaking everything?

>Index: Bugzilla/Util.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v
>retrieving revision 1.4
>diff -u -r1.4 Util.pm
>--- Bugzilla/Util.pm	27 Aug 2002 04:27:56 -0000	1.4
>+++ Bugzilla/Util.pm	28 Aug 2002 03:09:47 -0000
>@@ -258,3 +258,5 @@
> =back
> 
> =cut
>+
>+1;

Nit: does this have anything to do with this patch?

Comment 7

[Bradley Baetz (:bbaetz)]

> [ XXX comments ]

OK, I'll add my name to those. I'll file a coule of the bugs, too, and then add
those numbers

> [ ensure checker is a coderef ]

There was no such check before, though. This code is part of bugzilla, remember
- the admin doesn't touch this. I don't think its worthwhile, since its going to
be obvious when an error like that occurs.

> [function signatures]

I thikn they're useful; Now that stuff is in modules, teh compile time checks
perl does can actually occur.

> [Bugzilla::Util]

At one point I had code in there, and then it was needed. We should do it anyway
- it was an oversight that I didn't do this earlier.

I'll move the init code arround, though

Comment 8

[Bradley Baetz (:bbaetz)]

Attachment: v2

OK, try this one. I cleaned up the init code a bit, basically, and did most of
the other changes requested.

I dropped all the prototypes; I don't see why people don't like them, but...

I also rearranged the order of some subs and added pod docs

Comments?

Comment 9

[Joel Peshkin]

Comment on attachment 96998 [details] [diff] [review]
v2

r=joel

Comment 10

[J. Paul Reed [:preed]]

Comment on attachment 96998 [details] [diff] [review]
v2

Looks good.

r=preed.

Comment 11

[Bradley Baetz (:bbaetz)]

Checked in a few hours ago