From ad522beeea5037ced24781df515b880cf75733ca Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Fri, 19 Sep 2014 14:32:43 -0400 Subject: [PATCH] More sensible behavior when special page aliases conflict Right now, SpecialPageFactory::getAliasListObject() just chooses the last-seen alias and allows any alias to completely override the page's "canonical" name (from SpecialPageFactory::$list or $wgSpecialPages). Although the latter doesn't come up often since (almost?) all special pages have their canonical name as one of their English-language aliases. More sensible behavior is to always prefer the "canonical" name over any conflicting aliases, and to prefer an alias that's the first alias for a special page over one that is a fallback. Also, when a special page's first alias winds up not actually referring to that special page, we MUST NOT go redirecting other names for that special page to that wrong alias. Bug: 70686 Change-Id: I4b17ec0fdc87b4b0d7ae9d9eea7ffacb54dd6891 --- includes/specialpage/SpecialPageFactory.php | 83 +++++++--- languages/Language.php | 2 + .../specialpage/SpecialPageFactoryTest.php | 148 ++++++++++++++++-- 3 files changed, 197 insertions(+), 36 deletions(-) diff --git a/includes/specialpage/SpecialPageFactory.php b/includes/specialpage/SpecialPageFactory.php index 48bcb77867..7904140e2f 100644 --- a/includes/specialpage/SpecialPageFactory.php +++ b/includes/specialpage/SpecialPageFactory.php @@ -262,10 +262,9 @@ class SpecialPageFactory { /** * Initialise and return the list of special page aliases. Returns an object with - * properties which can be accessed $obj->pagename - each property is an array of - * aliases; the first in the array is the canonical alias. All registered special - * pages are guaranteed to have a property entry, and for that property array to - * contain at least one entry (English fallbacks will be added if necessary). + * properties which can be accessed $obj->pagename - each property name is an + * alias, with the value being the canonical name of the special page. All + * registered special pages are guaranteed to map to themselves. * @return object */ private static function getAliasListObject() { @@ -273,21 +272,44 @@ class SpecialPageFactory { global $wgContLang; $aliases = $wgContLang->getSpecialPageAliases(); - $missingPages = self::getPageList(); - self::$aliases = array(); + $keepAlias = array(); + + // Force every canonical name to be an alias for itself. + foreach ( self::getPageList() as $name => $stuff ) { + $caseFoldedAlias = $wgContLang->caseFold( $name ); + self::$aliases[$caseFoldedAlias] = $name; + $keepAlias[$caseFoldedAlias] = 'canonical'; + } + // Check for $aliases being an array since Language::getSpecialPageAliases can return null if ( is_array( $aliases ) ) { foreach ( $aliases as $realName => $aliasList ) { - foreach ( $aliasList as $alias ) { - self::$aliases[$wgContLang->caseFold( $alias )] = $realName; + $aliasList = array_values( $aliasList ); + foreach ( $aliasList as $i => $alias ) { + $caseFoldedAlias = $wgContLang->caseFold( $alias ); + + if ( isset( self::$aliases[$caseFoldedAlias] ) && + $realName === self::$aliases[$caseFoldedAlias] + ) { + // Ignore same-realName conflicts + continue; + } + + if ( !isset( $keepAlias[$caseFoldedAlias] ) ) { + self::$aliases[$caseFoldedAlias] = $realName; + if ( !$i ) { + $keepAlias[$caseFoldedAlias] = 'first'; + } + } elseif ( !$i ) { + wfWarn( "First alias '$alias' for $realName conflicts with " . + "{$keepAlias[$caseFoldedAlias]} alias for " . + self::$aliases[$caseFoldedAlias] + ); + } } - unset( $missingPages->$realName ); } } - foreach ( $missingPages as $name => $stuff ) { - self::$aliases[$wgContLang->caseFold( $name )] = $name; - } // Cast to object: func()[$key] doesn't work, but func()->$key does self::$aliases = (object)self::$aliases; @@ -620,29 +642,42 @@ class SpecialPageFactory { public static function getLocalNameFor( $name, $subpage = false ) { global $wgContLang; $aliases = $wgContLang->getSpecialPageAliases(); + $aliasList = self::getAliasListObject(); - if ( isset( $aliases[$name][0] ) ) { - $name = $aliases[$name][0]; - } else { - // Try harder in case someone misspelled the correct casing + // Find the first alias that maps back to $name + if ( isset( $aliases[$name] ) ) { $found = false; - // Check for $aliases being an array since Language::getSpecialPageAliases can return null + foreach ( $aliases[$name] as $alias ) { + $caseFoldedAlias = $wgContLang->caseFold( $alias ); + $caseFoldedAlias = str_replace( ' ', '_', $caseFoldedAlias ); + if ( isset( $aliasList->$caseFoldedAlias ) && + $aliasList->$caseFoldedAlias === $name + ) { + $name = $alias; + $found = true; + break; + } + } + if ( !$found ) { + wfWarn( "Did not find a usable alias for special page '$name'. " . + "It seems all defined aliases conflict?" ); + } + } else { + // Check if someone misspelled the correct casing if ( is_array( $aliases ) ) { foreach ( $aliases as $n => $values ) { if ( strcasecmp( $name, $n ) === 0 ) { wfWarn( "Found alias defined for $n when searching for " . "special page aliases for $name. Case mismatch?" ); - $name = $values[0]; - $found = true; - break; + return self::getLocalNameFor( $n, $subpage ); } } } - if ( !$found ) { - wfWarn( "Did not find alias for special page '$name'. " . - "Perhaps no aliases are defined for it?" ); - } + + wfWarn( "Did not find alias for special page '$name'. " . + "Perhaps no aliases are defined for it?" ); } + if ( $subpage !== false && !is_null( $subpage ) ) { $name = "$name/$subpage"; } diff --git a/languages/Language.php b/languages/Language.php index bf2e3a399b..b985077d82 100644 --- a/languages/Language.php +++ b/languages/Language.php @@ -737,6 +737,8 @@ class Language { } /** + * @deprecated since 1.24, doesn't handle conflicting aliases. Use + * SpecialPageFactory::getLocalNameFor instead. * @param string $name * @return string */ diff --git a/tests/phpunit/includes/specialpage/SpecialPageFactoryTest.php b/tests/phpunit/includes/specialpage/SpecialPageFactoryTest.php index d56ecad1d3..4619c2ed51 100644 --- a/tests/phpunit/includes/specialpage/SpecialPageFactoryTest.php +++ b/tests/phpunit/includes/specialpage/SpecialPageFactoryTest.php @@ -22,6 +22,12 @@ */ class SpecialPageFactoryTest extends MediaWikiTestCase { + protected function tearDown() { + parent::tearDown(); + + SpecialPageFactory::resetList(); + } + public function newSpecialAllPages() { return new SpecialAllPages(); } @@ -42,7 +48,6 @@ class SpecialPageFactoryTest extends MediaWikiTestCase { */ public function testGetPage( $spec, $shouldReuseInstance ) { $this->mergeMwGlobalArrayValue( 'wgSpecialPages', array( 'testdummy' => $spec ) ); - SpecialPageFactory::resetList(); $page = SpecialPageFactory::getPage( 'testdummy' ); @@ -50,8 +55,6 @@ class SpecialPageFactoryTest extends MediaWikiTestCase { $page2 = SpecialPageFactory::getPage( 'testdummy' ); $this->assertEquals( $shouldReuseInstance, $page2 === $page, "Should re-use instance:" ); - - SpecialPageFactory::resetList(); } /** @@ -59,12 +62,11 @@ class SpecialPageFactoryTest extends MediaWikiTestCase { */ public function testGetNames() { $this->mergeMwGlobalArrayValue( 'wgSpecialPages', array( 'testdummy' => 'SpecialAllPages' ) ); - SpecialPageFactory::resetList(); + $names = SpecialPageFactory::getNames(); $this->assertInternalType( 'array', $names ); $this->assertContains( 'testdummy', $names ); - SpecialPageFactory::resetList(); } /** @@ -72,14 +74,11 @@ class SpecialPageFactoryTest extends MediaWikiTestCase { */ public function testResolveAlias() { $this->setMwGlobals( 'wgContLang', Language::factory( 'de' ) ); - SpecialPageFactory::resetList(); list( $name, $param ) = SpecialPageFactory::resolveAlias( 'Spezialseiten/Foo' ); $this->assertEquals( 'Specialpages', $name ); $this->assertEquals( 'Foo', $param ); - - SpecialPageFactory::resetList(); } /** @@ -87,13 +86,10 @@ class SpecialPageFactoryTest extends MediaWikiTestCase { */ public function testGetLocalNameFor() { $this->setMwGlobals( 'wgContLang', Language::factory( 'de' ) ); - SpecialPageFactory::resetList(); $name = SpecialPageFactory::getLocalNameFor( 'Specialpages', 'Foo' ); $this->assertEquals( 'Spezialseiten/Foo', $name ); - - SpecialPageFactory::resetList(); } /** @@ -101,14 +97,142 @@ class SpecialPageFactoryTest extends MediaWikiTestCase { */ public function testGetTitleForAlias() { $this->setMwGlobals( 'wgContLang', Language::factory( 'de' ) ); - SpecialPageFactory::resetList(); $title = SpecialPageFactory::getTitleForAlias( 'Specialpages/Foo' ); $this->assertEquals( 'Spezialseiten/Foo', $title->getText() ); $this->assertEquals( NS_SPECIAL, $title->getNamespace() ); + } + /** + * @dataProvider provideTestConflictResolution + */ + public function testConflictResolution( + $test, $aliasesList, $alias, $expectedName, $expectedAlias, $expectWarnings + ) { + global $wgContLang; + $lang = clone $wgContLang; + $lang->mExtendedSpecialPageAliases = $aliasesList; + $this->setMwGlobals( 'wgContLang', $lang ); + $this->setMwGlobals( 'wgSpecialPages', + array_combine( array_keys( $aliasesList ), array_keys( $aliasesList ) ) + ); SpecialPageFactory::resetList(); + + // Catch the warnings we expect to be raised + $warnings = array(); + $this->setMwGlobals( 'wgDevelopmentWarnings', true ); + set_error_handler( function ( $errno, $errstr ) use ( &$warnings ) { + if ( preg_match( '/First alias \'[^\']*\' for .*/', $errstr ) || + preg_match( '/Did not find a usable alias for special page .*/', $errstr ) + ) { + $warnings[] = $errstr; + return true; + } + return false; + } ); + $reset = new ScopedCallback( 'restore_error_handler' ); + + list( $name, /*...*/ ) = SpecialPageFactory::resolveAlias( $alias ); + $this->assertEquals( $expectedName, $name, "$test: Alias to name" ); + $result = SpecialPageFactory::getLocalNameFor( $name ); + $this->assertEquals( $expectedAlias, $result, "$test: Alias to name to alias" ); + + $gotWarnings = count( $warnings ); + if ( $gotWarnings !== $expectWarnings ) { + $this->fail( "Expected $expectWarnings warning(s), but got $gotWarnings:\n" . + join( "\n", $warnings ) + ); + } + } + + /** + * @dataProvider provideTestConflictResolution + */ + public function testConflictResolutionReversed( + $test, $aliasesList, $alias, $expectedName, $expectedAlias, $expectWarnings + ) { + // Make sure order doesn't matter by reversing the list + $aliasesList = array_reverse( $aliasesList ); + return $this->testConflictResolution( + $test, $aliasesList, $alias, $expectedName, $expectedAlias, $expectWarnings + ); + } + + public function provideTestConflictResolution() { + return array( + array( + 'Canonical name wins', + array( 'Foo' => array( 'Foo', 'Bar' ), 'Baz' => array( 'Foo', 'BazPage', 'Baz2' ) ), + 'Foo', + 'Foo', + 'Foo', + 1, + ), + + array( + 'Doesn\'t redirect to a different special page\'s canonical name', + array( 'Foo' => array( 'Foo', 'Bar' ), 'Baz' => array( 'Foo', 'BazPage', 'Baz2' ) ), + 'Baz', + 'Baz', + 'BazPage', + 1, + ), + + array( + 'Canonical name wins even if not aliased', + array( 'Foo' => array( 'FooPage' ), 'Baz' => array( 'Foo', 'BazPage', 'Baz2' ) ), + 'Foo', + 'Foo', + 'FooPage', + 1, + ), + + array( + 'Doesn\'t redirect to a different special page\'s canonical name even if not aliased', + array( 'Foo' => array( 'FooPage' ), 'Baz' => array( 'Foo', 'BazPage', 'Baz2' ) ), + 'Baz', + 'Baz', + 'BazPage', + 1, + ), + + array( + 'First local name beats non-first', + array( 'First' => array( 'Foo' ), 'NonFirst' => array( 'Bar', 'Foo' ) ), + 'Foo', + 'First', + 'Foo', + 0, + ), + + array( + 'Doesn\'t redirect to a different special page\'s first alias', + array( + 'Foo' => array( 'Foo' ), + 'First' => array( 'Bar' ), + 'Baz' => array( 'Foo', 'Bar', 'BazPage', 'Baz2' ) + ), + 'Baz', + 'Baz', + 'BazPage', + 1, + ), + + array( + 'Doesn\'t redirect wrong even if all aliases conflict', + array( + 'Foo' => array( 'Foo' ), + 'First' => array( 'Bar' ), + 'Baz' => array( 'Foo', 'Bar' ) + ), + 'Baz', + 'Baz', + 'Baz', + 2, + ), + + ); } } -- 2.20.1