strip off subpages direct in GenderCache

LinkBatch can also give subpages to the GenderCache and therefor it is
easier to do it always in GenderCache, than in LinkBatch and Title

Add unit tests for GenderCache

Change-Id: Ia936ff8bb639a197b0b3a8e07c97a66edd57dd10

diff --git a/includes/Title.php b/includes/Title.php
index 1876d75..6764f8f 100644 (file)
--- a/includes/Title.php
+++ b/includes/Title.php
@@ -711,17 +711,9 @@ class Title {
                        }
                }
 
-               // Strip off subpages
-               $pagename = $this->getText();
-               if ( strpos( $pagename, '/' ) !== false ) {
-                       list( $username , ) = explode( '/', $pagename, 2 );
-               } else {
-                       $username = $pagename;
-               }
-
                if ( $wgContLang->needsGenderDistinction() &&
                                MWNamespace::hasGenderDistinction( $this->mNamespace ) ) {
-                       $gender = GenderCache::singleton()->getGenderOf( $username, __METHOD__ );
+                       $gender = GenderCache::singleton()->getGenderOf( $this->getText(), __METHOD__ );
                        return $wgContLang->getGenderNsText( $this->mNamespace, $gender );
                }
 

diff --git a/includes/cache/GenderCache.php b/includes/cache/GenderCache.php
index 2cc1061..c27ab00 100644 (file)
--- a/includes/cache/GenderCache.php
+++ b/includes/cache/GenderCache.php
@@ -48,7 +48,7 @@ class GenderCache {
                        $username = $username->getName();
                }
 
-               $username = strtr( $username, '_', ' ' );
+               $username = self::normalizeUsername( $username );
                if ( !isset( $this->cache[$username] ) ) {
 
                        if ( $this->misses >= $this->missLimit && $wgUser->getName() !== $username ) {
@@ -60,11 +60,7 @@ class GenderCache {
 
                        } else {
                                $this->misses++;
-                               if ( !User::isValidUserName( $username ) ) {
-                                       $this->cache[$username] = $this->getDefault();
-                               } else {
-                                       $this->doQuery( $username, $caller );
-                               }
+                               $this->doQuery( $username, $caller );
                        }
 
                }
@@ -86,7 +82,6 @@ class GenderCache {
                foreach ( $data as $ns => $pagenames ) {
                        if ( !MWNamespace::hasGenderDistinction( $ns ) ) continue;
                        foreach ( array_keys( $pagenames ) as $username ) {
-                               if ( isset( $this->cache[$username] ) ) continue;
                                $users[$username] = true;
                        }
                }
@@ -102,26 +97,28 @@ class GenderCache {
        public function doQuery( $users, $caller = '' ) {
                $default = $this->getDefault();
 
-               foreach ( (array) $users as $index => $value ) {
-                       $name = strtr( $value, '_', ' ' );
-                       if ( isset( $this->cache[$name] ) ) {
-                               // Skip users whose gender setting we already know
-                               unset( $users[$index] );
-                       } else {
-                               $users[$index] = $name;
+               $usersToCheck = array();
+               foreach ( (array) $users as $value ) {
+                       $name = self::normalizeUsername( $value );
+                       // Skip users whose gender setting we already know
+                       if ( !isset( $this->cache[$name] ) ) {
                                // For existing users, this value will be overwritten by the correct value
                                $this->cache[$name] = $default;
+                               // query only for valid names, which can be in the database
+                               if( User::isValidUserName( $name ) ) {
+                                       $usersToCheck[] = $name;
+                               }
                        }
                }
 
-               if ( count( $users ) === 0 ) {
+               if ( count( $usersToCheck ) === 0 ) {
                        return;
                }
 
                $dbr = wfGetDB( DB_SLAVE );
                $table = array( 'user', 'user_properties' );
                $fields = array( 'user_name', 'up_value' );
-               $conds = array( 'user_name' => $users );
+               $conds = array( 'user_name' => $usersToCheck );
                $joins = array( 'user_properties' =>
                        array( 'LEFT JOIN', array( 'user_id = up_user', 'up_property' => 'gender' ) ) );
 
@@ -129,11 +126,20 @@ class GenderCache {
                if ( strval( $caller ) !== '' ) {
                        $comment .= "/$caller";
                }
-               $res = $dbr->select( $table, $fields, $conds, $comment, $joins, $joins );
+               $res = $dbr->select( $table, $fields, $conds, $comment, array(), $joins );
 
                foreach ( $res as $row ) {
                        $this->cache[$row->user_name] = $row->up_value ? $row->up_value : $default;
                }
        }
 
+       private static function normalizeUsername( $username ) {
+               // Strip off subpages
+               $indexSlash = strpos( $username, '/' );
+               if ( $indexSlash !== false ) {
+                       $username = substr( $username, 0, $indexSlash );
+               }
+               // normalize underscore/spaces
+               return strtr( $username, '_', ' ' );
+       }
 }

diff --git a/includes/cache/LinkBatch.php b/includes/cache/LinkBatch.php
index 29fde03..ab33dcf 100644 (file)
--- a/includes/cache/LinkBatch.php
+++ b/includes/cache/LinkBatch.php
@@ -195,7 +195,7 @@ class LinkBatch {
                }
 
                $genderCache = GenderCache::singleton();
-               $genderCache->dolinkBatch( $this->data, $this->caller );
+               $genderCache->doLinkBatch( $this->data, $this->caller );
                return true;
        }
 

diff --git a/tests/phpunit/includes/cache/GenderCacheTest.php b/tests/phpunit/includes/cache/GenderCacheTest.php
new file mode 100644 (file)
index 0000000..d62ee20
--- /dev/null
+++ b/tests/phpunit/includes/cache/GenderCacheTest.php
@@ -0,0 +1,100 @@
+<?php\r
+\r
+/**\r
+ * @group Database\r
+ * @group Cache\r
+ */\r
+class GenderCacheTest extends MediaWikiLangTestCase {\r
+\r
+       function setUp() {\r
+               parent::setUp();\r
+               //ensure the correct default gender\r
+               $wgDefaultUserOptions['gender'] = 'unknown';\r
+       }\r
+\r
+       function addDBData() {\r
+               $user = User::newFromName( 'UTMale' );\r
+               if( $user->getID() == 0 ) {\r
+                       $user->addToDatabase();\r
+                       $user->setPassword( 'UTMalePassword' );\r
+               }\r
+               //ensure the right gender\r
+               $user->setOption( 'gender', 'male' );\r
+               $user->saveSettings();\r
+\r
+               $user = User::newFromName( 'UTFemale' );\r
+               if( $user->getID() == 0 ) {\r
+                       $user->addToDatabase();\r
+                       $user->setPassword( 'UTFemalePassword' );\r
+               }\r
+               //ensure the right gender\r
+               $user->setOption( 'gender', 'female' );\r
+               $user->saveSettings();\r
+\r
+               $user = User::newFromName( 'UTDefaultGender' );\r
+               if( $user->getID() == 0 ) {\r
+                       $user->addToDatabase();\r
+                       $user->setPassword( 'UTDefaultGenderPassword' );\r
+               }\r
+               //ensure the default gender\r
+               $user->setOption( 'gender', null );\r
+               $user->saveSettings();\r
+       }\r
+\r
+       /**\r
+        * test usernames\r
+        *\r
+        * @dataProvider dataUserName\r
+        */\r
+       function testUserName( $username, $expectedGender ) {\r
+               $genderCache = GenderCache::singleton();\r
+               $gender = $genderCache->getGenderOf( $username );\r
+               $this->assertEquals( $gender, $expectedGender, "GenderCache normal" );\r
+       }\r
+\r
+       /**\r
+        * genderCache should work with user objects, too\r
+        *\r
+        * @dataProvider dataUserName\r
+        */\r
+       function testUserObjects( $username, $expectedGender ) {\r
+               $genderCache = GenderCache::singleton();\r
+               $user = User::newFromName( $username );\r
+               $gender = $genderCache->getGenderOf( $user );\r
+               $this->assertEquals( $gender, $expectedGender, "GenderCache normal" );\r
+       }\r
+\r
+       function dataUserName() {\r
+               return array(\r
+                       array( 'UTMale', 'male' ),\r
+                       array( 'UTFemale', 'female' ),\r
+                       array( 'UTDefaultGender', 'unknown' ),\r
+                       array( 'UTNotExist', 'unknown' ),\r
+                       //some not valid user\r
+                       array( '127.0.0.1', 'unknown' ),\r
+                       array( 'user@test', 'unknown' ),\r
+               );\r
+       }\r
+\r
+       /**\r
+        * test strip of subpages to avoid unnecessary queries\r
+        * against the never existing username\r
+        *\r
+        * @dataProvider dataStripSubpages\r
+        */\r
+       function testStripSubpages( $pageWithSubpage, $expectedGender ) {\r
+               $genderCache = GenderCache::singleton();\r
+               $gender = $genderCache->getGenderOf( $pageWithSubpage );\r
+               $this->assertEquals( $gender, $expectedGender, "GenderCache must strip of subpages" );\r
+       }\r
+\r
+       function dataStripSubpages() {\r
+               return array(\r
+                       array( 'UTMale/subpage', 'male' ),\r
+                       array( 'UTFemale/subpage', 'female' ),\r
+                       array( 'UTDefaultGender/subpage', 'unknown' ),\r
+                       array( 'UTNotExist/subpage', 'unknown' ),\r
+                       array( '127.0.0.1/subpage', 'unknown' ),\r
+               );\r
+       }\r
+}\r