r41377 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r41376‎ | r41377 | r41378 >
Date:10:08, 29 September 2008
Author:mattj
Status:old (Comments)
Tags:
Comment:
New format for accessing Interwiki data. Replaced all old ways within core I could find, but should be backwards compatible wherever needed.

Also fix the notes in RELEASE-NOTES to state PHP 5.2 recommended as certain classes require it (e.g. ForeignAPIRepo, which uses json_decode)
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/config/index.php (modified) (history)
  • /trunk/phase3/includes/AutoLoader.php (modified) (history)
  • /trunk/phase3/includes/Interwiki.php (added) (history)
  • /trunk/phase3/includes/Title.php (modified) (history)

Diff [purge]

Index: trunk/phase3/RELEASE-NOTES
@@ -310,7 +310,7 @@
311311
312312 == Compatibility ==
313313
314 -MediaWiki 1.14 requires PHP 5 (5.1 recommended). PHP 4 is no longer supported.
 314+MediaWiki 1.14 requires PHP 5 (5.2 recommended). PHP 4 is no longer supported.
315315
316316 PHP 5.0.x fails on 64-bit systems due to serious bugs with array processing:
317317 http://bugs.php.net/bug.php?id=34879
Index: trunk/phase3/includes/AutoLoader.php
@@ -91,6 +91,7 @@
9292 'ImageQueryPage' => 'includes/ImageQueryPage.php',
9393 'IncludableSpecialPage' => 'includes/SpecialPage.php',
9494 'IndexPager' => 'includes/Pager.php',
 95+ 'Interwiki' => 'includes/Interwiki.php',
9596 'IP' => 'includes/IP.php',
9697 'Job' => 'includes/JobQueue.php',
9798 'License' => 'includes/Licenses.php',
Index: trunk/phase3/includes/Title.php
@@ -404,103 +404,13 @@
405405 * @return \type{\string} the associated URL, containing "$1",
406406 * which should be replaced by an article title
407407 * @static (arguably)
 408+ * @deprecated See Interwiki class
408409 */
409410 public function getInterwikiLink( $key ) {
410 - global $wgMemc, $wgInterwikiExpiry;
411 - global $wgInterwikiCache, $wgContLang;
412 - $fname = 'Title::getInterwikiLink';
413 -
414 - if ( count( Title::$interwikiCache ) >= self::CACHE_MAX ) {
415 - // Don't use infinite memory
416 - reset( Title::$interwikiCache );
417 - unset( Title::$interwikiCache[ key( Title::$interwikiCache ) ] );
418 - }
419 -
420 - $key = $wgContLang->lc( $key );
421 -
422 - $k = wfMemcKey( 'interwiki', $key );
423 - if( array_key_exists( $k, Title::$interwikiCache ) ) {
424 - return Title::$interwikiCache[$k]->iw_url;
425 - }
426 -
427 - if ($wgInterwikiCache) {
428 - return Title::getInterwikiCached( $key );
429 - }
430 -
431 - $s = $wgMemc->get( $k );
432 - # Ignore old keys with no iw_local
433 - if( $s && isset( $s->iw_local ) && isset($s->iw_trans)) {
434 - Title::$interwikiCache[$k] = $s;
435 - return $s->iw_url;
436 - }
437 -
438 - $dbr = wfGetDB( DB_SLAVE );
439 - $res = $dbr->select( 'interwiki',
440 - array( 'iw_url', 'iw_local', 'iw_trans' ),
441 - array( 'iw_prefix' => $key ), $fname );
442 - if( !$res ) {
443 - return '';
444 - }
445 -
446 - $s = $dbr->fetchObject( $res );
447 - if( !$s ) {
448 - # Cache non-existence: create a blank object and save it to memcached
449 - $s = (object)false;
450 - $s->iw_url = '';
451 - $s->iw_local = 0;
452 - $s->iw_trans = 0;
453 - }
454 - $wgMemc->set( $k, $s, $wgInterwikiExpiry );
455 - Title::$interwikiCache[$k] = $s;
456 -
457 - return $s->iw_url;
 411+ return Interwiki::fetch( $key )->getURL( );
458412 }
459413
460414 /**
461 - * Fetch interwiki prefix data from local cache in constant database.
462 - *
463 - * @note More logic is explained in DefaultSettings.
464 - *
465 - * @param $key \type{\string} Database key
466 - * @return \type{\string} URL of interwiki site
467 - */
468 - public static function getInterwikiCached( $key ) {
469 - global $wgInterwikiCache, $wgInterwikiScopes, $wgInterwikiFallbackSite;
470 - static $db, $site;
471 -
472 - if (!$db)
473 - $db=dba_open($wgInterwikiCache,'r','cdb');
474 - /* Resolve site name */
475 - if ($wgInterwikiScopes>=3 and !$site) {
476 - $site = dba_fetch('__sites:' . wfWikiID(), $db);
477 - if ($site=="")
478 - $site = $wgInterwikiFallbackSite;
479 - }
480 - $value = dba_fetch( wfMemcKey( $key ), $db);
481 - if ($value=='' and $wgInterwikiScopes>=3) {
482 - /* try site-level */
483 - $value = dba_fetch("_{$site}:{$key}", $db);
484 - }
485 - if ($value=='' and $wgInterwikiScopes>=2) {
486 - /* try globals */
487 - $value = dba_fetch("__global:{$key}", $db);
488 - }
489 - if ($value=='undef')
490 - $value='';
491 - $s = (object)false;
492 - $s->iw_url = '';
493 - $s->iw_local = 0;
494 - $s->iw_trans = 0;
495 - if ($value!='') {
496 - list($local,$url)=explode(' ',$value,2);
497 - $s->iw_url=$url;
498 - $s->iw_local=(int)$local;
499 - }
500 - Title::$interwikiCache[wfMemcKey( 'interwiki', $key )] = $s;
501 - return $s->iw_url;
502 - }
503 -
504 - /**
505415 * Determine whether the object refers to a page within
506416 * this project.
507417 *
@@ -509,10 +419,7 @@
510420 */
511421 public function isLocal() {
512422 if ( $this->mInterwiki != '' ) {
513 - # Make sure key is loaded into cache
514 - $this->getInterwikiLink( $this->mInterwiki );
515 - $k = wfMemcKey( 'interwiki', $this->mInterwiki );
516 - return (bool)(Title::$interwikiCache[$k]->iw_local);
 423+ return Interwiki::fetch( $this->mInterwiki )->isLocal();
517424 } else {
518425 return true;
519426 }
@@ -527,10 +434,8 @@
528435 public function isTrans() {
529436 if ($this->mInterwiki == '')
530437 return false;
531 - # Make sure key is loaded into cache
532 - $this->getInterwikiLink( $this->mInterwiki );
533 - $k = wfMemcKey( 'interwiki', $this->mInterwiki );
534 - return (bool)(Title::$interwikiCache[$k]->iw_trans);
 438+
 439+ return Interwiki::fetch( $this->mInterwiki )->isTranscludable();
535440 }
536441
537442 /**
@@ -778,7 +683,7 @@
779684 $url = $wgServer . $url;
780685 }
781686 } else {
782 - $baseUrl = $this->getInterwikiLink( $this->mInterwiki );
 687+ $baseUrl = Interwiki::fetch( $this->mInterwiki )->getURL( );
783688
784689 $namespace = wfUrlencode( $this->getNsText() );
785690 if ( '' != $namespace ) {
@@ -2162,7 +2067,7 @@
21632068 # Ordinary namespace
21642069 $dbkey = $m[2];
21652070 $this->mNamespace = $ns;
2166 - } elseif( $this->getInterwikiLink( $p ) ) {
 2071+ } elseif( new Interwiki( $p ) ) {
21672072 if( !$firstPass ) {
21682073 # Can't make a local interwiki link to an interwiki link.
21692074 # That's just crazy!
Index: trunk/phase3/includes/Interwiki.php
@@ -0,0 +1,217 @@
 2+<?php
 3+/**
 4+ * @file
 5+ * Interwiki table entry
 6+ */
 7+
 8+/**
 9+ * The interwiki class
 10+ * All information is loaded on creation when called by Interwiki::fetch( $prefix ).
 11+ * All work is done on slave, because this should *never* change (except during schema updates etc, which arent wiki-related)
 12+ */
 13+class Interwiki {
 14+
 15+ // Cache - removed in LRU order when it hits limit
 16+ protected static $smCache = array();
 17+ const CACHE_LIMIT = 100; // 0 means unlimited, any other value is max number of entries.
 18+
 19+ protected $mPrefix, $mURL, $mLocal, $mTrans;
 20+
 21+ function __construct( $prefix = null, $url = '', $local = 0, $trans = 0 )
 22+ {
 23+ $this->mPrefix = $prefix;
 24+ $this->mURL = $url;
 25+ $this->mLocal = $local;
 26+ $this->mTrans = $trans;
 27+ }
 28+
 29+ /**
 30+ * Fetch an Interwiki object
 31+ *
 32+ * @return Interwiki Object, or null if not valid
 33+ * @param $prefix string Interwiki prefix to use
 34+ */
 35+ static public function fetch( $prefix ) {
 36+ if( isset( self::$smCache[$prefix] ) ){
 37+ return self::$smCache[$prefix];
 38+ }
 39+ global $wgInterwikiCache;
 40+ if ($wgInterwikiCache) {
 41+ return Interwiki::getInterwikiCached( $key );
 42+ }
 43+ $iw = new Interwiki;
 44+ $iw->load( $prefix );
 45+ if( self::CACHE_LIMIT && count( self::$smCache ) >= self::CACHE_LIMIT ){
 46+ array_shift( self::$smCache );
 47+ }
 48+ self::$smCache[$prefix] = &$iw;
 49+ return $iw;
 50+ }
 51+
 52+ /**
 53+ * Fetch interwiki prefix data from local cache in constant database.
 54+ *
 55+ * @note More logic is explained in DefaultSettings.
 56+ *
 57+ * @param $key \type{\string} Database key
 58+ * @return \type{\string} URL of interwiki site
 59+ */
 60+ protected static function getInterwikiCached( $key ) {
 61+ global $wgInterwikiCache, $wgInterwikiScopes, $wgInterwikiFallbackSite;
 62+ static $db, $site;
 63+
 64+ if (!$db)
 65+ $db=dba_open($wgInterwikiCache,'r','cdb');
 66+ /* Resolve site name */
 67+ if ($wgInterwikiScopes>=3 and !$site) {
 68+ $site = dba_fetch('__sites:' . wfWikiID(), $db);
 69+ if ($site=="")
 70+ $site = $wgInterwikiFallbackSite;
 71+ }
 72+ $value = dba_fetch( wfMemcKey( $key ), $db);
 73+ if ($value=='' and $wgInterwikiScopes>=3) {
 74+ /* try site-level */
 75+ $value = dba_fetch("_{$site}:{$key}", $db);
 76+ }
 77+ if ($value=='' and $wgInterwikiScopes>=2) {
 78+ /* try globals */
 79+ $value = dba_fetch("__global:{$key}", $db);
 80+ }
 81+ if ($value=='undef')
 82+ $value='';
 83+ $s = new Interwiki( $key );
 84+ if ( $value != '' ) {
 85+ list( $local, $url ) = explode( ' ', $value, 2 );
 86+ $s->mURL = $url;
 87+ $s->mLocal = (int)$local;
 88+ }
 89+ if( self::CACHE_LIMIT && count( self::$smCache ) >= self::CACHE_LIMIT ){
 90+ array_shift( self::$smCache );
 91+ }
 92+ self::$smCache[$prefix] = &$s;
 93+ return $s;
 94+ }
 95+
 96+ /**
 97+ * Clear all member variables in the current object. Does not clear
 98+ * the block from the DB.
 99+ */
 100+ function clear() {
 101+ $this->mURL = '';
 102+ $this->mLocal = $this->mTrans = 0;
 103+ $this->mPrefix = null;
 104+ }
 105+
 106+ /**
 107+ * Get the DB object
 108+ *
 109+ * @return Database
 110+ */
 111+ function &getDB(){
 112+ $db = wfGetDB( DB_SLAVE );
 113+ return $db;
 114+ }
 115+
 116+ /**
 117+ * Load interwiki from the DB
 118+ *
 119+ * @param $prefix The interwiki prefix
 120+ * @return bool The prefix is valid
 121+ *
 122+ */
 123+ function load( $prefix ) {
 124+ global $wgMemc;
 125+ $key = wfMemcKey( 'interwiki', $prefix );
 126+ $mc = $wgMemc->get( $key );
 127+ if( $mc ){
 128+ if( $this->loadFromArray( $mc ) ){
 129+ wfDebug("Succeeded\n");
 130+ return true;
 131+ }
 132+ }else{
 133+ $db =& $this->getDB();
 134+
 135+ $res = $db->resultObject( $db->select( 'interwiki', '*', array( 'iw_prefix' => $prefix ),
 136+ __METHOD__ ) );
 137+ if ( $this->loadFromResult( $res ) ) {
 138+ $mc = array( 'url' => $this->mURL, 'local' => $this->mLocal, 'trans' => $this->mTrans );
 139+ $wgMemc->add( $key, $mc );
 140+ return true;
 141+ }
 142+ }
 143+
 144+ # Give up
 145+ $this->clear();
 146+ return false;
 147+ }
 148+
 149+ /**
 150+ * Fill in member variables from an array (e.g. memcached result)
 151+ *
 152+ * @return bool Whether everything was there
 153+ * @param $res ResultWrapper Row from the interwiki table
 154+ */
 155+ function loadFromArray( $mc ) {
 156+ if( isset( $mc['url'] ) && isset( $mc['local'] ) && isset( $mc['trans'] ) ){
 157+ $this->mURL = $mc['url'];
 158+ $this->mLocal = $mc['local'];
 159+ $this->mTrans = $mc['trans'];
 160+ return true;
 161+ }
 162+ return false;
 163+ }
 164+
 165+ /**
 166+ * Fill in member variables from a result wrapper
 167+ *
 168+ * @return bool Whether there was a row there
 169+ * @param $res ResultWrapper Row from the interwiki table
 170+ */
 171+ function loadFromResult( ResultWrapper $res ) {
 172+ $ret = false;
 173+ if ( 0 != $res->numRows() ) {
 174+ # Get first entry
 175+ $row = $res->fetchObject();
 176+ $this->initFromRow( $row );
 177+ $ret = true;
 178+ }
 179+ $res->free();
 180+ return $ret;
 181+ }
 182+
 183+ /**
 184+ * Given a database row from the interwiki table, initialize
 185+ * member variables
 186+ *
 187+ * @param $row ResultWrapper A row from the interwiki table
 188+ */
 189+ function initFromRow( $row ) {
 190+ $this->mPrefix = $row->iw_prefix;
 191+ $this->mURL = $row->iw_url;
 192+ $this->mLocal = $row->iw_local;
 193+ $this->mTrans = $row->iw_trans;
 194+ }
 195+
 196+ /**
 197+ * Get the URL for a particular title (or with $1 if no title given)
 198+ *
 199+ * @param $title string What text to put for the article name
 200+ * @return string The URL
 201+ */
 202+ function getURL( $title = null ){
 203+ $url = $this->mURL;
 204+ if( $title != null ){
 205+ $url = str_replace( "$1", $title, $url );
 206+ }
 207+ return $url;
 208+ }
 209+
 210+ function isLocal(){
 211+ return $this->mLocal;
 212+ }
 213+
 214+ function isTranscludable(){
 215+ return $this->mTrans;
 216+ }
 217+
 218+}
Index: trunk/phase3/config/index.php
@@ -1702,7 +1702,7 @@
17031703 ## you can enable inline LaTeX equations:
17041704 \$wgUseTeX = false;
17051705
1706 -\$wgLocalInterwiki = \$wgSitename;
 1706+\$wgLocalInterwiki = strtolower( \$wgSitename );
17071707
17081708 \$wgLanguageCode = \"{$slconf['LanguageCode']}\";
17091709

Follow-up revisions

RevisionCommit summaryAuthorDate
r41378Fix r41377 - Incompatible old keys caused Fatal Errormattj10:14, 29 September 2008
r50952Remove deprecated getInterwikiLink(). Method was deprecated in r41377 (2008-0...siebrand09:05, 24 May 2009

Comments

#Comment by Tim Starling (talk | contribs)   03:00, 10 October 2008

Needs the following work done:

  • array_shift() is O(N) due to the need to scan the array for numeric keys to renumber them. Use reset/key like I did in the code you're replacing.
  • Implement negative caching
  • Don't use references e.g. on lines 51, 78 and 122. They don't provide a significant performance benefit since object assignment is a kind of shallow copy anyway, and they often cause bugs when filling arrays due to subsequent reuse of variables, as in:
foreach ( $stuff as $key => $item ) {
   $realItem = $this->processItem( $item );
   $array[$key] =& $realItem;
}

Broken because every element in the array is now bound to $realItem, so every item has the same value as the last call to processItem().

  • Fix insufficiently spacey code in getInterwikiCached(). I know you're copying it but it's a good opportunity.
  • Remove Interwiki::clear() as previously discussed. Move all functionality in load() to a static factory function and have it return false on failure. Same with loadFromArray(), loadFromRow().
  • Remove initFromRow(), use the constructor from the factory functions
  • Remove Interwiki::getDB(), unnecessary indirection hinders readability. Use wfGetDB() directly.
  • Don't use references on line 201, PHP 4-ism, fix it in the code you copied from as well
  • $db->resultObject() is obsolete, remove it here and where you copied from
  • Remove calls to $res->free(), established to be unnecessary

Next time use some decent modern code like filerepo as your model, instead of ancient legacy code like Block.php.

#Comment by Brion VIBBER (talk | contribs)   21:42, 15 October 2008

Issues resolved in r41569, r42022, r42023, r42041, r42046 follow-ups.

Status & tagging log