From fcbde8ae4ec6890eca9cebcce6db3fc22c8a20d8 Mon Sep 17 00:00:00 2001 From: "C. Scott Ananian" Date: Fri, 19 Oct 2018 15:00:52 -0400 Subject: [PATCH] Make Language::hasVariant() more strict In d59f27aeab08b171e5ab6a081e763a4cad0bca04 we made LanguageConverter::validateVariant() try harder to convert a variant into an acceptable MediaWiki-internal form, looking at deprecated codes and BCP 47 aliases. However, this misled Language::hasVariant() into thinking that bogus names (like all-uppercase strings) were acceptable variant names, which then led exceptions when they were passed to the various conversion methods. This is a belt-and-suspenders patch for T207433 -- in that case we shouldn't have created a Language object with code 'sr-cyrl' in the first place, but once one was created we shouldn't have tried to ask LanguageSr to convert texts to 'sr-cyrl'. The latter problem is fixed by this patch. Bug: T207433 Change-Id: Id993bc7989144b5031a551662e8e492bd23f698a --- languages/Language.php | 7 ++- languages/LanguageConverter.php | 8 ++- tests/phpunit/languages/LanguageTest.php | 13 +++++ .../languages/classes/LanguageSrTest.php | 57 +++++++++++++++++++ 4 files changed, 81 insertions(+), 4 deletions(-) diff --git a/languages/Language.php b/languages/Language.php index e75ea1a194..44c4ece335 100644 --- a/languages/Language.php +++ b/languages/Language.php @@ -4263,14 +4263,17 @@ class Language { } /** - * Check if the language has the specific variant + * Strict check if the language has the specific variant. + * + * Compare to LanguageConverter::validateVariant() which does a more + * lenient check and attempts to coerce the given code to a valid one. * * @since 1.19 * @param string $variant * @return bool */ public function hasVariant( $variant ) { - return (bool)$this->mConverter->validateVariant( $variant ); + return $variant && ( $variant === $this->mConverter->validateVariant( $variant ) ); } /** diff --git a/languages/LanguageConverter.php b/languages/LanguageConverter.php index ea26c64dc7..21902af0a6 100644 --- a/languages/LanguageConverter.php +++ b/languages/LanguageConverter.php @@ -212,9 +212,13 @@ class LanguageConverter { } /** - * Validate the variant + * Validate the variant and return an appropriate strict internal + * variant code if one exists. Compare to Language::hasVariant() + * which does a strict test. + * * @param string|null $variant The variant to validate - * @return mixed Returns the variant if it is valid, null otherwise + * @return mixed Returns an equivalent valid variant code if possible, + * null otherwise */ public function validateVariant( $variant = null ) { if ( $variant === null ) { diff --git a/tests/phpunit/languages/LanguageTest.php b/tests/phpunit/languages/LanguageTest.php index e828e3fa83..dca1363885 100644 --- a/tests/phpunit/languages/LanguageTest.php +++ b/tests/phpunit/languages/LanguageTest.php @@ -1878,6 +1878,19 @@ class LanguageTest extends LanguageClassesTestCase { ]; } + /** + * @covers Language::hasVariant + */ + public function testHasVariant() { + // See LanguageSrTest::testHasVariant() for additional tests + $en = Language::factory( 'en' ); + $this->assertTrue( $en->hasVariant( 'en' ), 'base is always a variant' ); + $this->assertFalse( $en->hasVariant( 'en-bogus' ), 'bogus en variant' ); + + $bogus = Language::factory( 'bogus' ); + $this->assertTrue( $bogus->hasVariant( 'bogus' ), 'base is always a variant' ); + } + /** * @covers Language::equals */ diff --git a/tests/phpunit/languages/classes/LanguageSrTest.php b/tests/phpunit/languages/classes/LanguageSrTest.php index b846c56daa..c9f2f3edf7 100644 --- a/tests/phpunit/languages/classes/LanguageSrTest.php +++ b/tests/phpunit/languages/classes/LanguageSrTest.php @@ -21,6 +21,63 @@ * @covers SrConverter */ class LanguageSrTest extends LanguageClassesTestCase { + /** + * @covers Language::hasVariants + */ + public function testHasVariants() { + $this->assertTrue( $this->getLang()->hasVariants(), 'sr has variants' ); + } + + /** + * @covers Language::hasVariant + */ + public function testHasVariant() { + $langs = [ + 'sr' => $this->getLang(), + 'sr-ec' => Language::factory( 'sr-ec' ), + 'sr-cyrl' => Language::factory( 'sr-cyrl' ), + ]; + foreach ( $langs as $code => $l ) { + $p = $l->getParentLanguage(); + $this->assertTrue( $p !== null, 'parent language exists' ); + $this->assertEquals( 'sr', $p->getCode(), 'sr is parent language' ); + $this->assertTrue( $p instanceof LanguageSr, 'parent is LanguageSr' ); + // This is a valid variant of the base + $this->assertTrue( $p->hasVariant( $l->getCode() ) ); + // This test should be tweaked if/when sr-ec is renamed (T117845) + // to swap the roles of sr-ec and sr-Cyrl + $this->assertTrue( $l->hasVariant( 'sr-ec' ), 'sr-ec exists' ); + // note that sr-cyrl is an alias, not a (strict) variant name + foreach ( [ 'sr-EC', 'sr-Cyrl', 'sr-cyrl', 'sr-bogus' ] as $v ) { + $this->assertFalse( $l->hasVariant( $v ), "$v is not a variant of $code" ); + } + } + } + + /** + * @covers Language::hasVariant + */ + public function testHasVariantBogus() { + $langs = [ + // Note that case matters when calling Language::factory(); + // these are all bogus language codes + 'sr-EC' => Language::factory( 'sr-EC' ), + 'sr-Cyrl' => Language::factory( 'sr-Cyrl' ), + 'sr-bogus' => Language::factory( 'sr-bogus' ), + ]; + foreach ( $langs as $code => $l ) { + $p = $l->getParentLanguage(); + $this->assertTrue( $p === null, 'no parent for bogus language' ); + $this->assertFalse( $l instanceof LanguageSr, "$code is not sr" ); + $this->assertFalse( $this->getLang()->hasVariant( $code ), "$code is not a sr variant" ); + foreach ( [ 'sr', 'sr-ec', 'sr-EC', 'sr-Cyrl', 'sr-cyrl', 'sr-bogus' ] as $v ) { + if ( $v !== $code ) { + $this->assertFalse( $l->hasVariant( $v ), "no variant $v" ); + } + } + } + } + /** * @covers LanguageConverter::convertTo */ -- 2.20.1