r102083 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r102082‎ | r102083 | r102084 >
Date:04:14, 5 November 2011
Author:aaron
Status:ok (Comments)
Tags:
Comment:
Made use of Pager::doBatchLookups() and Pager::getUser() in special page Pagers
Modified paths:
  • /trunk/extensions/FlaggedRevs/presentation/FlaggedRevsXML.php (modified) (history)
  • /trunk/extensions/FlaggedRevs/presentation/specialpages/reports/ConfiguredPages_body.php (modified) (history)
  • /trunk/extensions/FlaggedRevs/presentation/specialpages/reports/PendingChanges_body.php (modified) (history)
  • /trunk/extensions/FlaggedRevs/presentation/specialpages/reports/ProblemChanges_body.php (modified) (history)
  • /trunk/extensions/FlaggedRevs/presentation/specialpages/reports/ReviewedPages_body.php (modified) (history)
  • /trunk/extensions/FlaggedRevs/presentation/specialpages/reports/StablePages_body.php (modified) (history)
  • /trunk/extensions/FlaggedRevs/presentation/specialpages/reports/UnreviewedPages_body.php (modified) (history)

Diff [purge]

Index: trunk/extensions/FlaggedRevs/presentation/FlaggedRevsXML.php
——@@ -57,13 +57,16 @@
5858 ) {
5959 $s = "<label for='wpLevel'>" . wfMsgHtml( 'revreview-levelfilter' ) . "</label>\n";
6060 $s .= Xml::openElement( 'select', array( 'name' => 'level', 'id' => 'wpLevel' ) );
61 - if ( $all !== false )
 61+ if ( $all !== false ) {
6262 $s .= Xml::option( wfMsg( $all ), - 1, $selected === - 1 );
 63+ }
6364 $s .= Xml::option( wfMsg( 'revreview-lev-basic' ), 0, $selected === 0 );
64 - if ( FlaggedRevs::qualityVersions() )
 65+ if ( FlaggedRevs::qualityVersions() ) {
6566 $s .= Xml::option( wfMsg( 'revreview-lev-quality' ), 1, $selected === 1 );
66 - if ( $max >= 2 && FlaggedRevs::pristineVersions() )
 67+ }
 68+ if ( $max >= 2 && FlaggedRevs::pristineVersions() ) {
6769 $s .= Xml::option( wfMsg( 'revreview-lev-pristine' ), 2, $selected === 2 );
 70+ }
6871 # Note: Pristine not tracked at sp:QualityOversight (counts as quality)
6972 $s .= Xml::closeElement( 'select' ) . "\n";
7073 return $s;
Index: trunk/extensions/FlaggedRevs/presentation/specialpages/reports/PendingChanges_body.php
——@@ -339,13 +339,12 @@
340340 $query['category'] = $this->category;
341341 return $query;
342342 }
343 -
 343+
344344 function getDefaultDirections() {
345345 return false;
346346 }
347347
348348 function getQueryInfo() {
349 - global $wgUser;
350349 $tables = array( 'page', 'revision' );
351350 $fields = array( 'page_namespace', 'page_title', 'page_len', 'rev_len', 'page_latest' );
352351 # Show outdated "stable" versions
——@@ -401,11 +400,14 @@
402401 $conds['page_namespace'] = $this->namespace;
403402 }
404403 # Filter by watchlist
405 - if ( $this->watched && ( $uid = $wgUser->getId() ) ) {
406 - $tables[] = 'watchlist';
407 - $conds[] = "wl_user = '$uid'";
408 - $conds[] = 'page_namespace = wl_namespace';
409 - $conds[] = 'page_title = wl_title';
 404+ if ( $this->watched ) {
 405+ $uid = (int)$this->getUser()->getId();
 406+ if ( $uid ) {
 407+ $tables[] = 'watchlist';
 408+ $conds[] = "wl_user = '$uid'";
 409+ $conds[] = 'page_namespace = wl_namespace';
 410+ $conds[] = 'page_title = wl_title';
 411+ }
410412 }
411413 # Filter by bytes changed
412414 if ( $this->size !== null && $this->size >= 0 ) {
——@@ -423,19 +425,21 @@
424426 function getIndexField() {
425427 return $this->mIndexField;
426428 }
427 -
428 - function getStartBody() {
 429+
 430+ function doBatchLookups() {
429431 wfProfileIn( __METHOD__ );
430 - # Do a link batch query
431432 $lb = new LinkBatch();
432433 foreach ( $this->mResult as $row ) {
433434 $lb->add( $row->page_namespace, $row->page_title );
434435 }
435436 $lb->execute();
436437 wfProfileOut( __METHOD__ );
 438+ }
 439+
 440+ function getStartBody() {
437441 return '<ul>';
438442 }
439 -
 443+
440444 function getEndBody() {
441445 return '</ul>';
442446 }
Index: trunk/extensions/FlaggedRevs/presentation/specialpages/reports/UnreviewedPages_body.php
——@@ -432,15 +432,17 @@
433433 return $this->mIndexField;
434434 }
435435
436 - function getStartBody() {
 436+ function doBatchLookups() {
437437 wfProfileIn( __METHOD__ );
438 - # Do a link batch query
439438 $lb = new LinkBatch();
440439 foreach ( $this->mResult as $row ) {
441440 $lb->add( $row->page_namespace, $row->page_title );
442441 }
443442 $lb->execute();
444443 wfProfileOut( __METHOD__ );
 444+ }
 445+
 446+ function getStartBody() {
445447 return '<ul>';
446448 }
447449
Index: trunk/extensions/FlaggedRevs/presentation/specialpages/reports/ConfiguredPages_body.php
——@@ -177,18 +177,20 @@
178178 return 'fpc_page_id';
179179 }
180180
181 - function getStartBody() {
 181+ function doBatchLookups() {
182182 wfProfileIn( __METHOD__ );
183 - # Do a link batch query
184183 $lb = new LinkBatch();
185184 foreach ( $this->mResult as $row ) {
186185 $lb->add( $row->page_namespace, $row->page_title );
187186 }
188187 $lb->execute();
189188 wfProfileOut( __METHOD__ );
 189+ }
 190+
 191+ function getStartBody() {
190192 return '<ul>';
191193 }
192 -
 194+
193195 function getEndBody() {
194196 return '</ul>';
195197 }
Index: trunk/extensions/FlaggedRevs/presentation/specialpages/reports/ProblemChanges_body.php
——@@ -314,7 +314,7 @@
315315 function formatRow( $row ) {
316316 return $this->mForm->formatRow( $row );
317317 }
318 -
 318+
319319 function getDefaultDirections() {
320320 return false;
321321 }
——@@ -394,19 +394,21 @@
395395 function getIndexField() {
396396 return $this->mIndexField;
397397 }
398 -
399 - function getStartBody() {
 398+
 399+ function doBatchLookups() {
400400 wfProfileIn( __METHOD__ );
401 - # Do a link batch query
402401 $lb = new LinkBatch();
403402 foreach ( $this->mResult as $row ) {
404403 $lb->add( $row->page_namespace, $row->page_title );
405404 }
406405 $lb->execute();
407406 wfProfileOut( __METHOD__ );
 407+ }
 408+
 409+ function getStartBody() {
408410 return '<ul>';
409411 }
410 -
 412+
411413 function getEndBody() {
412414 return '</ul>';
413415 }
Index: trunk/extensions/FlaggedRevs/presentation/specialpages/reports/ReviewedPages_body.php
——@@ -166,27 +166,30 @@
167167 'tables' => array( 'flaggedpages', 'page' ),
168168 'fields' => 'page_namespace,page_title,page_len,fp_page_id',
169169 'conds' => $conds,
170 - 'options' => array( 'USE INDEX' => array( 'flaggedpages' => $index,
171 - 'page' => 'PRIMARY' ) )
 170+ 'options' => array(
 171+ 'USE INDEX' => array( 'flaggedpages' => $index, 'page' => 'PRIMARY' )
 172+ )
172173 );
173174 }
174175
175176 function getIndexField() {
176177 return 'fp_page_id';
177178 }
178 -
179 - function getStartBody() {
 179+
 180+ function doBatchLookups() {
180181 wfProfileIn( __METHOD__ );
181 - # Do a link batch query
182182 $lb = new LinkBatch();
183183 foreach ( $this->mResult as $row ) {
184184 $lb->add( $row->page_namespace, $row->page_title );
185185 }
186186 $lb->execute();
187187 wfProfileOut( __METHOD__ );
 188+ }
 189+
 190+ function getStartBody() {
188191 return '<ul>';
189192 }
190 -
 193+
191194 function getEndBody() {
192195 return '</ul>';
193196 }
Index: trunk/extensions/FlaggedRevs/presentation/specialpages/reports/StablePages_body.php
——@@ -168,19 +168,21 @@
169169 function getIndexField() {
170170 return 'fpc_page_id';
171171 }
172 -
173 - function getStartBody() {
 172+
 173+ function doBatchLookups() {
174174 wfProfileIn( __METHOD__ );
175 - # Do a link batch query
176175 $lb = new LinkBatch();
177176 foreach ( $this->mResult as $row ) {
178177 $lb->add( $row->page_namespace, $row->page_title );
179178 }
180179 $lb->execute();
181180 wfProfileOut( __METHOD__ );
 181+ }
 182+
 183+ function getStartBody() {
182184 return '<ul>';
183185 }
184 -
 186+
185187 function getEndBody() {
186188 return '</ul>';
187189 }

Comments

#Comment by Hashar (talk | contribs)   14:57, 28 November 2011

I love how that getStartBody() and doBatchLookups() really looks like duplicate code. Not a big issue anyway.

#Comment by Aaron Schulz (talk | contribs)   04:53, 29 November 2011

What do you mean?

#Comment by Hashar (talk | contribs)   09:53, 29 November 2011

Both methods are exactly the same in StablePages_body.php ReviewedPages_body.php etc .. Those classes could be refactored to extend a common class that would host the getStartBody() / doBatchLookups() methods.

But I am not sure it is worth the time to do it, hence why I marked the change ok.

Is it clearer?

#Comment by Aaron Schulz (talk | contribs)   21:24, 29 November 2011

Oh, I thought you were talking about the core function.

I'm not sure if it's worth a common method. Different Pagers will want different batch lookups. Though a page_namespace,page_title one is probably common enough to deserve a function maybe. I don't want nominal OOP just for code reuse though.

Status & tagging log