r58887 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r58886‎ | r58887 | r58888 >
Date:22:26, 10 November 2009
Author:happy-melon
Status:resolved (Comments)
Tags:
Comment:
Fix code flow so that messages added to questions by the ballot type (ie column labels for RadioRange voting) are available to translate in TranslatePage. May need some tweaking to work with non-DB storage.
Modified paths:
  • /trunk/extensions/SecurePoll/includes/ballots/Ballot.php (modified) (history)
  • /trunk/extensions/SecurePoll/includes/ballots/RadioRangeBallot.php (modified) (history)
  • /trunk/extensions/SecurePoll/includes/entities/Election.php (modified) (history)
  • /trunk/extensions/SecurePoll/includes/entities/Entity.php (modified) (history)
  • /trunk/extensions/SecurePoll/includes/entities/Question.php (modified) (history)
  • /trunk/extensions/SecurePoll/includes/main/Store.php (modified) (history)

Diff [purge]

Index: trunk/extensions/SecurePoll/includes/ballots/RadioRangeBallot.php
@@ -49,16 +49,16 @@
5050 return $labels;
5151 }
5252
53 - function getMessageNames( $entity ) {
54 - if ( $entity->getType() !== 'question' ) {
 53+ function getMessageNames( $entity=null ) {
 54+ if( $entity===null || $entity->getType() !== 'question' ) {
5555 return array();
5656 }
57 - if ( !$entity->getProperty( 'column-label-msgs' ) ) {
 57+ if( !$entity->getProperty( 'column-label-msgs' ) ) {
5858 return array();
5959 }
6060 $msgs = array();
6161 list( $min, $max ) = $this->getMinMax( $entity );
62 - for ( $score = min; $score <= $max; $score++ ) {
 62+ for ( $score = $min; $score <= $max; $score++ ) {
6363 $signedScore = $this->addSign( $entity, $score );
6464 $msgs[] = "column$signedScore";
6565 }
Index: trunk/extensions/SecurePoll/includes/ballots/Ballot.php
@@ -21,6 +21,14 @@
2222 * @return string
2323 */
2424 abstract function getQuestionForm( $question, $options );
 25+
 26+ /**
 27+ * Get any extra messages that this ballot type uses to render questions.
 28+ * Used to get the list of translatable messages for TranslatePage.
 29+ * @return Array
 30+ * @see SecurePoll_Election::getMessageNames()
 31+ */
 32+ function getMessageNames(){ return array(); }
2533
2634 /**
2735 * Called when the form is submitted. This returns a Status object which,
Index: trunk/extensions/SecurePoll/includes/entities/Entity.php
@@ -40,6 +40,7 @@
4141 /**
4242 * Get a list of localisable message names. This is used to provide the
4343 * translate subpage with a list of messages to localise.
 44+ * @return Array
4445 */
4546 function getMessageNames() {
4647 # STUB
Index: trunk/extensions/SecurePoll/includes/entities/Election.php
@@ -37,7 +37,7 @@
3838 * RemoteMWAuth
3939 * remote-mw-script-path
4040 *
41 - * ChooseBallot
 41+ * Ballot
4242 * shuffle-questions
4343 * shuffle-options
4444 *
Index: trunk/extensions/SecurePoll/includes/entities/Question.php
@@ -5,7 +5,7 @@
66 * more than one question in an election.
77 */
88 class SecurePoll_Question extends SecurePoll_Entity {
9 - var $options;
 9+ var $options, $electionId;
1010
1111 /**
1212 * Constructor
@@ -18,13 +18,16 @@
1919 foreach ( $info['options'] as $optionInfo ) {
2020 $this->options[] = new SecurePoll_Option( $context, $optionInfo );
2121 }
 22+ $this->electionId = $info['election'];
2223 }
2324
2425 /**
2526 * Get a list of localisable message names.
2627 */
2728 function getMessageNames() {
28 - return array( 'text' );
 29+ $ballot = $this->context->getElection( $this->electionId )->getBallot();
 30+ return array_merge( $ballot->getMessageNames( $this ), array( 'text' ) );
 31+
2932 }
3033
3134 /**
Index: trunk/extensions/SecurePoll/includes/main/Store.php
@@ -193,17 +193,27 @@
194194 $questions = array();
195195 $options = array();
196196 $questionId = false;
 197+ $electionId = false;
197198 foreach ( $res as $row ) {
198199 if ( $questionId === false ) {
199200 } elseif ( $questionId !== $row->qu_entity ) {
200 - $questions[] = array( 'id' => $questionId, 'options' => $options );
 201+ $questions[] = array(
 202+ 'id' => $questionId,
 203+ 'election' => $electionId,
 204+ 'options' => $options
 205+ );
201206 $options = array();
202207 }
203208 $options[] = array( 'id' => $row->op_entity );
204209 $questionId = $row->qu_entity;
 210+ $electionId = $row->qu_election;
205211 }
206212 if ( $questionId !== false ) {
207 - $questions[] = array( 'id' => $questionId, 'options' => $options );
 213+ $questions[] = array(
 214+ 'id' => $questionId,
 215+ 'election' => $electionId,
 216+ 'options' => $options
 217+ );
208218 }
209219 return $questions;
210220 }
@@ -337,7 +347,7 @@
338348 'endDate',
339349 'auth'
340350 ),
341 - 'question' => array( 'id' ),
 351+ 'question' => array( 'id', 'election' ),
342352 'option' => array( 'id' ),
343353 );
344354

Follow-up revisions

RevisionCommit summaryAuthorDate
r60920Fixed code style issues from r58887.tstarling04:23, 11 January 2010
r64483Follow-ups to r58887. There might be some stuff from my configuration interf...happy-melon11:55, 1 April 2010

Comments

#Comment by Tim Starling (talk | contribs)   04:41, 11 January 2010

The XML store is obviously broken, since it does not set the election property when it creates questions. That could be fixed by adding parent ID properties to the child elements at the end of readEntity(), near where $this->idsByName is set up.

There's also a problem with the DB store. Context::getElection() is uncached, so this change will increase the query count. A cache would fix that.

I had considered adding parent entity references to Question and Option, but then you have all the usual problems with circular references: PHP's memory management breaks, and Context::varDump() would have to have cycle detection to avoid infinite recursion. So I can see the value in using IDs instead.

It sorely needs an Entity::getElection() method to abstract out the code you have in getMessageNames(). And Options should be able to get their parent election as well, not just Questions.

#Comment by Happy-melon (talk | contribs)   11:57, 1 April 2010

In r64483:

The XML store isn't actually broken, because import.php sets parent ids when adding stuff to the db. But I made the XML extractor set the ids anyway.
Added a cache to Context::getElection()
abstracted Question::getElection() to Entity::getElection().

Status & tagging log