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

diff --git a/languages/Language.php b/languages/Language.php
index e75ea1a..44c4ece 100644 (file)
--- 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 ea26c64..21902af 100644 (file)
--- 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 e828e3f..dca1363 100644 (file)
--- 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 b846c56..c9f2f3e 100644 (file)
--- 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
         */