r51600 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r51599‎ | r51600 | r51601 >
Date:17:58, 8 June 2009
Author:jan
Status:deferred (Comments)
Tags:
Comment:
Fixes for r51338:
* Remove isset($_POST['controll_delete'])
* Replace $_POST with $wgRequest->getVal
Modified paths:
  • /trunk/extensions/Poll/Poll_body.php (modified) (history)

Diff [purge]

Index: trunk/extensions/Poll/Poll_body.php
——@@ -103,6 +103,7 @@
104104 $wgOut->addHtml( '<tr><td>'.wfMsg( 'poll-alternative' ).' 5:</td><td>'.Xml::input('poll_alternative_5').'</td></tr>' );
105105 $wgOut->addHtml( '<tr><td>'.wfMsg( 'poll-alternative' ).' 6:</td><td>'.Xml::input('poll_alternative_6').'</td></tr>' );
106106 $wgOut->addHtml( '<tr><td>'.wfMsg( 'poll-dis' ).':</td><td>'.Xml::textarea('dis', '').'</td></tr>' );
 107+ $wgOut->addHtml( '<tr><td>'.Xml::check('allow_more').' 6:</td><td>'.wfMsg( 'poll-create-allow-more' ).'</td></tr>' );
107108 $wgOut->addHtml( '<tr><td>'.Xml::submitButton(wfMsg( 'poll-submit' )).''.Xml::hidden('type', 'create').'</td></tr>' );
108109 $wgOut->addHtml( Xml::closeElement( 'table' ) );
109110 $wgOut->addHtml( Xml::closeElement( 'form' ) );
——@@ -195,8 +196,7 @@
196197 $query_num_6 = $dbr->numRows( $query_6 );
197198
198199 $wgOut->addHtml( Xml::openElement( 'table' ) );
199 - $wgOut->addHtml( '<tr><th><center>'.$question.'</center></th></tr>' );
200 - //$wgOut->addHtml( '<tr><td><b>'.wfMsg( 'poll-alternative' ).'</b></td><td><b>'.wfMsg( 'poll-number-poll' ).'</td></tr>' );
 200+ $wgOut->addHtml( '<tr><th><center>'.$question.'</center></th></tr>' );;
201201 $wgOut->addHtml( '<tr><td>'.$alternative_1.'</td><td>'.$query_num_1.'</td></tr>' );
202202 $wgOut->addHtml( '<tr><td>'.$alternative_2.'</td><td>'.$query_num_2.'</td></tr>' );
203203 if($alternative_3 != "") { $wgOut->addHtml( '<tr><td>'.$alternative_3.'</td><td>'.$query_num_3.'</td></tr>' ); }
——@@ -277,7 +277,7 @@
278278 public function submit( $pid ) {
279279 global $wgRequest, $wgOut, $wgUser, $wgTitle;
280280
281 - $type = $_POST['type'];
 281+ $type = $wgRequest->getVal('type');
282282
283283 if($type == 'create') {
284284 $controll_create_right = $wgUser->isAllowed( 'poll-create' );
——@@ -293,14 +293,14 @@
294294
295295 else {
296296 $dbw = wfGetDB( DB_MASTER );
297 - $question = $_POST['question'];
298 - $alternative_1 = $_POST['poll_alternative_1'];
299 - $alternative_2 = $_POST['poll_alternative_2'];
300 - $alternative_3 = ($_POST['poll_alternative_3'] != "")? $_POST['poll_alternative_3'] : "";
301 - $alternative_4 = ($_POST['poll_alternative_4'] != "")? $_POST['poll_alternative_4'] : "";
302 - $alternative_5 = ($_POST['poll_alternative_5'] != "")? $_POST['poll_alternative_5'] : "";
303 - $alternative_6 = ($_POST['poll_alternative_6'] != "")? $_POST['poll_alternative_6'] : "";
304 - $dis = ($_POST['dis'] != "")? $_POST['dis'] : wfMsg('poll-no-dis');
 297+ $question = $wgRequest->getVal('question');
 298+ $alternative_1 = $wgRequest->getVal('poll_alternative_1');
 299+ $alternative_2 = $wgRequest->getVal('poll_alternative_2');
 300+ $alternative_3 = ($wgRequest->getVal('poll_alternative_3') != "")? $wgRequest->getVal('poll_alternative_3') : "";
 301+ $alternative_4 = ($wgRequest->getVal('poll_alternative_4') != "")? $wgRequest->getVal('poll_alternative_4') : "";
 302+ $alternative_5 = ($wgRequest->getVal('poll_alternative_5') != "")? $wgRequest->getVal('poll_alternative_5') : "";
 303+ $alternative_6 = ($wgRequest->getVal('poll_alternative_6') != "")? $wgRequest->getVal('poll_alternative_6') : "";
 304+ $dis = ($wgRequest->getVal('dis') != "")? $wgRequest->getVal( 'dis' ) : wfMsg('poll-no-dis');
305305 $user = $wgUser->getName();
306306
307307 if($question != "" && $alternative_1 != "" && $alternative_2 != "") {
——@@ -337,7 +337,7 @@
338338 else {
339339 $dbw = wfGetDB( DB_MASTER );
340340 $dbr = wfGetDB( DB_SLAVE );
341 - $vote = $_POST['vote'];
 341+ $vote = $wgRequest->getVal('vote');
342342 $user = $wgUser->getName();
343343 $uid = $wgUser->getId();
344344
——@@ -382,21 +382,21 @@
383383 $wgOut->addHtml( '<a href="'.$wgTitle->getFullURL('action=list').'">'.wfMsg('poll-back').'</a>' );
384384 }
385385
386 - if ( isset($controll_delete_blocked) AND ( $controll_change_blocked == true ) ) {
 386+ if ( $controll_change_blocked == true ) {
387387 $wgOut->addWikiMsg( 'poll-change-block-error' );
388388 $wgOut->addHtml( '<a href="'.$wgTitle->getFullURL('action=list').'">'.wfMsg('poll-back').'</a>' );
389389 }
390390
391391 if ( ( ( $creater == $user ) OR ( $controll_change_right == true ) ) AND ( $controll_change_blocked != true ) ) {
392392 $dbw = wfGetDB( DB_MASTER );
393 - $question = $_POST['question'];
394 - $alternative_1 = $_POST['poll_alternative_1'];
395 - $alternative_2 = $_POST['poll_alternative_2'];
396 - $alternative_3 = ($_POST['poll_alternative_3'] != "")? $_POST['poll_alternative_3'] : "";
397 - $alternative_4 = ($_POST['poll_alternative_4'] != "")? $_POST['poll_alternative_4'] : "";
398 - $alternative_5 = ($_POST['poll_alternative_5'] != "")? $_POST['poll_alternative_5'] : "";
399 - $alternative_6 = ($_POST['poll_alternative_6'] != "")? $_POST['poll_alternative_6'] : "";
400 - $dis = ($_POST['dis'] != "")? $_POST['dis'] : wfMsg('poll-no-dis');
 393+ $question = $wgRequest->getVal('question');
 394+ $alternative_1 = $wgRequest->getVal('poll_alternative_1');
 395+ $alternative_2 = $wgRequest->getVal('poll_alternative_2');
 396+ $alternative_3 = ($wgRequest->getVal('poll_alternative_3') != "")? $wgRequest->getVal('poll_alternative_3') : "";
 397+ $alternative_4 = ($wgRequest->getVal('poll_alternative_4') != "")? $wgRequest->getVal('poll_alternative_4') : "";
 398+ $alternative_5 = ($wgRequest->getVal('poll_alternative_5') != "")? $wgRequest->getVal('poll_alternative_5') : "";
 399+ $alternative_6 = ($wgRequest->getVal('poll_alternative_6') != "")? $wgRequest->getVal('poll_alternative_6') : "";
 400+ $dis = ($wgRequest->getVal('dis') != "")? $wgRequest->getVal('dis') : wfMsg('poll-no-dis');
401401 $user = $wgUser->getName();
402402
403403 $dbw->update( 'poll', array( 'question' => $question, 'alternative_1' => $alternative_1, 'alternative_2' => $alternative_2,
——@@ -430,13 +430,13 @@
431431 $wgOut->addHtml( '<a href="'.$wgTitle->getFullURL('action=list').'">'.wfMsg('poll-back').'</a>' );
432432 }
433433
434 - if ( isset($controll_delete_blocked) AND ( $controll_delete_blocked == true ) ) {
 434+ if ( $controll_delete_blocked == true ) {
435435 $wgOut->addWikiMsg( 'poll-delete-block-error' );
436436 $wgOut->addHtml( '<a href="'.$wgTitle->getFullURL('action=list').'">'.wfMsg('poll-back').'</a>' );
437437 }
438438
439439 if ( ( ( $creater == $user ) OR ( $controll_delete_right == true ) ) AND ( $controll_delete_blocked != true ) ) {
440 - if( isset($_POST['controll_delete']) AND $_POST['controll_delete'] == 1 ) {
 440+ if( isset($wgRequest->getVal('controll_delete')) AND $wgRequest->getVal('controll_delete') === 1 ) {
441441 $dbw = wfGetDB( DB_MASTER );
442442 $user = $wgUser->getName();
443443

Follow-up revisions

RevisionCommit summaryAuthorDate
r51602Fixes for r51600: Use instead of isset($wgRequest->getVal('controll_delete')...jan18:14, 8 June 2009

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r51338* Add a log for Polls...jan12:26, 2 June 2009

Comments

#Comment by 😂 (talk | contribs)   18:09, 8 June 2009

if( isset($wgRequest->getVal('controll_delete')) AND $wgRequest->getVal('controll_delete') === 1 ) {

^ This doesn't seem necessary. Why not just use $wgRequest->getCheck( 'controll_delete' )? There's lots of functions like this in WebRequest that should be used.

Similar cleanup is needed throughout this code.

#Comment by 😂 (talk | contribs)   18:30, 8 June 2009

Saw the followup in r51602. Once again: is there any compelling reason to check === 1? getCheck() should handle the boolean on its own just fine. The following is usually how we handle checkboxes:

if( $wgRequest->getCheck( 'field' ) ) { }

Also, you shouldn't be using $wgTitle. If you're referring to the Special page itself (which it appears you are), you can just use $this->getTitle(). $wgTitle is evil.

#Comment by Jan Luca (talk | contribs)   14:28, 9 June 2009

getCheck() controll only if $_POST/$_GET[...] is set and not the value

Mean you $wgTitle->getFullURL() ?


#Comment by 😂 (talk | contribs)   14:37, 9 June 2009

With a checkbox, you're looking to see if the check is enabled, bool true/false. It doesn't matter what the actual _value_ is.

And yes, I mean $wgTitle->getFullUrl(). $wgTitle is not safe, as you never know what Title it might be at a given point (extensions can change it on a whim, so there's no assurance of it). Using $this->getTitle->getFullUrl() in Special pages (or similar functions elsewhere) is better practice.

#Comment by Jan Luca (talk | contribs)   16:46, 9 June 2009

$this->getTitle->getFullUrl('action=create') (line 64) create a error:

Notice: Undefined property: Poll::$getTitle in ...\wiki\extensions\Poll\Poll_body.php on line 64

Fatal error: Call to a member function getFullUrl() on a non-object in ...\wiki\extensions\Poll\Poll_body.php on line 64

#Comment by 😂 (talk | contribs)   16:55, 9 June 2009

Meant $this->getTitle()->getFullUrl(). GetTitle() is a method, not a member variable.

Status & tagging log