r71382 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r71381‎ | r71382 | r71383 >
Date:22:34, 20 August 2010
Author:kaldari
Status:resolved (Comments)
Tags:
Comment:
fixing table field names per r68666, fixing inserts and deletes per r68580, removing unused showAll
Modified paths:
  • /trunk/extensions/CentralNotice/CentralNotice.db.php (modified) (history)
  • /trunk/extensions/CentralNotice/CentralNotice.sql (modified) (history)
  • /trunk/extensions/CentralNotice/SpecialCentralNotice.php (modified) (history)
  • /trunk/extensions/CentralNotice/patches/patch-notice_languages.sql (modified) (history)

Diff [purge]

Index: trunk/extensions/CentralNotice/SpecialCentralNotice.php
——@@ -192,7 +192,6 @@
193193 }
194194
195195 // Handle adding of campaign
196 - $this->showAll = $wgRequest->getVal( 'showAll' );
197196 if ( $this->editable && $method == 'addNotice' && $wgUser->matchEditToken( $wgRequest->getVal( 'authtoken' ) ) ) {
198197 $noticeName = $wgRequest->getVal( 'noticeName' );
199198 $start = $wgRequest->getArray( 'start' );
——@@ -377,43 +376,21 @@
378377 $readonly = array( 'disabled' => 'disabled' );
379378 }
380379
381 - // This is temporarily hard-coded
382 - $this->showAll = 'Y';
383 -
384 - // If all languages should be shown
385 - if ( isset( $this->showAll ) ) {
386 - // Get campaigns for all languages
387 - $res = $dbr->select( 'cn_notices',
388 - array(
389 - 'not_name',
390 - 'not_start',
391 - 'not_end',
392 - 'not_enabled',
393 - 'not_preferred',
394 - 'not_project',
395 - 'not_locked'
396 - ),
397 - null,
398 - __METHOD__,
399 - array( 'ORDER BY' => 'not_id' )
400 - );
401 - } else {
402 - // Get only campaigns for this language
403 - $res = $dbr->select( 'cn_notices',
404 - array(
405 - 'not_name',
406 - 'not_start',
407 - 'not_end',
408 - 'not_enabled',
409 - 'not_preferred',
410 - 'not_project',
411 - 'not_locked'
412 - ),
413 - array ( 'not_language' => $wgLang->getCode() ),
414 - __METHOD__,
415 - array( 'ORDER BY' => 'not_id' )
416 - );
417 - }
 380+ // Get all campaigns from the database
 381+ $res = $dbr->select( 'cn_notices',
 382+ array(
 383+ 'not_name',
 384+ 'not_start',
 385+ 'not_end',
 386+ 'not_enabled',
 387+ 'not_preferred',
 388+ 'not_project',
 389+ 'not_locked'
 390+ ),
 391+ null,
 392+ __METHOD__,
 393+ array( 'ORDER BY' => 'not_id' )
 394+ );
418395
419396 // Begin building HTML
420397 $htmlOut = '';
——@@ -669,9 +646,6 @@
670647 );
671648 }
672649
673 - // Temporarily hard coded
674 - $this->showAll = 'Y';
675 -
676650 $output_detail = $this->noticeDetailForm( $notice );
677651 $output_assigned = $this->assignedTemplatesForm( $notice );
678652 $output_templates = $this->addTemplatesForm( $notice );
——@@ -747,13 +721,13 @@
748722 __METHOD__
749723 );
750724 $res = $dbr->select( 'cn_notice_languages',
751 - 'not_language',
752 - array( 'not_id' => $row->not_id ),
 725+ 'nl_language',
 726+ array( 'nl_notice_id' => $row->not_id ),
753727 __METHOD__
754728 );
755729 $project_languages = array();
756730 foreach ( $res as $langRow ) {
757 - $project_languages[] = $langRow->not_language;
 731+ $project_languages[] = $langRow->nl_language;
758732 }
759733
760734 if ( $row ) {
——@@ -972,8 +946,8 @@
973947 "not_start <= $encTimestamp",
974948 "not_end >= $encTimestamp",
975949 "not_enabled = 1",
976 - 'cn_notice_languages.not_id = cn_notices.not_id',
977 - 'cn_notice_languages.not_language' => $language,
 950+ 'nl_notice_id = cn_notices.not_id',
 951+ 'nl_language' => $language,
978952 "not_project" => array( '', $project ),
979953 'cn_notices.not_id=cn_assignments.not_id',
980954 'cn_assignments.tmp_id=cn_templates.tmp_id'
——@@ -1033,13 +1007,14 @@
10341008 )
10351009 );
10361010 $not_id = $dbw->insertId();
 1011+
 1012+ // Do multi-row insert for campaign languages
 1013+ $insertArray = array();
10371014 foreach( $project_languages as $code ) {
1038 - $res = $dbw->insert( 'cn_notice_languages',
1039 - array( 'not_id' => $not_id,
1040 - 'not_language' => $code
1041 - )
1042 - );
 1015+ $insertArray[] = array( 'nl_notice_id' => $not_id, 'nl_language' => $code );
10431016 }
 1017+ $res = $dbw->insert( 'cn_notice_languages', $insertArray, __METHOD__, array( 'IGNORE' ) );
 1018+
10441019 $dbw->commit();
10451020 return;
10461021 }
——@@ -1066,7 +1041,7 @@
10671042 $noticeId = htmlspecialchars( $this->getNoticeId( $noticeName ) );
10681043 $res = $dbw->delete( 'cn_assignments', array ( 'not_id' => $noticeId ) );
10691044 $res = $dbw->delete( 'cn_notices', array ( 'not_name' => $noticeName ) );
1070 - $res = $dbw->delete( 'cn_notice_languages', array ( 'not_id' => $noticeId ) );
 1045+ $res = $dbw->delete( 'cn_notice_languages', array ( 'nl_notice_id' => $noticeId ) );
10711046 $dbw->commit();
10721047 return;
10731048 }
——@@ -1111,14 +1086,16 @@
11121087 return $row->not_id;
11131088 }
11141089
1115 - function getNoticeLanguages ( $noticeName ) {
 1090+ function getNoticeLanguages( $noticeName ) {
11161091 $dbr = wfGetDB( DB_SLAVE );
11171092 $eNoticeName = htmlspecialchars( $noticeName );
11181093 $row = $dbr->selectRow( 'cn_notices', 'not_id', array( 'not_name' => $eNoticeName ) );
1119 - $res = $dbr->select( 'cn_notice_languages', 'not_language', array( 'not_id' => $row->not_id ) );
11201094 $languages = array();
1121 - foreach ( $res as $langRow ) {
1122 - $languages[] = $langRow->not_language;
 1095+ if ( $dbr->numRows( $row ) > 0 ) {
 1096+ $res = $dbr->select( 'cn_notice_languages', 'nl_language', array( 'nl_notice_id' => $row->not_id ) );
 1097+ foreach ( $res as $langRow ) {
 1098+ $languages[] = $langRow->nl_language;
 1099+ }
11231100 }
11241101 return $languages;
11251102 }
——@@ -1314,18 +1291,17 @@
13151292 $addLanguages = array_diff( $newLanguages, $oldLanguages );
13161293 $insertArray = array();
13171294 foreach( $addLanguages as $code ) {
1318 - $insertArray[] = array( 'not_id' => $row->not_id, 'not_language' => $code );
 1295+ $insertArray[] = array( 'nl_notice_id' => $row->not_id, 'nl_language' => $code );
13191296 }
13201297 $res = $dbw->insert( 'cn_notice_languages', $insertArray, __METHOD__, array( 'IGNORE' ) );
13211298
13221299 // Remove disassociated languages
13231300 $removeLanguages = array_diff( $oldLanguages, $newLanguages );
1324 - foreach( $removeLanguages as $code ) {
 1301+ if ( !empty( $removeLanguages ) ) {
13251302 $res = $dbw->delete( 'cn_notice_languages',
1326 - array( 'not_id' => $row->not_id, 'not_language' => $code )
 1303+ array( 'nl_notice_id' => $row->not_id, 'nl_language' => $removeLanguages )
13271304 );
13281305 }
1329 -
13301306 $dbw->commit();
13311307 }
13321308
Index: trunk/extensions/CentralNotice/patches/patch-notice_languages.sql
——@@ -1,8 +1,8 @@
22 -- Update to allow for any number of languages per notice.
33
44 CREATE TABLE IF NOT EXISTS /*$wgDBprefix*/cn_notice_languages (
5 - `id` int unsigned NOT NULL PRIMARY KEY auto_increment,
6 - `not_id` int unsigned NOT NULL,
7 - `not_language` varchar(32) NOT NULL
 5+ `nl_id` int unsigned NOT NULL PRIMARY KEY auto_increment,
 6+ `nl_notice_id` int unsigned NOT NULL,
 7+ `nl_language` varchar(32) NOT NULL
88 ) /*$wgDBTableOptions*/;
9 -CREATE UNIQUE INDEX /*i*/cn_not_id_not_language ON /*$wgDBprefix*/cn_notice_languages (not_id, not_language);
 9+CREATE UNIQUE INDEX /*i*/nl_notice_id_language ON /*$wgDBprefix*/cn_notice_languages (nl_notice_id, nl_language);
Index: trunk/extensions/CentralNotice/CentralNotice.sql
——@@ -25,8 +25,8 @@
2626 ) /*$wgDBTableOptions*/;
2727
2828 CREATE TABLE IF NOT EXISTS /*$wgDBprefix*/cn_notice_languages (
29 - `id` int unsigned NOT NULL PRIMARY KEY auto_increment,
30 - `not_id` int unsigned NOT NULL,
31 - `not_language` varchar(32) NOT NULL
 29+ `nl_id` int unsigned NOT NULL PRIMARY KEY auto_increment,
 30+ `nl_notice_id` int unsigned NOT NULL,
 31+ `nl_language` varchar(32) NOT NULL
3232 ) /*$wgDBTableOptions*/;
33 -CREATE UNIQUE INDEX /*i*/cn_not_id_not_language ON /*$wgDBprefix*/cn_notice_languages (not_id, not_language);
 33+CREATE UNIQUE INDEX /*i*/nl_notice_id_language ON /*$wgDBprefix*/cn_notice_languages (nl_notice_id, nl_language);
Index: trunk/extensions/CentralNotice/CentralNotice.db.php
——@@ -32,8 +32,8 @@
3333 $conds[] = "not_project =" . $dbr->addQuotes( $project );
3434 }
3535 if ( $language ) {
36 - $conds[] = "cn_notice_languages.not_id = cn_notices.not_id";
37 - $conds[] = "cn_notice_languages.not_language =" . $dbr->addQuotes( $language );
 36+ $conds[] = "nl_notice_id = cn_notices.not_id";
 37+ $conds[] = "nl_language =" . $dbr->addQuotes( $language );
3838 }
3939 if ( $preferred ) {
4040 $conds[] = "not_preferred = 1";

Follow-up revisions

RevisionCommit summaryAuthorDate
r71496some fixes to r71382kaldari18:18, 23 August 2010
r71499axing nl_id field for languages table per r71382 and r68666kaldari19:03, 23 August 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r68580CentralNotice: adding support for multiple languages per notice, Bug 20229kaldari18:48, 25 June 2010
r68666updating mySQL set-up script for multi-language support (bug 20229), deleting...kaldari17:01, 28 June 2010

Comments

#Comment by Tim Starling (talk | contribs)   03:16, 21 August 2010 
+		if ( $dbr->numRows( $row ) > 0 ) {

Plainly wrong, should be !$row, like in the pre-existing code in noticeDetailForm(). Database::numRows() takes a result object as its parameter, not a row object. Speaking of noticeDetailForm(), you didn't fix the same error there. 

-		foreach( $removeLanguages as $code ) {
+		if ( !empty( $removeLanguages ) ) {

Inappropriate use of empty(), see Manual:Coding conventions#PHP pitfalls.

I asked on CR r68666 what the id field is for, you didn't answer. I'll now modify my question: what is the nl_id field for? I suspect it's useless and needs to be removed. Removing it will make PostgreSQL and SQLite support for this feature slightly easier to implement.

#Comment by Kaldari (talk | contribs)   19:17, 23 August 2010

Now I'm just getting sloppy :P Should be fixed in r71496 and r71499. You're correct that the id field was useless. It was just added out of habit.

Status & tagging log