Closed Bug 143040 Opened 21 years ago Closed 21 years ago

Tidy up remove parameters message in checksetup.pl

Categories

(Bugzilla :: Installation & Upgrading, defect, P1)

2.15
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: ddkilzer, Assigned: gerv)

Details

Attachments

(1 file, 1 obsolete file)

The message that is printed out when checksetup.pl removes
unused parameters from data/params has way too many spaces
for its indented lines.  Patch to follow shortly.

N.B. The WriteParams() method in defparams.pl already
removes unused parameters itself, but it prints out HTML
(use of <em></em> tags) and does not save the old parameters
to the old-params.txt file.  If there is an acceptable way
to determine when WriteParams() is being called from
checksetup.pl versus a CGI script (check if 
$0 =~ m/checksetup.pl$/ ?), then checksetup.pl could be
cleaned up to simply call WriteParams() after require-ing
the appropriate files, and the old-params.txt behavior
could be added to WriteParams() in defparams.pl.

Any thoughts on this?  Is there a better way to print out
the "removed from params file" text through the header
template?  (I'll go look.)
Status: NEW → ASSIGNED
Keywords: patch, review
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.16
Attached patch Patch v.1 (obsolete) — Splinter Review
Fixes the extra whitespace on lines 2 and 3 of the
'parameter removed' text.  Also tidies up the output
to the old-params.txt file.
No, no, this is all wrong!

...not you; the patch I checked in. :-| I changed it to print just one message,
and a list of the params removed, but these changes appear to have got lost. I
really messed this one up :-(

I'll work out what's gone wrong, and produce a proper patch (incorporating your
stuff.) Is that OK?

Gerv
Gerv, please feel free to make those changes and reassign the
bug to yourself.  I would also recommend looking at
incorporating the old-params.txt stuff into  WriteParams() in
defparams.pl, and perhaps using the 'message' parameter in
WriteParams() for the additional HTML rather than simply
printing it straight out.
Of course, until _all_ CGI scripts are templatised, using the
'message' parameter with the template object won't do anything
for non-templatised CGI scripts.  (Don't mind me...just thinking
out loud.)
Taking.

Gerv
Assignee: ddkilzer → gerv
Status: ASSIGNED → NEW
Attached patch Patch v.2Splinter Review
OK. This is how it _should_ be done. I've removed the code to remove parameters
in WriteParams() completely; people should run checksetup after upgrading, and
the printout wasn't localisable anyway. If people fail to run checksetup, it's
no big deal - they just have params which don't do anything.

Gerv
Attachment #82809 - Attachment is obsolete: true
Comment on attachment 82821 [details] [diff] [review]
Patch v.2

r=ddk  Looks good!
Attachment #82821 - Flags: review+
Comment on attachment 82821 [details] [diff] [review]
Patch v.2

r= justdave
Attachment #82821 - Flags: review+
Fixed.

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.149; previous revision: 1.148
done
Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.73; previous revision: 1.72
done

Gerv
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.