From 0548f1902cb308c70541ff0f4ffb2feb24d03066 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Tue, 25 Apr 2017 13:36:06 -0700 Subject: [PATCH] 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 --- includes/interwiki/ClassicInterwikiLookup.php | 2 - .../interwiki/ClassicInterwikiLookupTest.php | 65 ++++++-- .../includes/interwiki/InterwikiTest.php | 142 ------------------ 3 files changed, 51 insertions(+), 158 deletions(-) diff --git a/includes/interwiki/ClassicInterwikiLookup.php b/includes/interwiki/ClassicInterwikiLookup.php index 5226aa099b..d9c04240c8 100644 --- 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 db6d00295c..48310a9f41 100644 --- 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 b1ad77a7e5..22b1304b7a 100644 --- 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' ); - } - } -- 2.20.1