jenkins-bot has submitted this change and it was merged. ( https://gerrit.wikimedia.org/r/350281 )
Change subject: Interwiki: Don't override interwiki map order ...................................................................... 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 --- M includes/interwiki/ClassicInterwikiLookup.php M tests/phpunit/includes/interwiki/ClassicInterwikiLookupTest.php M tests/phpunit/includes/interwiki/InterwikiTest.php 3 files changed, 51 insertions(+), 158 deletions(-) Approvals: jenkins-bot: Verified Jforrester: Looks good to me, approved diff --git a/includes/interwiki/ClassicInterwikiLookup.php b/includes/interwiki/ClassicInterwikiLookup.php index 5226aa0..d9c0424 100644 --- a/includes/interwiki/ClassicInterwikiLookup.php +++ b/includes/interwiki/ClassicInterwikiLookup.php @@ -383,8 +383,6 @@ . $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 --- a/tests/phpunit/includes/interwiki/ClassicInterwikiLookupTest.php +++ b/tests/phpunit/includes/interwiki/ClassicInterwikiLookupTest.php @@ -133,16 +133,16 @@ // 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( @@ -160,7 +160,7 @@ ); $this->assertEquals( - [ $dewiki, $zzwiki ], + [ $zzwiki, $dewiki ], $lookup->getAllPrefixes(), 'getAllPrefixes()' ); @@ -185,16 +185,15 @@ } 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( @@ -212,7 +211,7 @@ ); $this->assertEquals( - [ $dewiki, $zzwiki ], + [ $zzwiki, $dewiki ], $lookup->getAllPrefixes(), 'getAllPrefixes()' ); @@ -233,4 +232,42 @@ $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 --- a/tests/phpunit/includes/interwiki/InterwikiTest.php +++ b/tests/phpunit/includes/interwiki/InterwikiTest.php @@ -119,146 +119,4 @@ $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' ); - } - } -- To view, visit https://gerrit.wikimedia.org/r/350281 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I7348748801cbdf16c6ceea5b0654fc174b79707e Gerrit-PatchSet: 7 Gerrit-Project: mediawiki/core Gerrit-Branch: master Gerrit-Owner: Krinkle <krinklem...@gmail.com> Gerrit-Reviewer: Jforrester <jforres...@wikimedia.org> Gerrit-Reviewer: Krinkle <krinklem...@gmail.com> Gerrit-Reviewer: Legoktm <lego...@member.fsf.org> Gerrit-Reviewer: Subramanya Sastry <ssas...@wikimedia.org> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits