676200 - We shouldn't manually delete obsolete parameters in Bugzilla::Config::update_params(), else they are not saved in old-params.txt

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect)

Description

[Frédéric Buclin]

While working on bug 255606, I realized that several old parameters are deleted manually in Bugzilla::Config::update_params() once their value has been used to configure some new parameters. This prevents these old parameters from being listed in data/old-params.txt. Even if it's as trivial as 'new_param' = 'old_param', e.g. when renaming a parameter, both parameters should still be mentioned by checksetup.pl, and the old param be saved anyway.

Comment 1

[Frédéric Buclin]

Attachment: patch, v1

Comment 2

[Frédéric Buclin]

Let's take it for 4.2. It's important for admins to know where parameters are going.

Comment 3

[Frédéric Buclin]

Attachment: patch, v2

This patch is even better: both new and obsolete parameters will be listed by checksetup.pl. This doesn't change the logic at all.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 550328 [details] [diff] [review]
patch, v2

Review of attachment 550328 [details] [diff] [review]:
-----------------------------------------------------------------

Hey, pretty clever! I didn't test this, but I'm sure that you did. :-) I do have one comment that is important to fix on checkin:

::: Bugzilla/Config.pm
@@ +207,1 @@
>                  $param->{$name} = $answer->{$name};

answers should override new param values.

Comment 5

[Frédéric Buclin]

(In reply to comment #4)
> answers should override new param values.

If we do that, we are in trouble for 2 reasons: first, %new_params is set only for new parameters whose value is based on old (or current) parameters. New parameters which do not depend on any old parameter are not in %new_params and so will get their value from $answers. With your proposal, it would be possible to interact with the conversion code, which is unexpected. Secondly, my patch keeps the current behavior, i.e. 1) conversion code, 2) answers file if not conversion code exists for the new parameter, 3) default value for this parameter if nothing else. With your proposal, update_params() would suddenly work differently, i.e. 2) first, then 1) and finally 3), which is not the goal of this patch.

Comment 6

[Max Kanat-Alexander]

Ah, I didn't realize it would be a behavioral change. That's fine, then.

Comment 7

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Config.pm
Committed revision 7883.