Thanks for pointing them out. I'm going to update this and other reason dropdowns, so they would use a function to generate their dropdown rather a similar code pasted in different places. I will also make the code cleaner, as you mentioned.
Cheers,
Huji
On 1/25/08, Simetrical Simetrical+wikilist@gmail.com wrote:
On Jan 24, 2008 10:35 AM, huji@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.
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org http://lists.wikimedia.org/mailman/listinfo/wikitech-l