r92528 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r92527‎ | r92528 | r92529 >
Date:12:30, 19 July 2011
Author:robin
Status:resolved (Comments)
Tags:scaptrap, todo 
Comment:
(bug 19838) API does not use interwiki cache.

Patch by Beau; modified to work with current code.
Modified paths:
  • /trunk/phase3/CREDITS (modified) (history)
  • /trunk/phase3/RELEASE-NOTES-1.19 (modified) (history)
  • /trunk/phase3/includes/api/ApiQuerySiteinfo.php (modified) (history)
  • /trunk/phase3/includes/interwiki/Interwiki.php (modified) (history)
  • /trunk/phase3/maintenance/dumpInterwiki.php (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/dumpInterwiki.php
——@@ -118,7 +118,7 @@
119119 $this->error( "m:Interwiki_map not found", true );
120120 }
121121
122 - # Global iterwiki map
 122+ # Global interwiki map
123123 foreach ( $lines as $line ) {
124124 if ( preg_match( '/^\|\s*(.*?)\s*\|\|\s*(.*?)\s*$/', $line, $matches ) ) {
125125 $prefix = $wgContLang->lc( $matches[1] );
——@@ -195,6 +195,17 @@
196196 foreach ( $extraLinks as $link ) {
197197 $this->makeLink( $link, "__global" );
198198 }
 199+
 200+ # List prefixes for each source
 201+ foreach ( $this->prefixLists as $source => $hash ) {
 202+ $list = array_keys( $hash );
 203+ sort( $list );
 204+ if ( $this->dbFile ) {
 205+ $this->dbFile->set( "__list:{$source}", implode( ' ', $list ) );
 206+ } else {
 207+ print "__list:{$source} " . implode( ' ', $list ) . "\n";
 208+ }
 209+ }
199210 }
200211
201212 # ------------------------------------------------------------------------------------------
——@@ -229,6 +240,9 @@
230241 } else {
231242 $this->output( "{$source}:{$entry['iw_prefix']} {$entry['iw_url']} {$entry['iw_local']}\n" );
232243 }
 244+
 245+ # Add to list of prefixes
 246+ $this->prefixLists[$source][$entry['iw_prefix']] = 1;
233247 }
234248 }
235249
Index: trunk/phase3/RELEASE-NOTES-1.19
——@@ -15,6 +15,7 @@
1616 === New features in 1.19 ===
1717
1818 === Bug fixes in 1.19 ===
 19+* (bug 19838) API does not use interwiki cache.
1920
2021 === API changes in 1.19 ===
2122
Index: trunk/phase3/CREDITS
——@@ -158,6 +158,9 @@
159159 * Yuvaraj Pandian T
160160 * Zachary Hauri
161161
 162+== Patch Contributors ==
 163+* Beau
 164+
162165 == Translators ==
163166 * Anders Wegge Jakobsen
164167 * Hk kng
Index: trunk/phase3/includes/interwiki/Interwiki.php
——@@ -199,6 +199,125 @@
200200 }
201201
202202 /**
 203+ * Fetch all interwiki prefixes from interwiki cache
 204+ *
 205+ * @param $local If set, limits output to local/non-local interwikis
 206+ * @return Array List of prefixes
 207+ * @since 1.19
 208+ * @static
 209+ */
 210+ protected static function getAllPrefixesCached( $local ) {
 211+ global $wgInterwikiCache, $wgInterwikiScopes, $wgInterwikiFallbackSite;
 212+ static $db, $site;
 213+
 214+ wfDebug( __METHOD__ . "()\n" );
 215+ if( !$db ) {
 216+ $db = CdbReader::open( $wgInterwikiCache );
 217+ }
 218+ /* Resolve site name */
 219+ if( $wgInterwikiScopes >= 3 && !$site ) {
 220+ $site = $db->get( '__sites:' . wfWikiID() );
 221+ if ( $site == '' ) {
 222+ $site = $wgInterwikiFallbackSite;
 223+ }
 224+ }
 225+
 226+ // List of interwiki sources
 227+ $sources = array();
 228+ // Global Level
 229+ if ( $wgInterwikiScopes >= 2 ) {
 230+ $sources[] = '__global';
 231+ }
 232+ // Site level
 233+ if ( $wgInterwikiScopes >= 3 ) {
 234+ $sources[] = '_' . $site;
 235+ }
 236+ $sources[] = wfWikiID();
 237+
 238+ $data = array();
 239+
 240+ foreach( $sources as $source ) {
 241+ $list = $db->get( "__list:{$source}" );
 242+ foreach ( explode( ' ', $list ) as $iw_prefix ) {
 243+ $row = $db->get( "{$source}:{$iw_prefix}" );
 244+ if( !$row ) {
 245+ continue;
 246+ }
 247+
 248+ list( $iw_local, $iw_url ) = explode( ' ', $row );
 249+
 250+ if ( isset( $local ) && $local != $iw_local ) {
 251+ continue;
 252+ }
 253+
 254+ $data[$iw_prefix] = array(
 255+ 'iw_prefix' => $iw_prefix,
 256+ 'iw_url' => $iw_url,
 257+ 'iw_local' => $iw_local,
 258+ );
 259+ }
 260+ }
 261+
 262+ ksort( $data );
 263+
 264+ return array_values( $data );
 265+ }
 266+
 267+ /**
 268+ * Fetch all interwiki prefixes from DB
 269+ *
 270+ * @param $local If set, limits output to local/non-local interwikis
 271+ * @return Array List of prefixes
 272+ * @since 1.19
 273+ * @static
 274+ */
 275+ protected static function getAllPrefixesDb( $local ) {
 276+ $db = wfGetDB( DB_SLAVE );
 277+
 278+ $where = array();
 279+
 280+ if ( isset($local) ) {
 281+ if ( $local == 1 ) {
 282+ $where['iw_local'] = 1;
 283+ }
 284+ elseif ( $local == 0 ) {
 285+ $where['iw_local'] = 0;
 286+ }
 287+ }
 288+
 289+ $res = $db->select( 'interwiki',
 290+ array( 'iw_prefix', 'iw_url', 'iw_api', 'iw_wikiid', 'iw_local', 'iw_trans' ),
 291+ $where, __METHOD__, array( 'ORDER BY' => 'iw_prefix' )
 292+ );
 293+
 294+ $data = array();
 295+ while( $row = $db->fetchRow($res) ) {
 296+ $data[] = $row;
 297+ }
 298+ $db->freeResult( $res );
 299+
 300+ return $data;
 301+ }
 302+
 303+ /**
 304+ * Returns all interwiki prefixes
 305+ *
 306+ * @param $local If set, limits output to local/non-local interwikis
 307+ * @return Array List of prefixes
 308+ * @since 1.19
 309+ * @static
 310+ */
 311+ public static function getAllPrefixes( $local ) {
 312+ global $wgInterwikiCache;
 313+
 314+ if ( $wgInterwikiCache ) {
 315+ return self::getAllPrefixesCached( $local );
 316+ } else {
 317+ return self::getAllPrefixesDb( $local );
 318+ }
 319+ }
 320+
 321+ /**
203322 * Get the URL for a particular title (or with $1 if no title given)
204323 *
205324 * @param $title String: what text to put for the article name
Index: trunk/phase3/includes/api/ApiQuerySiteinfo.php
——@@ -256,37 +256,36 @@
257257 }
258258
259259 protected function appendInterwikiMap( $property, $filter ) {
260 - $this->resetQueryParams();
261 - $this->addTables( 'interwiki' );
262 - $this->addFields( array( 'iw_prefix', 'iw_local', 'iw_url', 'iw_wikiid', 'iw_api' ) );
263 -
 260+ $local = null;
264261 if ( $filter === 'local' ) {
265 - $this->addWhere( 'iw_local = 1' );
 262+ $local = 1;
266263 } elseif ( $filter === '!local' ) {
267 - $this->addWhere( 'iw_local = 0' );
 264+ $local = 0;
268265 } elseif ( $filter ) {
269266 ApiBase::dieDebug( __METHOD__, "Unknown filter=$filter" );
270267 }
271268
272 - $this->addOption( 'ORDER BY', 'iw_prefix' );
273 -
274 - $res = $this->select( __METHOD__ );
275 -
 269+ $getPrefixes = Interwiki::getAllPrefixes( $local );
276270 $data = array();
277271 $langNames = Language::getLanguageNames();
278 - foreach ( $res as $row ) {
 272+ foreach ( $getPrefixes as $row ) {
 273+ $prefix = $row['iw_prefix'];
279274 $val = array();
280 - $val['prefix'] = $row->iw_prefix;
281 - if ( $row->iw_local == '1' ) {
 275+ $val['prefix'] = $prefix;
 276+ if ( $row['iw_local'] == '1' ) {
282277 $val['local'] = '';
283278 }
284 - // $val['trans'] = intval( $row->iw_trans ); // should this be exposed?
285 - if ( isset( $langNames[$row->iw_prefix] ) ) {
286 - $val['language'] = $langNames[$row->iw_prefix];
 279+ // $val['trans'] = intval( $row['iw_trans'] ); // should this be exposed?
 280+ if ( isset( $langNames[$prefix] ) ) {
 281+ $val['language'] = $langNames[$prefix];
287282 }
288 - $val['url'] = wfExpandUrl( $row->iw_url );
289 - $val['wikiid'] = $row->iw_wikiid;
290 - $val['api'] = $row->iw_api;
 283+ $val['url'] = wfExpandUrl( $row['iw_url'] );
 284+ if( isset( $row['iw_wikiid'] ) ) {
 285+ $val['wikiid'] = $row['iw_wikiid'];
 286+ }
 287+ if( isset( $row['iw_api'] ) ) {
 288+ $val['api'] = $row['iw_api'];
 289+ }
291290
292291 $data[] = $val;
293292 }

Sign-offs

UserFlagDate
RobLa-WMFtested21:57, 19 July 2011

Follow-up revisions

RevisionCommit summaryAuthorDate
r92529* Use interwiki cache (per r92528)...robin12:45, 19 July 2011
r92533Follow-up r92528: probably better to put it in the API & new features sectionsrobin13:30, 19 July 2011
r92538Follow-up r92528:...robin15:52, 19 July 2011
r92813Fixes for r92538 & r92528:...robin22:19, 21 July 2011
r95020Remove duplicate patch contributors section from CREDITS added in r92528reedy17:54, 19 August 2011
r97789Fixed accessing of $row var. Was giving:...aaron03:59, 22 September 2011
r97806Merge r97789 into trunk...reedy11:04, 22 September 2011

Comments

#Comment by Jack Phoenix (talk | contribs)   15:24, 19 July 2011 
+	 * @static
+	 */
+	protected static function getAllPrefixesCached( $local ) {
[...]
+	 * @static
+	 */
+	protected static function getAllPrefixesDb( $local ) {

Regarding @static: "Please don't use this silly annotation anymore, that's what the static keyword is for."

Also, I'd prefer if getAllPrefixesDb would be called getAllPrefixesDB. 

+			}
+			elseif ( $local == 0 ) {

This should be on the same line: } elseif ( $local == 0 ) { 

+		$data = array();
+		while( $row = $db->fetchRow($res) ) {
+			$data[] = $row;
+		}
+		$db->freeResult( $res );

Old-school style; you can use foreach( $res as $row ) { instead of the while loop and you don't need the freeResult() call. See Manual:Database access#Database Abstraction Layer.

#Comment by SPQRobin (talk | contribs)   15:53, 19 July 2011

Done in r92538. There was actually no need for the while() loop.

#Comment by Brion VIBBER (talk | contribs)   16:52, 19 July 2011

It looks like this'll require rebuilding the interwiki .cdb databases before deploying in order for the prefix lookup to work, is that correct?

#Comment by 😂 (talk | contribs)   16:54, 19 July 2011

Possibly. To be honest I think having a special key called "allcacheitems" or similar would be nice and could be done when the file's generated.

#Comment by SPQRobin (talk | contribs)   17:14, 19 July 2011

Afaik, rebuilding (with the new dumpInterwiki) is needed to make the API return the correct results. Otherwise it would return an empty result (FYI, this is currently the case anyway on several wikis, e.g. http://pl.wikisource.org/w/api.php?action=query&meta=siteinfo&siprop=interwikimap ). So it's not absolutely needed, but I assume bots depend on the API interwikimap.

#Comment by RobLa-WMF (talk | contribs)   21:58, 19 July 2011

Roan swears he tested this  :)

#Comment by Catrope (talk | contribs)   18:22, 20 July 2011

I was referring to a different rev when I said this, oops. Striking sign-off.

#Comment by Catrope (talk | contribs)   18:28, 20 July 2011 
+				print "__list:{$source} " . implode( ' ', $list ) . "\n";

The one other piece of code that writes to standard out instead of to $this->dbFile uses $this->output() instead of print.

It would be nice if $this->prefixLists was declared and initialized somewhere, such that 1) the script won't throw a notice in the unlikely case makeLink() is never called and 2) you can document its format.

#Comment by Catrope (talk | contribs)   18:47, 20 July 2011 
+				if ( isset( $local ) && $local != $iw_local ) {

Don't use isset() if you really meant to check for null, use is_null( $local ) or $local === null. The function is never called without arguments and it's private, so $local is always set. 

+		if ( isset($local) ) {

Same. 

+	 * @param $local If set, limits output to local/non-local interwikis

Eww. It's nicer to just make it default to null (with function getAllPrefixes( $local = null ), and say "If not null, ..."

OK otherwise.

#Comment by SPQRobin (talk | contribs)   22:20, 21 July 2011

Done in r92813

#Comment by Duplicatebug (talk | contribs)   17:52, 19 August 2011

There is no need, to create a new section "Patch Contributors" in CREDITS

#Comment by Reedy (talk | contribs)   19:04, 1 March 2012

I think this may have caused bug 34865

Status & tagging log