Closed Bug 676200 Opened 13 years ago Closed 13 years ago

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

Categories

(Bugzilla :: Installation & Upgrading, defect)

4.1.2
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.2

People

(Reporter: LpSolit, Assigned: LpSolit)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch, v1 (obsolete) — Splinter Review
Assignee: installation → LpSolit
Status: NEW → ASSIGNED
Attachment #550323 - Flags: review?(mkanat)
Let's take it for 4.2. It's important for admins to know where parameters are going.
Target Milestone: Bugzilla 5.0 → Bugzilla 4.2
Attached patch patch, v2Splinter Review
This patch is even better: both new and obsolete parameters will be listed by checksetup.pl. This doesn't change the logic at all.
Attachment #550323 - Attachment is obsolete: true
Attachment #550323 - Flags: review?(mkanat)
Attachment #550328 - Flags: review?(mkanat)
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.
Attachment #550328 - Flags: review?(mkanat) → review+
(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.
Ah, I didn't realize it would be a behavioral change. That's fine, then.
Flags: approval+
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/ modified Bugzilla/Config.pm Committed revision 7883.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: