r73028 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r73027‎ | r73028 | r73029 >
Date:23:22, 14 September 2010
Author:kaldari
Status:resolved (Comments)
Tags:
Comment:
trying to fix r72954
Modified paths:
  • /trunk/extensions/CentralNotice/SpecialCentralNotice.php (modified) (history)

Diff [purge]

Index: trunk/extensions/CentralNotice/SpecialCentralNotice.php
——@@ -1006,40 +1006,82 @@
10071007 * @return A 2D array of running banners with associated weights and settings
10081008 */
10091009 static function selectNoticeTemplates( $project, $language, $location = null ) {
1010 - $location = htmlspecialchars( $location );
 1010+
 1011+ // Normalize location parameter (should be an uppercase 2-letter country code)
 1012+ preg_match( '/[a-zA-Z][a-zA-Z]/', $location, $matches );
 1013+ $location = strtoupper( $matches[0] );
 1014+
10111015 $dbr = wfGetDB( DB_SLAVE );
10121016 $encTimestamp = $dbr->addQuotes( $dbr->timestamp() );
1013 - $res = $dbr->select(
1014 - array(
1015 - 'cn_notices',
1016 - 'cn_notice_languages',
1017 - 'cn_notice_countries',
1018 - 'cn_assignments',
1019 - 'cn_templates'
1020 - ),
1021 - array(
1022 - 'tmp_name',
1023 - 'SUM(tmp_weight) AS total_weight',
1024 - 'tmp_display_anon',
1025 - 'tmp_display_account'
1026 - ),
1027 - array (
1028 - "not_start <= $encTimestamp",
1029 - "not_end >= $encTimestamp",
1030 - 'not_enabled = 1',
1031 - 'nc_notice_id = cn_notices.not_id',
1032 - "(not_geo = 0) OR ((not_geo = 1) AND (nc_country = '$location'))",
1033 - 'nl_notice_id = cn_notices.not_id',
1034 - 'nl_language' => $language,
1035 - 'not_project' => array( '', $project ),
1036 - 'cn_notices.not_id=cn_assignments.not_id',
1037 - 'cn_assignments.tmp_id=cn_templates.tmp_id'
1038 - ),
1039 - __METHOD__,
1040 - array(
1041 - 'GROUP BY' => 'tmp_name'
1042 - )
1043 - );
 1017+ if ( $location ) {
 1018+ $res = $dbr->select(
 1019+ array(
 1020+ 'cn_notices',
 1021+ 'cn_notice_languages',
 1022+ 'cn_notice_countries',
 1023+ 'cn_assignments',
 1024+ 'cn_templates'
 1025+ ),
 1026+ array(
 1027+ 'tmp_name',
 1028+ 'SUM(tmp_weight) AS total_weight',
 1029+ 'tmp_display_anon',
 1030+ 'tmp_display_account'
 1031+ ),
 1032+ array (
 1033+ "not_start <= $encTimestamp",
 1034+ "not_end >= $encTimestamp",
 1035+ 'not_enabled = 1',
 1036+ 'nc_notice_id = cn_notices.not_id',
 1037+ "(not_geo = 0) OR ((not_geo = 1) AND (nc_country = '$location'))", // not geotargeted or (geotargeted and matches location)
 1038+ 'nl_notice_id = cn_notices.not_id',
 1039+ 'nl_language' => $language,
 1040+ 'not_project' => array( '', $project ),
 1041+ 'cn_notices.not_id=cn_assignments.not_id',
 1042+ 'cn_assignments.tmp_id=cn_templates.tmp_id'
 1043+ ),
 1044+ __METHOD__,
 1045+ array(
 1046+ 'GROUP BY' => 'tmp_name'
 1047+ ),
 1048+ array(
 1049+ 'cn_notice_countries' => array(
 1050+ 'LEFT JOIN',
 1051+ "nc_country = '$location'"
 1052+ )
 1053+ )
 1054+ );
 1055+ } else {
 1056+ $res = $dbr->select(
 1057+ array(
 1058+ 'cn_notices',
 1059+ 'cn_notice_languages',
 1060+ 'cn_assignments',
 1061+ 'cn_templates'
 1062+ ),
 1063+ array(
 1064+ 'tmp_name',
 1065+ 'SUM(tmp_weight) AS total_weight',
 1066+ 'tmp_display_anon',
 1067+ 'tmp_display_account'
 1068+ ),
 1069+ array (
 1070+ "not_start <= $encTimestamp",
 1071+ "not_end >= $encTimestamp",
 1072+ 'not_enabled = 1', // enabled
 1073+ 'not_geo = 0', // not geotargeted
 1074+ 'nl_notice_id = cn_notices.not_id',
 1075+ 'nl_language' => $language,
 1076+ 'not_project' => array( '', $project ),
 1077+ 'cn_notices.not_id=cn_assignments.not_id',
 1078+ 'cn_assignments.tmp_id=cn_templates.tmp_id'
 1079+ ),
 1080+ __METHOD__,
 1081+ array(
 1082+ 'GROUP BY' => 'tmp_name'
 1083+ )
 1084+ );
 1085+ }
10441086 $templates = array();
10451087 foreach ( $res as $row ) {
10461088 $template = array();

Follow-up revisions

RevisionCommit summaryAuthorDate
r73818handle case of $matches being empty per comment at r73028kaldari18:14, 27 September 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r72954add geotargeting to banner selection codekaldari02:30, 14 September 2010

Comments

#Comment by Kaldari (talk | contribs)   00:37, 15 September 2010

Hmm, this doesn't work.

#Comment by Kaldari (talk | contribs)   01:51, 15 September 2010

I think I've fixed the issues in r73044.

#Comment by Catrope (talk | contribs)   17:41, 27 September 2010 
+		preg_match( '/[a-zA-Z][a-zA-Z]/', $location, $matches );
+		$location = strtoupper( $matches[0] );

If $location doesn't match /[a-zA-Z]{2}/ (equivalent regex) at all, this'll set $matches to an empty array and issue a notice when you try to access $matches[0].

The queries in the if and else blocks have a lot in common. You should factor out the common parts into four variables ($tables, $fields, $conds, $join_conds), conditionally modify those variables, then call $dbr->select().

I also don't like the fact that this code doesn't escape $location. I know it's not necessary because of the validation above, but one of our guidelines is that code should be demonstrably and obviously secure. If you escape $location, any reader will instantly be able to tell that it's safe without having to think and figure it out for themselves.

#Comment by Catrope (talk | contribs)   17:43, 27 September 2010

The second point is moot as of r73044

#Comment by Kaldari (talk | contribs)   18:20, 27 September 2010

remaining issue fixed in r73818.

Status & tagging log