Log warning and show error on empty username

Historically it seems that if Linker::userLink or friends were passed an
empty username (probably due to an incorrect database entry), they would
produce bogus output, e.g., an <a> with no contents or a link to the
invalid page "User_talk:" or similar.

In b6e1e99bec8d we replaced an occurrence of Title::makeTitle() (no
safety checks!) with creating a TitleValue, which asserts in its
constructor that the title text is not empty. This made such pages fail
an assertion and stop displaying at all.

Now there's a proper check for the error. Such cases will log a
production error and return "(no username available)".

Bug: T222529
Change-Id: Id65bdf9666b0d16e5553b8f38c7cf8fce2e37a25

diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34
index 98ed961..22b7443 100644 (file)
--- a/RELEASE-NOTES-1.34
+++ b/RELEASE-NOTES-1.34
@@ -55,7 +55,10 @@ For notes on 1.33.x and older releases, see HISTORY.
 * …
 
 === Bug fixes in 1.34 ===
-* …
+* (T222529) If a log entry or page revision is recorded in the database with an
+  empty username, attempting to display it will log an error and return a "no
+  username available" to the user instead of silently displaying nothing or
+  invalid links.
 
 === Action API changes in 1.34 ===
 * The 'recenteditcount' response property from action=query list=allusers,

diff --git a/includes/Linker.php b/includes/Linker.php
index 9cca0be..6acfda3 100644 (file)
--- a/includes/Linker.php
+++ b/includes/Linker.php
@@ -894,6 +894,12 @@ class Linker {
         * @since 1.16.3. $altUserName was added in 1.19.
         */
        public static function userLink( $userId, $userName, $altUserName = false ) {
+               if ( $userName === '' ) {
+                       wfLogWarning( __METHOD__ . ' received an empty username. Are there database errors ' .
+                               'that need to be fixed?' );
+                       return wfMessage( 'empty-username' )->parse();
+               }
+
                $classes = 'mw-userlink';
                $page = null;
                if ( $userId == 0 ) {
@@ -936,6 +942,12 @@ class Linker {
                $userId, $userText, $redContribsWhenNoEdits = false, $flags = 0, $edits = null,
                $useParentheses = true
        ) {
+               if ( $userText === '' ) {
+                       wfLogWarning( __METHOD__ . ' received an empty username. Are there database errors ' .
+                               'that need to be fixed?' );
+                       return ' ' . wfMessage( 'empty-username' )->parse();
+               }
+
                global $wgUser, $wgDisableAnonTalk, $wgLang;
                $talkable = !( $wgDisableAnonTalk && $userId == 0 );
                $blockable = !( $flags & self::TOOL_LINKS_NOBLOCK );
@@ -1018,6 +1030,12 @@ class Linker {
         * @return string HTML fragment with user talk link
         */
        public static function userTalkLink( $userId, $userText ) {
+               if ( $userText === '' ) {
+                       wfLogWarning( __METHOD__ . ' received an empty username. Are there database errors ' .
+                               'that need to be fixed?' );
+                       return wfMessage( 'empty-username' )->parse();
+               }
+
                $userTalkPage = new TitleValue( NS_USER_TALK, strtr( $userText, ' ', '_' ) );
                $moreLinkAttribs['class'] = 'mw-usertoollinks-talk';
 
@@ -1034,6 +1052,12 @@ class Linker {
         * @return string HTML fragment with block link
         */
        public static function blockLink( $userId, $userText ) {
+               if ( $userText === '' ) {
+                       wfLogWarning( __METHOD__ . ' received an empty username. Are there database errors ' .
+                               'that need to be fixed?' );
+                       return wfMessage( 'empty-username' )->parse();
+               }
+
                $blockPage = SpecialPage::getTitleFor( 'Block', $userText );
                $moreLinkAttribs['class'] = 'mw-usertoollinks-block';
 
@@ -1049,6 +1073,12 @@ class Linker {
         * @return string HTML fragment with e-mail user link
         */
        public static function emailLink( $userId, $userText ) {
+               if ( $userText === '' ) {
+                       wfLogWarning( __METHOD__ . ' received an empty username. Are there database errors ' .
+                               'that need to be fixed?' );
+                       return wfMessage( 'empty-username' )->parse();
+               }
+
                $emailPage = SpecialPage::getTitleFor( 'Emailuser', $userText );
                $moreLinkAttribs['class'] = 'mw-usertoollinks-mail';
                return self::link( $emailPage,

diff --git a/languages/i18n/en.json b/languages/i18n/en.json
index 5eeaf93..15edfc0 100644 (file)
--- a/languages/i18n/en.json
+++ b/languages/i18n/en.json
@@ -2711,6 +2711,7 @@
        "blocklink": "block",
        "unblocklink": "unblock",
        "change-blocklink": "change block",
+       "empty-username": "(no username available)",
        "contribslink": "contribs",
        "emaillink": "send email",
        "autoblocker": "Autoblocked because your IP address has been recently used by \"[[User:$1|$1]]\".\nThe reason given for $1's block is \"$2\"",

diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json
index 8a776dc..9dfa861 100644 (file)
--- a/languages/i18n/qqq.json
+++ b/languages/i18n/qqq.json
@@ -2918,6 +2918,7 @@
        "blocklink": "Display name for a link that, when selected, leads to a form where a user can be blocked. Used in page history and recent changes pages. Example: \"''UserName (Talk | contribs | '''block''')''\".\n\nUsed as link title in [[Special:Contributions]] and in [[Special:DeletedContributions]].\n\nSee also:\n* {{msg-mw|Sp-contributions-talk}}\n* {{msg-mw|Change-blocklink}}\n* {{msg-mw|Unblocklink}}\n* {{msg-mw|Sp-contributions-blocklog}}\n* {{msg-mw|Sp-contributions-uploads}}\n* {{msg-mw|Sp-contributions-logs}}\n* {{msg-mw|Sp-contributions-deleted}}\n* {{msg-mw|Sp-contributions-userrights}}\n{{Identical|Block}}",
        "unblocklink": "Used as link title in [[Special:Contributions]] and in [[Special:DeletedContributions]].\n\nSee also:\n* {{msg-mw|Sp-contributions-talk}}\n* {{msg-mw|change-blocklink}}\n* {{msg-mw|blocklink}}\n* {{msg-mw|sp-contributions-blocklog}}\n* {{msg-mw|sp-contributions-uploads}}\n* {{msg-mw|sp-contributions-logs}}\n* {{msg-mw|sp-contributions-deleted}}\n* {{msg-mw|sp-contributions-userrights}}\n{{Identical|Unblock}}",
        "change-blocklink": "Used to name the link on [[Special:Log]].\n\nAlso used as link title in [[Special:Contributions]] and in [[Special:DeletedContributions]].\n\nSee also:\n* {{msg-mw|Sp-contributions-talk}}\n* {{msg-mw|unblocklink}}\n* {{msg-mw|blocklink}}\n* {{msg-mw|sp-contributions-blocklog}}\n* {{msg-mw|sp-contributions-uploads}}\n* {{msg-mw|sp-contributions-logs}}\n* {{msg-mw|sp-contributions-deleted}}\n* {{msg-mw|sp-contributions-userrights}}",
+       "empty-username": "If no username is available to display for a log or history entry, such as because of an incorrect database entry, this message is displayed in place of the links to the user page/talk/contribs/etc.",
        "contribslink": "Short for \"contributions\". Used as display name for a link to user contributions on history pages, [[Special:RecentChanges]], [[Special:Watchlist]], etc.\n{{Identical|Contribution}}",
        "emaillink": "Used as display name for a link to send an e-mail to a user in the user tool links. Example: \"(Talk | contribs | block | send e-mail)\".\n\n{{Identical|E-mail}}",
        "autoblocker": "Used in [[Special:Block]].\n* $1 - target username\n* $2 - reason",

diff --git a/tests/phpunit/includes/LinkerTest.php b/tests/phpunit/includes/LinkerTest.php
index 438d3e7..2362961 100644 (file)
--- a/tests/phpunit/includes/LinkerTest.php
+++ b/tests/phpunit/includes/LinkerTest.php
@@ -14,11 +14,16 @@ class LinkerTest extends MediaWikiLangTestCase {
                        'wgArticlePath' => '/wiki/$1',
                ] );
 
-               $this->assertEquals(
-                       $expected,
-                       Linker::userLink( $userId, $userName, $altUserName ),
-                       $msg
-               );
+               // We'd also test the warning, but injecting a mock logger into a static method is tricky.
+               if ( $userName === '' ) {
+                       Wikimedia\suppressWarnings();
+               }
+               $actual = Linker::userLink( $userId, $userName, $altUserName );
+               if ( $userName === '' ) {
+                       Wikimedia\restoreWarnings();
+               }
+
+               $this->assertEquals( $expected, $actual, $msg );
        }
 
        public static function provideCasesForUserLink() {
@@ -29,6 +34,9 @@ class LinkerTest extends MediaWikiLangTestCase {
                # - optional altUserName
                # - optional message
                return [
+                       # Empty name (T222529)
+                       'Empty username, userid 0' => [ '(no username available)', 0, '' ],
+                       'Empty username, userid > 0' => [ '(no username available)', 73, '' ],
 
                        # ## ANONYMOUS USER ########################################
                        [
@@ -87,6 +95,118 @@ class LinkerTest extends MediaWikiLangTestCase {
                ];
        }
 
+       /**
+        * @dataProvider provideUserToolLinks
+        * @covers Linker::userToolLinks
+        * @param string $expected
+        * @param int $userId
+        * @param string $userText
+        */
+       public function testUserToolLinks( $expected, $userId, $userText ) {
+               // We'd also test the warning, but injecting a mock logger into a static method is tricky.
+               if ( $userText === '' ) {
+                       Wikimedia\suppressWarnings();
+               }
+               $actual = Linker::userToolLinks( $userId, $userText );
+               if ( $userText === '' ) {
+                       Wikimedia\restoreWarnings();
+               }
+
+               $this->assertSame( $expected, $actual );
+       }
+
+       public static function provideUserToolLinks() {
+               return [
+                       // Empty name (T222529)
+                       'Empty username, userid 0' => [ ' (no username available)', 0, '' ],
+                       'Empty username, userid > 0' => [ ' (no username available)', 73, '' ],
+               ];
+       }
+
+       /**
+        * @dataProvider provideUserTalkLink
+        * @covers Linker::userTalkLink
+        * @param string $expected
+        * @param int $userId
+        * @param string $userText
+        */
+       public function testUserTalkLink( $expected, $userId, $userText ) {
+               // We'd also test the warning, but injecting a mock logger into a static method is tricky.
+               if ( $userText === '' ) {
+                       Wikimedia\suppressWarnings();
+               }
+               $actual = Linker::userTalkLink( $userId, $userText );
+               if ( $userText === '' ) {
+                       Wikimedia\restoreWarnings();
+               }
+
+               $this->assertSame( $expected, $actual );
+       }
+
+       public static function provideUserTalkLink() {
+               return [
+                       // Empty name (T222529)
+                       'Empty username, userid 0' => [ '(no username available)', 0, '' ],
+                       'Empty username, userid > 0' => [ '(no username available)', 73, '' ],
+               ];
+       }
+
+       /**
+        * @dataProvider provideBlockLink
+        * @covers Linker::blockLink
+        * @param string $expected
+        * @param int $userId
+        * @param string $userText
+        */
+       public function testBlockLink( $expected, $userId, $userText ) {
+               // We'd also test the warning, but injecting a mock logger into a static method is tricky.
+               if ( $userText === '' ) {
+                       Wikimedia\suppressWarnings();
+               }
+               $actual = Linker::blockLink( $userId, $userText );
+               if ( $userText === '' ) {
+                       Wikimedia\restoreWarnings();
+               }
+
+               $this->assertSame( $expected, $actual );
+       }
+
+       public static function provideBlockLink() {
+               return [
+                       // Empty name (T222529)
+                       'Empty username, userid 0' => [ '(no username available)', 0, '' ],
+                       'Empty username, userid > 0' => [ '(no username available)', 73, '' ],
+               ];
+       }
+
+       /**
+        * @dataProvider provideEmailLink
+        * @covers Linker::emailLink
+        * @param string $expected
+        * @param int $userId
+        * @param string $userText
+        */
+       public function testEmailLink( $expected, $userId, $userText ) {
+               // We'd also test the warning, but injecting a mock logger into a static method is tricky.
+               if ( $userText === '' ) {
+                       Wikimedia\suppressWarnings();
+               }
+               $actual = Linker::emailLink( $userId, $userText );
+               if ( $userText === '' ) {
+                       Wikimedia\restoreWarnings();
+               }
+
+               $this->assertSame( $expected, $actual );
+       }
+
+       public static function provideEmailLink() {
+               return [
+                       // Empty name (T222529)
+                       'Empty username, userid 0' => [ '(no username available)', 0, '' ],
+                       'Empty username, userid > 0' => [ '(no username available)', 73, '' ],
+               ];
+       }
+
        /**
         * @dataProvider provideCasesForFormatComment
         * @covers Linker::formatComment