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(a)gmail.com> wrote:
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.
_______________________________________________
Wikitech-l mailing list
Wikitech-l(a)lists.wikimedia.org
http://lists.wikimedia.org/mailman/listinfo/wikitech-l