r71613 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r71612‎ | r71613 | r71614 >
Date:01:41, 25 August 2010
Author:kaldari
Status:resolved (Comments)
Tags:
Comment:
cleaning up form tags, fixing error message, removing unneeded name check in removeTemplate
Modified paths:
  • /trunk/extensions/CentralNotice/SpecialCentralNotice.php (modified) (history)
  • /trunk/extensions/CentralNotice/SpecialNoticeTemplate.php (modified) (history)

Diff [purge]

Index: trunk/extensions/CentralNotice/SpecialCentralNotice.php
——@@ -48,7 +48,7 @@
4949
5050 $method = $wgRequest->getVal( 'method' );
5151 // Handle form sumissions
52 - if ( $this->editable && $wgRequest->wasPosted() ) {
 52+ if ( $this->editable && $wgRequest->wasPosted() ) {
5353
5454 // Check authentication token
5555 if ( $wgUser->matchEditToken( $wgRequest->getVal( 'authtoken' ) ) ) {
——@@ -411,12 +411,7 @@
412412 // If there are campaigns to show...
413413 if ( $dbr->numRows( $res ) >= 1 ) {
414414 if ( $this->editable ) {
415 - $htmlOut .= Xml::openElement( 'form',
416 - array(
417 - 'method' => 'post',
418 - 'action' => $this->getTitle()->getFullUrl()
419 - )
420 - );
 415+ $htmlOut .= Xml::openElement( 'form', array( 'method' => 'post' ) );
421416 }
422417 $htmlOut .= Xml::element( 'h2', null, wfMsg( 'centralnotice-manage' ) );
423418
——@@ -549,12 +544,7 @@
550545 $htmlOut .= Xml::openElement( 'fieldset', array( 'class' => 'prefsection' ) );
551546
552547 // Form for adding a campaign
553 - $htmlOut .= Xml::openElement( 'form',
554 - array(
555 - 'method' => 'post',
556 - 'action' => $this->getTitle()->getLocalUrl()
557 - )
558 - );
 548+ $htmlOut .= Xml::openElement( 'form', array( 'method' => 'post' ) );
559549 $htmlOut .= Xml::element( 'h2', null, wfMsg( 'centralnotice-add-notice' ) );
560550 $htmlOut .= Xml::hidden( 'title', $this->getTitle()->getPrefixedText() );
561551 $htmlOut .= Xml::hidden( 'method', 'addNotice' );
Index: trunk/extensions/CentralNotice/SpecialNoticeTemplate.php
——@@ -155,12 +155,7 @@
156156 $htmlOut .= Xml::element( 'p', null, wfMsg( 'centralnotice-no-templates' ) );
157157 } else {
158158 if ( $this->editable ) {
159 - $htmlOut .= Xml::openElement( 'form',
160 - array(
161 - 'method' => 'post',
162 - 'action' => ''
163 - )
164 - );
 159+ $htmlOut .= Xml::openElement( 'form', array( 'method' => 'post' ) );
165160 }
166161 $htmlOut .= Xml::element( 'h2', null, wfMsg( 'centralnotice-manage-templates' ) );
167162
——@@ -562,19 +557,12 @@
563558 private function removeTemplate ( $name ) {
564559 global $wgOut;
565560
566 - // FIXME: weak comparison
567 - if ( $name == '' ) {
568 - // FIXME: message not defined?
569 - $wgOut->addWikiMsg( 'centralnotice-template-doesnt-exist' );
570 - return;
571 - }
572 -
573561 $id = $this->getTemplateId( $name );
574562 $dbr = wfGetDB( DB_SLAVE );
575563 $res = $dbr->select( 'cn_assignments', 'asn_id', array( 'tmp_id' => $id ), __METHOD__ );
576564
577565 if ( $dbr->numRows( $res ) > 0 ) {
578 - $wgOut->addWikiMsg( 'centralnotice-template-still-bound' );
 566+ $wgOut->addHTML( Xml::element( 'div', array( 'class' => 'cn-error' ), wfMsg( 'centralnotice-template-still-bound' ) ) );
579567 return;
580568 } else {
581569 $dbw = wfGetDB( DB_MASTER );

Follow-up revisions

RevisionCommit summaryAuthorDate
r71639checking for bad notice names per r71613kaldari18:09, 25 August 2010
r71643fixing error messages per comment at r71613kaldari18:41, 25 August 2010

Comments

#Comment by Nikerabbit (talk | contribs)   06:28, 25 August 2010 
 - $wgOut->addHTML( Xml::element( 'div', array( 'class' => 'cn-error' ), wfMsg( 'centralnotice-template-still-bound' ) ) );
 + $wgOut->wrapWikiMsg( Html::element( 'div', array( 'class' => 'cn-error' ), "\n$1\n" ), 'centralnotice-template-still-bound' ) );
 + $wgOut->wrapWikiMsg( "<div class='cn-error'>\n$1\n</div>", 'centralnotice-template-still-bound' ) );

Pick your choice :)

#Comment by Kaldari (talk | contribs)   18:42, 25 August 2010

fixed in r71643.

#Comment by Catrope (talk | contribs)   16:14, 25 August 2010 
-		// FIXME: weak comparison
-		if ( $name == '' ) {
-			// FIXME: message not defined?
-			$wgOut->addWikiMsg( 'centralnotice-template-doesnt-exist' );
-			return;
-		}

Although you're right that this check is ridiculous and has to die, there should be some check against invalid notice names. Currently, getNoticeId() doesn't use selectRow() while it should and by the looks of it completely freaks out (as in fatal error) when passed a nonexistent notice name.

#Comment by Kaldari (talk | contribs)   18:09, 25 August 2010

fixed in r71639.

Status & tagging log