Merge "API: Remove leading/trailing spaces from error and description text"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Sun, 27 Oct 2013 20:16:08 +0000 (20:16 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Sun, 27 Oct 2013 20:16:08 +0000 (20:16 +0000)
RELEASE-NOTES-1.23
docs/hooks.txt
includes/Article.php
includes/ImagePage.php
includes/Skin.php
includes/User.php
includes/WatchedItem.php
includes/Wiki.php
includes/WikiPage.php
includes/diff/DifferenceEngine.php
maintenance/tables.sql

index 372170a..ec7b898 100644 (file)
@@ -13,6 +13,10 @@ production.
 === New features in 1.23 ===
 
 === Bug fixes in 1.23 ===
+* (bug 41759) The "updated since last visit" markers (on history pages, recent
+  changes and watchlist) and the talk page message indicator are now correctly
+  updated when the user is viewing old revisions of pages, instead of always
+  acting as if the latest revision was being viewed.
 
 === API changes in 1.23 ===
 
index 5aaf596..2671dd8 100644 (file)
@@ -2579,6 +2579,7 @@ $user: User (object) whose permission is being checked
 'UserClearNewTalkNotification': Called when clearing the "You have new
 messages!" message, return false to not delete it.
 $user: User (object) that will clear the message
+$oldid: ID of the talk page revision being viewed (0 means the most recent one)
 
 'UserComparePasswords': Called when checking passwords, return false to
 override the default password checks.
index 0b18221..928fda0 100644 (file)
@@ -586,7 +586,7 @@ class Article implements Page {
                                wfDebug( __METHOD__ . ": done file cache\n" );
                                # tell wgOut that output is taken care of
                                $outputPage->disable();
-                               $this->mPage->doViewUpdates( $user );
+                               $this->mPage->doViewUpdates( $user, $oldid );
                                wfProfileOut( __METHOD__ );
 
                                return;
@@ -765,7 +765,7 @@ class Article implements Page {
                $outputPage->setFollowPolicy( $policy['follow'] );
 
                $this->showViewFooter();
-               $this->mPage->doViewUpdates( $user );
+               $this->mPage->doViewUpdates( $user, $oldid );
 
                $outputPage->addModules( 'mediawiki.action.view.postEdit' );
 
@@ -815,10 +815,10 @@ class Article implements Page {
                $this->mRevIdFetched = $de->mNewid;
                $de->showDiffPage( $diffOnly );
 
-               if ( $diff == 0 || $diff == $this->mPage->getLatest() ) {
-                       # Run view updates for current revision only
-                       $this->mPage->doViewUpdates( $user );
-               }
+               // Run view updates for the newer revision being diffed (and shown below the diff if not $diffOnly)
+               list( $old, $new ) = $de->mapDiffPrevNext( $oldid, $diff );
+               // New can be false, convert it to 0 - this conveniently means the latest revision
+               $this->mPage->doViewUpdates( $user, (int)$new );
        }
 
        /**
index 515f146..cf05ee2 100644 (file)
@@ -128,7 +128,7 @@ class ImagePage extends Article {
                                $out->setPageTitle( $this->getTitle()->getPrefixedText() );
                                $out->addHTML( $this->viewRedirect( Title::makeTitle( NS_FILE, $this->mPage->getFile()->getName() ),
                                        /* $appendSubtitle */ true, /* $forceKnown */ true ) );
-                               $this->mPage->doViewUpdates( $this->getContext()->getUser() );
+                               $this->mPage->doViewUpdates( $this->getContext()->getUser(), $this->getOldID() );
                                return;
                        }
                }
@@ -165,7 +165,7 @@ class ImagePage extends Article {
                        # Just need to set the right headers
                        $out->setArticleFlag( true );
                        $out->setPageTitle( $this->getTitle()->getPrefixedText() );
-                       $this->mPage->doViewUpdates( $this->getContext()->getUser() );
+                       $this->mPage->doViewUpdates( $this->getContext()->getUser(), $this->getOldID() );
                }
 
                # Show shared description, if needed
index 5801806..170e96f 100644 (file)
@@ -1379,61 +1379,58 @@ abstract class Skin extends ContextSource {
 
                if ( count( $newtalks ) == 1 && $newtalks[0]['wiki'] === wfWikiID() ) {
                        $uTalkTitle = $user->getTalkPage();
-
-                       if ( !$uTalkTitle->equals( $out->getTitle() ) ) {
-                               $lastSeenRev = isset( $newtalks[0]['rev'] ) ? $newtalks[0]['rev'] : null;
-                               $nofAuthors = 0;
-                               if ( $lastSeenRev !== null ) {
-                                       $plural = true; // Default if we have a last seen revision: if unknown, use plural
-                                       $latestRev = Revision::newFromTitle( $uTalkTitle, false, Revision::READ_NORMAL );
-                                       if ( $latestRev !== null ) {
-                                               // Singular if only 1 unseen revision, plural if several unseen revisions.
-                                               $plural = $latestRev->getParentId() !== $lastSeenRev->getId();
-                                               $nofAuthors = $uTalkTitle->countAuthorsBetween(
-                                                       $lastSeenRev, $latestRev, 10, 'include_new' );
-                                       }
-                               } else {
-                                       // Singular if no revision -> diff link will show latest change only in any case
-                                       $plural = false;
+                       $lastSeenRev = isset( $newtalks[0]['rev'] ) ? $newtalks[0]['rev'] : null;
+                       $nofAuthors = 0;
+                       if ( $lastSeenRev !== null ) {
+                               $plural = true; // Default if we have a last seen revision: if unknown, use plural
+                               $latestRev = Revision::newFromTitle( $uTalkTitle, false, Revision::READ_NORMAL );
+                               if ( $latestRev !== null ) {
+                                       // Singular if only 1 unseen revision, plural if several unseen revisions.
+                                       $plural = $latestRev->getParentId() !== $lastSeenRev->getId();
+                                       $nofAuthors = $uTalkTitle->countAuthorsBetween(
+                                               $lastSeenRev, $latestRev, 10, 'include_new' );
                                }
-                               $plural = $plural ? 2 : 1;
-                               // 2 signifies "more than one revision". We don't know how many, and even if we did,
-                               // the number of revisions or authors is not necessarily the same as the number of
-                               // "messages".
-                               $newMessagesLink = Linker::linkKnown(
-                                       $uTalkTitle,
-                                       $this->msg( 'newmessageslinkplural' )->params( $plural )->escaped(),
-                                       array(),
-                                       array( 'redirect' => 'no' )
-                               );
+                       } else {
+                               // Singular if no revision -> diff link will show latest change only in any case
+                               $plural = false;
+                       }
+                       $plural = $plural ? 2 : 1;
+                       // 2 signifies "more than one revision". We don't know how many, and even if we did,
+                       // the number of revisions or authors is not necessarily the same as the number of
+                       // "messages".
+                       $newMessagesLink = Linker::linkKnown(
+                               $uTalkTitle,
+                               $this->msg( 'newmessageslinkplural' )->params( $plural )->escaped(),
+                               array(),
+                               array( 'redirect' => 'no' )
+                       );
 
-                               $newMessagesDiffLink = Linker::linkKnown(
-                                       $uTalkTitle,
-                                       $this->msg( 'newmessagesdifflinkplural' )->params( $plural )->escaped(),
-                                       array(),
-                                       $lastSeenRev !== null
-                                               ? array( 'oldid' => $lastSeenRev->getId(), 'diff' => 'cur' )
-                                               : array( 'diff' => 'cur' )
-                               );
+                       $newMessagesDiffLink = Linker::linkKnown(
+                               $uTalkTitle,
+                               $this->msg( 'newmessagesdifflinkplural' )->params( $plural )->escaped(),
+                               array(),
+                               $lastSeenRev !== null
+                                       ? array( 'oldid' => $lastSeenRev->getId(), 'diff' => 'cur' )
+                                       : array( 'diff' => 'cur' )
+                       );
 
-                               if ( $nofAuthors >= 1 && $nofAuthors <= 10 ) {
-                                       $newMessagesAlert = $this->msg(
-                                               'youhavenewmessagesfromusers',
-                                               $newMessagesLink,
-                                               $newMessagesDiffLink
-                                       )->numParams( $nofAuthors );
-                               } else {
-                                       // $nofAuthors === 11 signifies "11 or more" ("more than 10")
-                                       $newMessagesAlert = $this->msg(
-                                               $nofAuthors > 10 ? 'youhavenewmessagesmanyusers' : 'youhavenewmessages',
-                                               $newMessagesLink,
-                                               $newMessagesDiffLink
-                                       );
-                               }
-                               $newMessagesAlert = $newMessagesAlert->text();
-                               # Disable Squid cache
-                               $out->setSquidMaxage( 0 );
+                       if ( $nofAuthors >= 1 && $nofAuthors <= 10 ) {
+                               $newMessagesAlert = $this->msg(
+                                       'youhavenewmessagesfromusers',
+                                       $newMessagesLink,
+                                       $newMessagesDiffLink
+                               )->numParams( $nofAuthors );
+                       } else {
+                               // $nofAuthors === 11 signifies "11 or more" ("more than 10")
+                               $newMessagesAlert = $this->msg(
+                                       $nofAuthors > 10 ? 'youhavenewmessagesmanyusers' : 'youhavenewmessages',
+                                       $newMessagesLink,
+                                       $newMessagesDiffLink
+                               );
                        }
+                       $newMessagesAlert = $newMessagesAlert->text();
+                       # Disable Squid cache
+                       $out->setSquidMaxage( 0 );
                } elseif ( count( $newtalks ) ) {
                        $sep = $this->msg( 'newtalkseparator' )->escaped();
                        $msgs = array();
index 12912e1..c86b966 100644 (file)
@@ -3021,8 +3021,9 @@ class User {
         * the next change of the page if it's watched etc.
         * @note If the user doesn't have 'editmywatchlist', this will do nothing.
         * @param $title Title of the article to look at
+        * @param int $oldid The revision id being viewed. If not given or 0, latest revision is assumed.
         */
-       public function clearNotification( &$title ) {
+       public function clearNotification( &$title, $oldid = 0 ) {
                global $wgUseEnotif, $wgShowUpdatedMarker;
 
                // Do nothing if the database is locked to writes
@@ -3035,12 +3036,25 @@ class User {
                        return;
                }
 
-               if ( $title->getNamespace() == NS_USER_TALK &&
-                       $title->getText() == $this->getName() ) {
-                       if ( !wfRunHooks( 'UserClearNewTalkNotification', array( &$this ) ) ) {
+               // If we're working on user's talk page, we should update the talk page message indicator
+               if ( $title->getNamespace() == NS_USER_TALK && $title->getText() == $this->getName() ) {
+                       if ( !wfRunHooks( 'UserClearNewTalkNotification', array( &$this, $oldid ) ) ) {
                                return;
                        }
-                       $this->setNewtalk( false );
+
+                       $nextid = $oldid ? $title->getNextRevisionID( $oldid ) : null;
+
+                       if ( !$oldid || !$nextid ) {
+                               // If we're looking at the latest revision, we should definitely clear it
+                               $this->setNewtalk( false );
+                       } else {
+                               // Otherwise we should update its revision, if it's present
+                               if ( $this->getNewtalk() ) {
+                                       // Naturally the other one won't clear by itself
+                                       $this->setNewtalk( false );
+                                       $this->setNewtalk( true, Revision::newFromId( $nextid ) );
+                               }
+                       }
                }
 
                if ( !$wgUseEnotif && !$wgShowUpdatedMarker ) {
@@ -3063,7 +3077,7 @@ class User {
                        $force = 'force';
                }
 
-               $this->getWatchedItem( $title )->resetNotificationTimestamp( $force );
+               $this->getWatchedItem( $title )->resetNotificationTimestamp( $force, $oldid );
        }
 
        /**
@@ -3091,14 +3105,12 @@ class User {
                if ( $id != 0 ) {
                        $dbw = wfGetDB( DB_MASTER );
                        $dbw->update( 'watchlist',
-                               array( /* SET */
-                                       'wl_notificationtimestamp' => null
-                               ), array( /* WHERE */
-                                       'wl_user' => $id
-                               ), __METHOD__
+                               array( /* SET */ 'wl_notificationtimestamp' => null ),
+                               array( /* WHERE */ 'wl_user' => $id ),
+                               __METHOD__
                        );
-               #       We also need to clear here the "you have new message" notification for the own user_talk page
-               #       This is cleared one page view later in Article::viewUpdates();
+                       // We also need to clear here the "you have new message" notification for the own user_talk page;
+                       // it's cleared one page view later in WikiPage::doViewUpdates().
                }
        }
 
index 1e07e7c..d2fb468 100644 (file)
@@ -173,8 +173,9 @@ class WatchedItem {
         *
         * @param $force Whether to force the write query to be executed even if the
         *        page is not watched or the notification timestamp is already NULL.
+        * @param int $oldid The revision id being viewed. If not given or 0, latest revision is assumed.
         */
-       public function resetNotificationTimestamp( $force = '' ) {
+       public function resetNotificationTimestamp( $force = '', $oldid = 0 ) {
                // Only loggedin user can have a watchlist
                if ( wfReadOnly() || $this->mUser->isAnon() || !$this->isAllowed( 'editmywatchlist' ) ) {
                        return;
@@ -187,10 +188,50 @@ class WatchedItem {
                        }
                }
 
+               $title = $this->getTitle();
+               if ( !$oldid ) {
+                       // No oldid given, assuming latest revision; clear the timestamp.
+                       $notificationTimestamp = null;
+               } elseif ( !$title->getNextRevisionID( $oldid ) ) {
+                       // Oldid given and is the latest revision for this title; clear the timestamp.
+                       $notificationTimestamp = null;
+               } else {
+                       // See if the version marked as read is more recent than the one we're viewing.
+                       // Call load() if it wasn't called before due to $force.
+                       $this->load();
+
+                       if ( $this->timestamp === null ) {
+                               // This can only happen if $force is enabled.
+                               $notificationTimestamp = null;
+                       } else {
+                               // Oldid given and isn't the latest; update the timestamp.
+                               // This will result in no further notification emails being sent!
+                               $dbr = wfGetDB( DB_SLAVE );
+                               $notificationTimestamp = $dbr->selectField(
+                                       'revision', 'rev_timestamp',
+                                       array( 'rev_page' => $title->getArticleID(), 'rev_id' => $oldid )
+                               );
+                               // We need to go one second to the future because of various strict comparisons
+                               // throughout the codebase
+                               $ts = new MWTimestamp( $notificationTimestamp );
+                               $ts->timestamp->add( new DateInterval( 'PT1S' ) );
+                               $notificationTimestamp = $ts->getTimestamp( TS_MW );
+
+                               if ( $notificationTimestamp < $this->timestamp ) {
+                                       if ( $force != 'force' ) {
+                                               return;
+                                       } else {
+                                               // This is a little silly…
+                                               $notificationTimestamp = $this->timestamp;
+                                       }
+                               }
+                       }
+               }
+
                // If the page is watched by the user (or may be watched), update the timestamp on any
                // any matching rows
                $dbw = wfGetDB( DB_MASTER );
-               $dbw->update( 'watchlist', array( 'wl_notificationtimestamp' => null ),
+               $dbw->update( 'watchlist', array( 'wl_notificationtimestamp' => $notificationTimestamp ),
                        $this->dbCond(), __METHOD__ );
                $this->timestamp = null;
        }
index edfbba2..403ef11 100644 (file)
@@ -588,6 +588,7 @@ class MediaWiki {
                                                $cache->loadFromFileCache( $this->context );
                                        }
                                        // Do any stats increment/watchlist stuff
+                                       // Assume we're viewing the latest revision (this should always be the case with file cache)
                                        $this->context->getWikiPage()->doViewUpdates( $this->context->getUser() );
                                        // Tell OutputPage that output is taken care of
                                        $this->context->getOutput()->disable();
index 7c3dc93..6d2d80c 100644 (file)
@@ -1134,9 +1134,10 @@ class WikiPage implements Page, IDBAccessObject {
 
        /**
         * Do standard deferred updates after page view
-        * @param $user User The relevant user
+        * @param User $user The relevant user
+        * @param int $oldid The revision id being viewed. If not given or 0, latest revision is assumed.
         */
-       public function doViewUpdates( User $user ) {
+       public function doViewUpdates( User $user, $oldid = 0 ) {
                global $wgDisableCounters;
                if ( wfReadOnly() ) {
                        return;
@@ -1149,7 +1150,7 @@ class WikiPage implements Page, IDBAccessObject {
                }
 
                // Update newtalk / watchlist notification status
-               $user->clearNotification( $this->mTitle );
+               $user->clearNotification( $this->mTitle, $oldid );
        }
 
        /**
index e436f58..ea74164 100644 (file)
@@ -1041,6 +1041,33 @@ class DifferenceEngine extends ContextSource {
                $this->mDiffLang = wfGetLangObj( $lang );
        }
 
+       /**
+        * Maps a revision pair definition as accepted by DifferenceEngine constructor
+        * to a pair of actual integers representing revision ids.
+        *
+        * @param int $old Revision id, e.g. from URL parameter 'oldid'
+        * @param int|string $new Revision id or strings 'next' or 'prev', e.g. from URL parameter 'diff'
+        * @return array Array of two revision ids, older first, later second.
+        *     Zero signifies invalid argument passed.
+        *     false signifies that there is no previous/next revision ($old is the oldest/newest one).
+        */
+       public function mapDiffPrevNext( $old, $new ) {
+               if ( $new === 'prev' ) {
+                       // Show diff between revision $old and the previous one. Get previous one from DB.
+                       $newid = intval( $old );
+                       $oldid = $this->getTitle()->getPreviousRevisionID( $newid );
+               } elseif ( $new === 'next' ) {
+                       // Show diff between revision $old and the next one. Get next one from DB.
+                       $oldid = intval( $old );
+                       $newid = $this->getTitle()->getNextRevisionID( $oldid );
+               } else {
+                       $oldid = intval( $old );
+                       $newid = intval( $new );
+               }
+
+               return array( $oldid, $newid );
+       }
+
        /**
         * Load revision IDs
         */
@@ -1054,26 +1081,14 @@ class DifferenceEngine extends ContextSource {
                $old = $this->mOldid;
                $new = $this->mNewid;
 
-               if ( $new === 'prev' ) {
-                       # Show diff between revision $old and the previous one.
-                       # Get previous one from DB.
-                       $this->mNewid = intval( $old );
-                       $this->mOldid = $this->getTitle()->getPreviousRevisionID( $this->mNewid );
-               } elseif ( $new === 'next' ) {
-                       # Show diff between revision $old and the next one.
-                       # Get next one from DB.
-                       $this->mOldid = intval( $old );
-                       $this->mNewid = $this->getTitle()->getNextRevisionID( $this->mOldid );
-                       if ( $this->mNewid === false ) {
-                               # if no result, NewId points to the newest old revision. The only newer
-                               # revision is cur, which is "0".
-                               $this->mNewid = 0;
-                       }
-               } else {
-                       $this->mOldid = intval( $old );
-                       $this->mNewid = intval( $new );
-                       wfRunHooks( 'NewDifferenceEngine', array( $this->getTitle(), &$this->mOldid, &$this->mNewid, $old, $new ) );
+               list( $this->mOldid, $this->mNewid ) = self::mapDiffPrevNext( $old, $new );
+               if ( $new === 'next' && $this->mNewid === false ) {
+                       # if no result, NewId points to the newest old revision. The only newer
+                       # revision is cur, which is "0".
+                       $this->mNewid = 0;
                }
+
+               wfRunHooks( 'NewDifferenceEngine', array( $this->getTitle(), &$this->mOldid, &$this->mNewid, $old, $new ) );
        }
 
        /**
index de92ef5..61ffcf4 100644 (file)
@@ -1109,8 +1109,9 @@ CREATE TABLE /*_*/watchlist (
   wl_namespace int NOT NULL default 0,
   wl_title varchar(255) binary NOT NULL default '',
 
-  -- Timestamp when user was last sent a notification e-mail;
-  -- cleared when the user visits the page.
+  -- Timestamp used to send notification e-mails and show "updated since last visit" markers on
+  -- history and recent changes / watchlist. Set to NULL when the user visits the latest revision
+  -- of the page, which means that they should be sent an e-mail on the next change.
   wl_notificationtimestamp varbinary(14)
 
 ) /*$wgDBTableOptions*/;