r68580 MediaWiki - Code Review archive

Forgot to localize "Select: " in the multi-language selection interface in SpecialCentralNotice.php. Fixed in r70529.

Eh forgot a few more. Fixed in r70531.

This commit would have been a less confusing read if you had separated the migration from array-based to Xml:: function-based form/table building and various indentation changes out into separate commits (so that's 3 commits: multilang feature, form building migration and indent fixing). Please do separate unrelated changes like these in the future.

I see unlocalized strings like "multiple" and "(GMT)". The latter can probably just be folded into the message preceding it.

-			array( 'ORDER BY' => 'not_id' )

Are you sure you meant to drop the ordering by ID? If so, why?

+				"cn_notice_languages.not_language = '$language'",

Use array( 'cn_notice_languages.not_language' => $language ) so $language will be escaped. This is a potential security vulnerability (XSS).

+			$options .= Xml::option( "$code - $name", $code, in_array( $code, $selected ) ) . "\n";

Even trivial-looking things like "$code - $name" should be made localizable: other languages may not want to use a dash, may want to swap the order, or what have you.

+		$htmlOut = "
+<script type=\"text/javascript\">\n
+function selectLanguages(selectAll) {\n
+	var selectBox = document.getElementById(\"project_languages[]\");\n
+	var firstSelect = selectBox.options.length - 1;\n
+	for (var i = firstSelect; i >= 0; i--) {\n
+		selectBox.options[i].selected = selectAll;\n
+	}\n
+}\n
+function top10Languages() {\n
+	var selectBox = document.getElementById(\"project_languages[]\");\n
+	var top10 = new Array('en','de','fr','it','pt','ja','es','pl','ru','nl');\n
+	for (var i = 0; i < selectBox.options.length; i++) {\n
+		var lang = selectBox.options[i].value;\n
+		if (top10.toString().indexOf(lang)!==-1) {\n
+			selectBox.options[i].selected = true;\n
+		}\n
+	}\n
+}\n
+</script>";

You could make this look less ugly by using heredoc syntax.

+		foreach( $addLanguages as $code ) {
+			$res = $dbw->insert( 'cn_notice_languages',
+				array( 'not_id' => $row->not_id, 'not_language' => $code )
+			);
+		}

You should use a multi-row insert() call by passing in an array of rows instead of just one row. This will do a multi-row insert on DBMSes that support it.

Issues I noticed that were not introduced in this commit:

+			$htmlOut .= Xml::tags( 'td', array(), Xml::inputLabel( '', 'noticeName',  'noticeName', 25 ) );

So you want an <input> with a <label>, but want the label to be empty? Don't you just want to use Xml::input() here? Looks like this may have been present in the original

+			$htmlOut .= Xml::tags( 'td', array(), $this->timeSelector( 'start', $row->not_start, "[$row->not_name]" ) );

ID-based indexing is safer. You can't rely on notice names not containing evil characters that mess this up, such as ].

+		$eNoticeName = htmlspecialchars( $noticeName );
+		$row = $dbr->selectRow( 'cn_notices', 'not_id', array( 'not_name' => $eNoticeName ) );

Why the heck is this code storing HTML-escaped notice names in the database? This is considered bad style, and I think the Xml:: functions will just escape them a second time, creating double-escaping bugs.

Should be fixed now. See my comment below.

The '$language' thing was SQL injection, not XSS.

Thanks for the detailed feedback. It was very helpful!

The following issues have been fixed in r70901:

• Unlocalized strings localized
• $language is escaped
• switched to doing a multi-row insert() for the languages
• inputLabel switched to input

The Javascript code was moved to an included js file in r70609.

Regarding why I dropped the ordering by ID: It doesn't make sense to order the results of a selectRow() since the function only returns a single row.

Regarding the last two comments: I have no idea why these were done this way, but I'm reluctant to change them until I have time to review the existing code more deeply.

updateProjectLanguages() now uses a multi-row insert, but addNotice() still needs fixing. Also, updateProjectLanguages() uses multiple delete queries to delete the rows, it should use a multi-row delete instead:

$dbw->delete( 'cn_notice_languages', 
   array( 'not_id' => $row->not_id, 'not_language' => $removeLanguages ),
   __METHOD__ );

getNoticeLanguages() and noticeDetailForm() should fail gracefully when the cn_notices row is missing, instead of spewing errors and making unintended database queries.

All the fields in the cn_notice_languages table have to be renamed, as I said on r68666.

All issues should be fixed in r71382.