Interwiki: Don't override interwiki map order

The ksort() here was causing the order to be enforced as
alphabetical instead of preserving the original order.

The order usually doesn't matter, except with regards to handling
of duplicates. Due to Parsoid normalising external links to interwiki
links, it has to do a reverse lookup. In doing so it has to decide
which one to prefer. It currently picks the first match from the
API request for meta=siteinfo&siprop=interwikimap, which didn't
match the defined order in the actual Interwiki map due to ksort()
being called in getAllPrefixes().

Sort in this function was originally introduced in 2010 with
commit 844e7c83e4 (2011; r92528; T21838), which is otherwise unrelated
and left no rationale.

The existing unit tests needed to be adjusted slightly as they
assumed alphabetical order. While it appeared they were also defined
in alphabetical order, this was merely the order of the variable
creation. The effective order is preserved within locals and globals,
but overall globals come before locals.

Also removed the duplicate test for Hash and CDB in InterwikiTest
that belongs in ClassicInterwikiLookupTest instead.

Bug: T145337
Change-Id: I7348748801cbdf16c6ceea5b0654fc174b79707e

diff --git a/includes/interwiki/ClassicInterwikiLookup.php b/includes/interwiki/ClassicInterwikiLookup.php
index 5226aa0..d9c0424 100644 (file)
--- a/includes/interwiki/ClassicInterwikiLookup.php
+++ b/includes/interwiki/ClassicInterwikiLookup.php
@@ -383,8 +383,6 @@ class ClassicInterwikiLookup implements InterwikiLookup {
                                . $e->getMessage() );
                }
 
-               ksort( $data );
-
                return array_values( $data );
        }
 

diff --git a/tests/phpunit/includes/interwiki/ClassicInterwikiLookupTest.php b/tests/phpunit/includes/interwiki/ClassicInterwikiLookupTest.php
index db6d002..48310a9 100644 (file)
--- a/tests/phpunit/includes/interwiki/ClassicInterwikiLookupTest.php
+++ b/tests/phpunit/includes/interwiki/ClassicInterwikiLookupTest.php
@@ -133,18 +133,18 @@ class ClassicInterwikiLookupTest extends MediaWikiTestCase {
                // NOTE: CDB setup is expensive, so we only do
                //  it once and run all the tests in one go.
 
-               $dewiki = [
-                       'iw_prefix' => 'de',
-                       'iw_url' => 'http://de.wikipedia.org/wiki/',
-                       'iw_local' => 1
-               ];
-
                $zzwiki = [
                        'iw_prefix' => 'zz',
                        'iw_url' => 'http://zzwiki.org/wiki/',
                        'iw_local' => 0
                ];
 
+               $dewiki = [
+                       'iw_prefix' => 'de',
+                       'iw_url' => 'http://de.wikipedia.org/wiki/',
+                       'iw_local' => 1
+               ];
+
                $cdbFile = $this->populateCDB(
                        'en',
                        [ $dewiki ],
@@ -160,7 +160,7 @@ class ClassicInterwikiLookupTest extends MediaWikiTestCase {
                );
 
                $this->assertEquals(
-                       [ $dewiki, $zzwiki ],
+                       [ $zzwiki, $dewiki ],
                        $lookup->getAllPrefixes(),
                        'getAllPrefixes()'
                );
@@ -185,17 +185,16 @@ class ClassicInterwikiLookupTest extends MediaWikiTestCase {
        }
 
        public function testArrayStorage() {
-               $dewiki = [
-                       'iw_prefix' => 'de',
-                       'iw_url' => 'http://de.wikipedia.org/wiki/',
-                       'iw_local' => 1
-               ];
-
                $zzwiki = [
                        'iw_prefix' => 'zz',
                        'iw_url' => 'http://zzwiki.org/wiki/',
                        'iw_local' => 0
                ];
+               $dewiki = [
+                       'iw_prefix' => 'de',
+                       'iw_url' => 'http://de.wikipedia.org/wiki/',
+                       'iw_local' => 1
+               ];
 
                $hash = $this->populateHash(
                        'en',
@@ -212,7 +211,7 @@ class ClassicInterwikiLookupTest extends MediaWikiTestCase {
                );
 
                $this->assertEquals(
-                       [ $dewiki, $zzwiki ],
+                       [ $zzwiki, $dewiki ],
                        $lookup->getAllPrefixes(),
                        'getAllPrefixes()'
                );
@@ -233,4 +232,42 @@ class ClassicInterwikiLookupTest extends MediaWikiTestCase {
                $this->assertSame( false, $interwiki->isLocal(), 'isLocal' );
        }
 
+       public function testGetAllPrefixes() {
+               $zz = [
+                       'iw_prefix' => 'zz',
+                       'iw_url' => 'https://azz.example.org/',
+                       'iw_local' => 1
+               ];
+               $de = [
+                       'iw_prefix' => 'de',
+                       'iw_url' => 'https://de.example.org/',
+                       'iw_local' => 1
+               ];
+               $azz = [
+                       'iw_prefix' => 'azz',
+                       'iw_url' => 'https://azz.example.org/',
+                       'iw_local' => 1
+               ];
+
+               $hash = $this->populateHash(
+                       'en',
+                       [],
+                       [ $zz, $de, $azz ]
+               );
+               $lookup = new \MediaWiki\Interwiki\ClassicInterwikiLookup(
+                       Language::factory( 'en' ),
+                       WANObjectCache::newEmpty(),
+                       60 * 60,
+                       $hash,
+                       3,
+                       'en'
+               );
+
+               $this->assertEquals(
+                       [ $zz, $de, $azz ],
+                       $lookup->getAllPrefixes(),
+                       'getAllPrefixes() - preserves order'
+               );
+       }
+
 }

diff --git a/tests/phpunit/includes/interwiki/InterwikiTest.php b/tests/phpunit/includes/interwiki/InterwikiTest.php
index b1ad77a..22b1304 100644 (file)
--- a/tests/phpunit/includes/interwiki/InterwikiTest.php
+++ b/tests/phpunit/includes/interwiki/InterwikiTest.php
@@ -119,146 +119,4 @@ class InterwikiTest extends MediaWikiTestCase {
                $this->assertNotSame( $interwiki, $interwikiLookup->fetch( 'de' ), 'invalidate cache' );
        }
 
-       /**
-        * @param string $thisSite
-        * @param string[] $local
-        * @param string[] $global
-        *
-        * @return string[]
-        */
-       private function populateHash( $thisSite, $local, $global ) {
-               $hash = [];
-               $hash[ '__sites:' . wfWikiID() ] = $thisSite;
-
-               $globals = [];
-               $locals = [];
-
-               foreach ( $local as $row ) {
-                       $prefix = $row['iw_prefix'];
-                       $data = $row['iw_local'] . ' ' . $row['iw_url'];
-                       $locals[] = $prefix;
-                       $hash[ "_{$thisSite}:{$prefix}" ] = $data;
-               }
-
-               foreach ( $global as $row ) {
-                       $prefix = $row['iw_prefix'];
-                       $data = $row['iw_local'] . ' ' . $row['iw_url'];
-                       $globals[] = $prefix;
-                       $hash[ "__global:{$prefix}" ] = $data;
-               }
-
-               $hash[ '__list:__global' ] = implode( ' ', $globals );
-               $hash[ '__list:_' . $thisSite ] = implode( ' ', $locals );
-
-               return $hash;
-       }
-
-       private function populateCDB( $thisSite, $local, $global ) {
-               $cdbFile = tempnam( wfTempDir(), 'MW-ClassicInterwikiLookupTest-' ) . '.cdb';
-               $cdb = CdbWriter::open( $cdbFile );
-
-               $hash = $this->populateHash( $thisSite, $local, $global );
-
-               foreach ( $hash as $key => $value ) {
-                       $cdb->set( $key, $value );
-               }
-
-               $cdb->close();
-               return $cdbFile;
-       }
-
-       public function testCDBStorage() {
-               // NOTE: CDB setup is expensive, so we only do
-               //  it once and run all the tests in one go.
-
-               $dewiki = [
-                       'iw_prefix' => 'de',
-                       'iw_url' => 'http://de.wikipedia.org/wiki/',
-                       'iw_local' => 1
-               ];
-
-               $zzwiki = [
-                       'iw_prefix' => 'zz',
-                       'iw_url' => 'http://zzwiki.org/wiki/',
-                       'iw_local' => 0
-               ];
-
-               $cdbFile = $this->populateCDB(
-                       'en',
-                       [ $dewiki ],
-                       [ $zzwiki ]
-               );
-
-               $this->setWgInterwikiCache( $cdbFile );
-
-               $interwikiLookup = MediaWikiServices::getInstance()->getInterwikiLookup();
-               $this->assertEquals(
-                       [ $dewiki, $zzwiki ],
-                       $interwikiLookup->getAllPrefixes(),
-                       'getAllPrefixes()'
-               );
-
-               $this->assertTrue( $interwikiLookup->isValidInterwiki( 'de' ), 'known prefix is valid' );
-               $this->assertTrue( $interwikiLookup->isValidInterwiki( 'zz' ), 'known prefix is valid' );
-
-               $interwiki = $interwikiLookup->fetch( 'de' );
-               $this->assertInstanceOf( 'Interwiki', $interwiki );
-
-               $this->assertSame( 'http://de.wikipedia.org/wiki/', $interwiki->getURL(), 'getURL' );
-               $this->assertSame( true, $interwiki->isLocal(), 'isLocal' );
-
-               $interwiki = $interwikiLookup->fetch( 'zz' );
-               $this->assertInstanceOf( 'Interwiki', $interwiki );
-
-               $this->assertSame( 'http://zzwiki.org/wiki/', $interwiki->getURL(), 'getURL' );
-               $this->assertSame( false, $interwiki->isLocal(), 'isLocal' );
-
-               // cleanup temp file
-               unlink( $cdbFile );
-       }
-
-       public function testArrayStorage() {
-               $dewiki = [
-                       'iw_prefix' => 'de',
-                       'iw_url' => 'http://de.wikipedia.org/wiki/',
-                       'iw_local' => 1
-               ];
-
-               $zzwiki = [
-                       'iw_prefix' => 'zz',
-                       'iw_url' => 'http://zzwiki.org/wiki/',
-                       'iw_local' => 0
-               ];
-
-               $cdbData = $this->populateHash(
-                       'en',
-                       [ $dewiki ],
-                       [ $zzwiki ]
-               );
-
-               $this->setWgInterwikiCache( $cdbData );
-
-               $interwikiLookup = MediaWikiServices::getInstance()->getInterwikiLookup();
-               $this->assertEquals(
-                       [ $dewiki, $zzwiki ],
-                       $interwikiLookup->getAllPrefixes(),
-                       'getAllPrefixes()'
-               );
-
-               $this->assertTrue( $interwikiLookup->isValidInterwiki( 'de' ), 'known prefix is valid' );
-               $this->assertTrue( $interwikiLookup->isValidInterwiki( 'zz' ), 'known prefix is valid' );
-
-               $interwiki = $interwikiLookup->fetch( 'de' );
-               $this->assertInstanceOf( 'Interwiki', $interwiki );
-
-               $this->assertSame( 'http://de.wikipedia.org/wiki/', $interwiki->getURL(), 'getURL' );
-               $this->assertSame( true, $interwiki->isLocal(), 'isLocal' );
-
-               $interwiki = $interwikiLookup->fetch( 'zz' );
-               $this->assertInstanceOf( 'Interwiki', $interwiki );
-
-               $this->assertSame( 'http://zzwiki.org/wiki/', $interwiki->getURL(), 'getURL' );
-               $this->assertSame( false, $interwiki->isLocal(), 'isLocal' );
-       }
-
 }