r98411 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r98410‎ | r98411 | r98412 >
Date:15:16, 29 September 2011
Author:bawolff
Status:ok (Comments)
Tags:
Comment:
Fix handling of qplimit/qpoffset and cachedtimestamp in QueryPage api module

Basically api was thinking qplimit was the offset, and qpoffset was the limit. Also remove an extra ! when checking for the cached timestamp which was stopping the display of cachedtimestamp property in the api output.

I'm going to tag this 1.18 since this module was introduced in 1.18, and I assume we'd want to fix the limit/offset parameter before the wrong behaviour gets released and people depend on it, etc.
Modified paths:
  • /trunk/phase3/includes/QueryPage.php (modified) (history)
  • /trunk/phase3/includes/api/ApiQueryQueryPage.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/QueryPage.php
——@@ -423,7 +423,7 @@
424424 }
425425
426426 public function getCachedTimestamp() {
427 - if ( !is_null( $this->cachedTimestamp ) ) {
 427+ if ( is_null( $this->cachedTimestamp ) ) {
428428 $dbr = wfGetDB( DB_SLAVE );
429429 $fname = get_class( $this ) . '::getCachedTimestamp';
430430 $this->cachedTimestamp = $dbr->selectField( 'querycache_info', 'qci_timestamp',
Index: trunk/phase3/includes/api/ApiQueryQueryPage.php
——@@ -103,7 +103,7 @@
104104 return;
105105 }
106106
107 - $res = $qp->doQuery( $params['limit'] + 1, $params['offset'] );
 107+ $res = $qp->doQuery( $params['offset'], $params['limit'] + 1 );
108108 $count = 0;
109109 $titles = array();
110110 foreach ( $res as $row ) {

Sign-offs

UserFlagDate
Bryaninspected19:41, 2 October 2011

Follow-up revisions

RevisionCommit summaryAuthorDate
r990941.18wmf1: MFT r98235, r98411, r98665, r98676, r98678, r98773, r98812, r99082,...catrope13:01, 6 October 2011
r99989REL1_18:...reedy22:24, 16 October 2011

Comments

#Comment by Reedy (talk | contribs)   19:53, 29 September 2011 
	function reallyDoQuery( $limit, $offset = false ) {

Doesn't that mean those parameters are wrong?

#Comment by Bawolff (talk | contribs)   22:25, 29 September 2011

Hmm 

function doQuery( $offset = false, $limit = false ) {

so reallyDoQuery and doQuery accept there parameters in different orders. That's not confusing at all...

#Comment by Reedy (talk | contribs)   22:38, 29 September 2011

Any other usages in extensions etc?

Certainly be the time to fix it...

#Comment by Bawolff (talk | contribs)   03:07, 30 September 2011

It seems like a bunch of extensions were fixed to use this new method (ex r78810. grep suggests quite a few others)

#Comment by Reedy (talk | contribs)   12:27, 3 October 2011

Right 

	function doQuery( $offset = false, $limit = false ) {

	function reallyDoQuery( $limit, $offset = false ) {
#Comment by Reedy (talk | contribs)   12:33, 3 October 2011

Right

Pre 1.18, "deprecated" 

	function doQuery( $offset = false, $limit = false ) {

Usages, 6: ApprovedRevs, SemanticDrilldown, SemanticForms, SpecialLinkSearch

Added in 1.18 

	function reallyDoQuery( $limit, $offset = false ) {

Usages, 6: ProofreadPage, QueryPage

Added in 1.18 

	function fetchFromCache( $limit, $offset = false ) {

Usages, 2: Only in QueryPage.php


To me, as doQuery is the first (oldest) method, normalising them to be the same as that seems to make sense, update all inbound callers, and the 1.18/1.18wmf for backport

Status & tagging log