328642 - Params should be in Bugzilla->params instead of being a Bugzilla::Config subroutine

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement, P2)

Description

[Max Kanat-Alexander]

"Params" are really a global object of Bugzilla. We get into a lot of trouble in various places, because we have to use Bugzilla::Config to call the "Params" subroutine, but Bugzilla::Config does all sorts of fancy stuff every time it starts, which has caused me all sorts of trouble throughout time, and also (because of the general architecture of Bugzilla::Config) will cause problems with mod_perl.

So I was thinking that instead we could have a Bugzilla->params function, and get them out of there.

Comment 1

[Max Kanat-Alexander]

*** Bug 328641 has been marked as a duplicate of this bug. ***

Comment 2

[Max Kanat-Alexander]

Attachment: Non-Working Patch

Okay, here's a patch that *should* work but doesn't.

Thanks to the fabulous BEGIN block in Bugzilla::Auth that tries to load a bunch of weird stuff, this doesn't work.

Comment 3

[Frédéric Buclin]

Comment on attachment 213247 [details] [diff] [review]
Non-Working Patch

>Index: checksetup.pl

>+# Because of the strange way that params work, "require" and "use" both
>+# fail here, but "eval use" works fine.
>+eval("use Bugzilla");

Why do you need this line? Bugzilla.pm doesn't export anything AFAIK. And Param() is still in Config.pm.

Comment 4

[Max Kanat-Alexander]

(In reply to comment #3)
> Why do you need this line? 

  Because Bugzilla::Config::Param() uses Bugzilla->params.

Comment 5

[Max Kanat-Alexander]

Attachment: v1

Okay, with the blocker applied, this patch works.

I tested checksetup creating a new installation without a data/ directory, and also running checksetup against an existing data/ directory, so I know the checksetup code works. I also checked it on a perl installation that didn't have Bugzilla's required perl modules installed, and checksetup worked fine.

In other words, I tested the checksetup part quite thoroughly.

Comment 6

[Frédéric Buclin]

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

>Index: Bugzilla.pm

> if (!$^C
>-    && Param("shutdownhtml") 
>+    && Bugzilla->params->{"shutdownhtml"}


>+my $_params;
>+sub params {
>+    my $class = shift;
>+    $_params ||= _load_param_values();
>+    return $_params;
>+}

I'm really not conviced by this Param('foo') -> Bugzilla->params->{'foo'} change. If data/params doesn't exist, Bugzilla->params->{'foo'} contains no data while Param('foo') will fall back to the default value. So from my point of view, Bugzilla->params should complete missing data by their default ones. Ideally, it should probably load default values, and replace those by values defined in data/params.

Moreover, Bugzilla->params need POD.

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> If data/params doesn't exist, Bugzilla->params->{'foo'} contains no
> data while Param('foo') will fall back to the default value.

  Actually, Param('foo') only falls back to the default value when we're doing a compile-only perl call. (That is, a perl -c.) It's only for runtests. It's not necessary behavior at this point. param->{'foo'} will work fine during compilation, even if Param('foo') wouldn't.

  Anyhow, compile-time should never need Param('foo') to work properly. That's pretty silly. We needed that for things like the old Bugzilla::Auth begin block.

Comment 8

[Max Kanat-Alexander]

Attachment: v2

Okay, here's a version with POD. See my comment above for why we don't need to return the default values when data/params doesn't exist.

Comment 9

[Frédéric Buclin]

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

r=LpSolit

Comment 10

[Dave Miller [:justdave]]

Hmm, yummy. :)

Comment 11

[Max Kanat-Alexander]

Checking in Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.33; previous revision: 1.32
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.479; previous revision: 1.478
done
Checking in Bugzilla/Config.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config.pm,v  <--  Config.pm
new revision: 1.56; previous revision: 1.55
done

Comment 12

[Max Kanat-Alexander]

Added to the release notes in bug 349423.