Max Kanat-Alexander Assignee
	Description • 15 years ago

Right now there are certain settings, like cookiedomain, that almost nobody needs to set but people think they do because they're listed as "Required Settings". These should be moved out of Required and into something like "Advanced" or "Network".

	Frédéric Buclin
	Comment 1 • 15 years ago

Should go elsewhere: announcehtml, shutdownhtml, maintainer (should be filed with the user having ID = 1 by default), cookiedomain.
Should stay in Required: urlbase, docs_urlbase, sslbase, ssl, cookiepath, utf8.
Not sure: proxy_url, upgrade_notification (they must stay together)

	Max Kanat-Alexander Assignee
	Comment 2 • 15 years ago

proxy_url should definitely go elsewhere. I understand that proxy_url is used by upgrade_notification, but it needs to be used only by a small number of users and it can easily be in the advanced network settings.

	Max Kanat-Alexander Assignee
	Comment 3 • 15 years ago

Okay, here we go! This is pretty nice. This patch does two big things:

1) Moves non-required "Required" parameters into a General section and an Advanced section.

2) Automatically sets the "maintainer" parameter to the admin created during checksetup.

I can split those out into separate patches if you want.

I moved a few more parameters than we originally discussed, and here's why:

utf8: In fact this parameter will rarely be set, and is not truly a "required" parameter, as it is already set sensibly by default. In fact, in most installations, it is on by default, as it has been since 2.22. (I also removed it from welcome-admin for that reason--if somebody's seeing that page, utf8 is on, and they don't need to change it.)

docs_urlbase: The average installation has no need to actually set this, provided that they downloaded Bugzilla using the tarball. If they downloaded Bugzilla using CVS, we assume that they're intelligent enough to also look through the parameters if things aren't working, for the docs links. (Also, we plan to get rid of this parameter anyhow for bug 399068.)

All of the others ones I think it should be pretty obvious why they were moved.

	Max Kanat-Alexander Assignee
	Comment 4 • 15 years ago

Oh, and this patch requires the simple patch from the blocker, in order to apply and work.

	Frédéric Buclin
	Comment 5 • 15 years ago

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

some bitrot:
  patching file Bugzilla/Config/Core.pm
  Hunk #1 FAILED at 30.


>=== modified file 'Bugzilla/Install/Filesystem.pm'

> sub fix_all_file_permissions {

>     my $ws_group = Bugzilla->localconfig->{'webservergroup'};

This variable is no longer used.



Looks good, but I couldn't test the patch due to the bitrot in Core.pm.

	Max Kanat-Alexander Assignee
	Comment 6 • 15 years ago

Ah, okay. This fixes the bitrot and removes the unused variable.

	Frédéric Buclin
	Comment 7 • 15 years ago

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

This patch conflicts with the one from bug 314871. I would prefer to see the security bug to be fixed first. What do you think?

	Max Kanat-Alexander Assignee
	Comment 8 • 15 years ago

(In reply to comment #7)
> (From update of attachment 417390 [details] [diff] [review])
> This patch conflicts with the one from bug 314871. I would prefer to see the
> security bug to be fixed first. What do you think?

  I don't think that's going to happen before the freeze, so we should just go ahead with this patch here and update the sec patch as needed.

	Frédéric Buclin
	Comment 9 • 15 years ago

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

>Index: Bugzilla/Config/Core.pm

>-   name => 'maintainer',
>-   type => 't',
>-   default => 'please.set.the.maintainer.parameter@administration.parameters',
>-   checker => \&check_email,
>-  },


>Index: Bugzilla/Config/General.pm

>+  {
>+   name => 'maintainer',
>+   type => 't',
>+   default => ''
>+  },

The maintainer parameter no longer has a default value nor a checker. They must stay as a valid email address is expected at several places (especially in email templates, in the To: field).

r=LpSolit with this comment fixed.

	Max Kanat-Alexander Assignee
	Comment 10 • 15 years ago

  Okay, I added the checker back in, but I didn't add in a default value--the make_admin code in Bugzilla::Install depends on the default value being blank. It's OK that the default value is invalid--it should be. The parameter needs to be set, so if somebody goes to the General page of the Parameters and tries to submit it without filling in a maintainer, it won't let them, but the default can be blank for Bugzilla::Install to work and set the maintainer when there isn't one.

Checking in Bugzilla/Config.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config.pm,v  <--  Config.pm
new revision: 1.82; previous revision: 1.81
done
Checking in Bugzilla/Install.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install.pm,v  <--  Install.pm
new revision: 1.24; previous revision: 1.23
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Advanced.pm,v
done
Checking in Bugzilla/Config/Advanced.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Advanced.pm,v  <--  Advanced.pm
initial revision: 1.1
done
Checking in Bugzilla/Config/Core.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Core.pm,v  <--  Core.pm
new revision: 1.14; previous revision: 1.13
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/General.pm,v
done
Checking in Bugzilla/Config/General.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/General.pm,v  <--  General.pm
initial revision: 1.1
done
Checking in Bugzilla/Install/Filesystem.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Filesystem.pm,v  <--  Filesystem.pm
new revision: 1.45; previous revision: 1.44
done
Checking in template/en/default/index.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/index.html.tmpl,v  <--  index.html.tmpl
new revision: 1.48; previous revision: 1.47
done
Checking in template/en/default/welcome-admin.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/welcome-admin.html.tmpl,v  <--  welcome-admin.html.tmpl
new revision: 1.6; previous revision: 1.5
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/advanced.html.tmpl,v
done
Checking in template/en/default/admin/params/advanced.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/advanced.html.tmpl,v  <--  advanced.html.tmpl
initial revision: 1.1
done
Checking in template/en/default/admin/params/core.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/core.html.tmpl,v  <--  core.html.tmpl
new revision: 1.14; previous revision: 1.13
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/general.html.tmpl,v
done
Checking in template/en/default/admin/params/general.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/general.html.tmpl,v  <--  general.html.tmpl
initial revision: 1.1
done

	Frédéric Buclin
	Comment 11 • 15 years ago

(In reply to comment #10)
> It's OK that the default value is invalid--it should be.

Except that when I now click "Reset", an error is thrown. It's a bug.

	Max Kanat-Alexander Assignee
	Comment 12 • 15 years ago

No, you can't reset the maintainer. It must always contain a *valid* email address.

	Frédéric Buclin
	Comment 13 • 15 years ago

In that case, the "Reset" checkbox must be disabled or hidden.

	Max Kanat-Alexander Assignee
	Comment 14 • 15 years ago

Okay, I filed bug 537846 for that.

	Max Kanat-Alexander Assignee
	Comment 15 • 15 years ago

Added to the release notes in bug 547466.

	Frédéric Buclin
	Comment 16 • 15 years ago

We have to update the documentation to reflect where parameters now are:

http://www.bugzilla.org/docs/tip/en/html/parameters.html