jenkins-bot has submitted this change and it was merged.
Change subject: More sensible behavior when special page aliases conflict
......................................................................
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
---
M includes/specialpage/SpecialPageFactory.php
M languages/Language.php
M tests/phpunit/includes/specialpage/SpecialPageFactoryTest.php
3 files changed, 197 insertions(+), 36 deletions(-)
Approvals:
Nikerabbit: Looks good to me, approved
jenkins-bot: Verified
diff --git a/includes/specialpage/SpecialPageFactory.php
b/includes/specialpage/SpecialPageFactory.php
index 48bcb77..7904140 100644
--- a/includes/specialpage/SpecialPageFactory.php
+++ b/includes/specialpage/SpecialPageFactory.php
@@ -262,10 +262,9 @@
/**
* 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,20 +272,43 @@
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
@@ -620,29 +642,42 @@
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 bf2e3a3..b985077 100644
--- a/languages/Language.php
+++ b/languages/Language.php
@@ -737,6 +737,8 @@
}
/**
+ * @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 d56ecad..4619c2e 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 @@
*/
public function testGetPage( $spec, $shouldReuseInstance ) {
$this->mergeMwGlobalArrayValue( 'wgSpecialPages', array(
'testdummy' => $spec ) );
-
SpecialPageFactory::resetList();
$page = SpecialPageFactory::getPage( 'testdummy' );
@@ -50,8 +55,6 @@
$page2 = SpecialPageFactory::getPage( 'testdummy' );
$this->assertEquals( $shouldReuseInstance, $page2 === $page,
"Should re-use instance:" );
-
- SpecialPageFactory::resetList();
}
/**
@@ -59,12 +62,11 @@
*/
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 @@
*/
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 @@
*/
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 @@
*/
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,
+ ),
+
+ );
}
}
--
To view, visit https://gerrit.wikimedia.org/r/161499
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I4b17ec0fdc87b4b0d7ae9d9eea7ffacb54dd6891
Gerrit-PatchSet: 5
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Anomie <bjor...@wikimedia.org>
Gerrit-Reviewer: Anomie <bjor...@wikimedia.org>
Gerrit-Reviewer: Nikerabbit <niklas.laxst...@gmail.com>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
