143040 - Tidy up remove parameters message in checksetup.pl

Status: RESOLVED FIXED

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

Description

[David D. Kilzer (ddk)]

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.)

Comment 1

[David D. Kilzer (ddk)]

Attachment: Patch v.1

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.

Comment 2

[Gervase Markham [:gerv]]

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

Comment 3

[David D. Kilzer (ddk)]

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.

Comment 4

[David D. Kilzer (ddk)]

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.)

Comment 5

[Gervase Markham [:gerv]]

Taking.

Gerv

Comment 6

[Gervase Markham [:gerv]]

Attachment: Patch v.2

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

Comment 7

[David D. Kilzer (ddk)]

Comment on attachment 82821 [details] [diff] [review]
Patch v.2

r=ddk  Looks good!

Comment 8

[Dave Miller [:justdave]]

Comment on attachment 82821 [details] [diff] [review]
Patch v.2

r= justdave

Comment 9

[Gervase Markham [:gerv]]

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