On Jan 24, 2008 10:35 AM, <huji(a)svn.wikimedia.org> wrote:
> + // Entry from drop down menu + additional comment
> + $reason .= ': ' . $this->DeleteReason;
Should the ': ' string be localized (below as well as here)? Also, as
far as I can tell this will result in summaries like:
Deleted old revision $1: Boilerplate reason: Custom reason
Having two colons is a bit odd.
> + $mDeletereasonother = Xml::label( wfMsg( 'filedelete-otherreason' ), 'wpReason' );
> + $mDeletereasonotherlist = wfMsgHtml( 'filedelete-reason-otherlist' );
> + $scDeleteReasonList = wfMsgForContent( 'filedelete-reason-dropdown' );
> + $mDeleteReasonList = '';
> + $delcom = Xml::label( wfMsg( 'filedelete-comment' ), 'wpDeleteReasonList' );
It seems incredibly confusing to use local variables whose names begin
with $m. An initial lowercase 'm' prefix is used to indicate member
variables. Either these should be made member variables, or the 'm'
should be dropped. You also have variables that are named identically
except for the 'm' ($deleteReasonList vs. $mDeleteReasonList), which
is even more confusing.
filedelete-comment and filedelete-otherreason seem to allow arbitrary HTML.
> + $value = trim( htmlspecialchars($option) );
Consider not escaping ampersands here, so that entities can be used --
really we only want to ban tags.
> + } elseif ( substr( $value, 0, 1) == '*' && substr( $value, 1, 1) != '*' ) {
> + // A new group is starting ...
> + $value = trim( substr( $value, 1 ) );
> + $deleteReasonList .= "$optgroup<optgroup label=\"$value\">";
> + $optgroup = "</optgroup>";
> + } elseif ( substr( $value, 0, 2) == '**' ) {
It would probably be simpler to read if you reversed these two
elseifs. Then you could drop the second part of the (current) first
one's condition, and just check substr( $value, 0, 1 ) == '*'.
Also, maybe a clearer name for $optgroup? Like $close or
$closeoptgroup or something? The current one is okay, I guess.
> + if ( $mDeleteReasonList === $value)
> + $selected = ' selected="selected"';
Indentation is wrong here. The second line should be indented by one more tab.
