jenkins-bot has submitted this change and it was merged.
Change subject: Patrol overhaul phase 1: Remove rcid parameters
......................................................................
Patrol overhaul phase 1: Remove rcid parameters
I've changed the logic in Article::showPatrolFooter to be able
to fetch the recent changes id and to only show the patrol link
in case the change hasn't yet been patrolled.
In case recentchanges patrolling is enabled this will try to
create a patrol link for the revision the user is currently
viewing. If only new page patrolling is enabled it tries to
create a patrol link for the first revision of the page.
Furthermore I've removed the passing around of &rcid parameters
within MediaWiki as those had several issues (some even security
related) and were only a workaround to protect the DB from some
queries, which is no longer needed.
This has already been partly implemented in a different manner in
r45778 but had to be reverted in r46542 due to performance issues.
This version shouldn't cause such issues as I'm only adding one or
two indexed database queries per page view.
I've written this new version of the patch with mostly
performance in mind and even tested the database queries it uses
against the replicated databases of enwiki on the toolserver. I'm
pretty sure this can't be implemented any faster without creating
a new index on the recentchanges table.
As I was on it I've implemented RecentChange::isInRCLifespan which
checks whether the given timestamp is new enough to may have a RC
row. That way we can avoid some DB queries for timestamps which are
older than the max RC age.
Fixes bugs:
(bug 15936) New page's patrol button should always be visible
(bug 35810) ! N pages non-patrol-able
(bug 36641) Patrol page link shows on non-existent revs
Change-Id: I1e24733cafbfdc51b7a5a9a1c1baf948e760fe1a
---
M includes/Article.php
M includes/ChangesList.php
M includes/RecentChange.php
M includes/diff/DifferenceEngine.php
M includes/specials/SpecialNewpages.php
5 files changed, 167 insertions(+), 61 deletions(-)
Approvals:
Tim Starling: Looks good to me, approved
jenkins-bot: Verified
diff --git a/includes/Article.php b/includes/Article.php
index a0d4438..9f54a52 100644
--- a/includes/Article.php
+++ b/includes/Article.php
@@ -1037,8 +1037,7 @@
$this->getContext()->getOutput()->addWikiMsg(
'anontalkpagetext' );
}
- # If we have been passed an &rcid= parameter, we want to give
the user a
- # chance to mark this new article as patrolled.
+ // Show a footer allowing the user to patrol the shown revision
or page if possible
$this->showPatrolFooter();
wfRunHooks( 'ArticleViewFooter', array( $this ) );
@@ -1053,15 +1052,118 @@
* OutputPage::preventClickjacking() and load
mediawiki.page.patrol.ajax.
*/
public function showPatrolFooter() {
+ global $wgUseRCPatrol, $wgUseNPPatrol, $wgRCMaxAge;
+
$request = $this->getContext()->getRequest();
$outputPage = $this->getContext()->getOutput();
$user = $this->getContext()->getUser();
- $rcid = $request->getVal( 'rcid' );
+ $cache = wfGetMainCache();
- if ( !$rcid || !$this->getTitle()->quickUserCan( 'patrol',
$user ) ) {
+ // Conditions to potentially patrol the current revision
+ // patrolPage is set in case we want to patrol the first
+ // revision and not the current one (set in Special:NewPages)
+ $useRCPatrol = $wgUseRCPatrol && !$request->getBool(
'patrolpage' );
+
+ if ( !$this->getTitle()->quickUserCan( 'patrol', $user ) || (
!$wgUseNPPatrol && !$wgUseRCPatrol ) ) {
+ // Patrolling is fully disabled or the user isn't
allowed to
return;
}
+ wfProfileIn( __METHOD__ );
+
+ if ( $useRCPatrol ) {
+ // Check for cached results
+ if ( $cache->get( wfMemcKey( 'NotPatrollableRevId',
$this->getRevIdFetched() ) ) ) {
+ wfProfileOut( __METHOD__ );
+ return;
+ }
+
+ // We make use of a little index trick over here:
+ // First we get the timestamp of the last revision and
then
+ // we look up the RC row by that as the timestamp is
indexed
+ // and usually very few rows exist for one timestamp
+ // (While several thousand can exists for a single page)
+ if ( !$this->mRevision ) {
+ $this->mRevision = Revision::newFromId(
$this->getRevIdFetched() );
+ }
+
+ if ( !$this->mRevision ||
!RecentChange::isInRCLifespan( $this->mRevision->getTimestamp(), 21600 ) ) {
+ // The revision is more than 6 hours older than
the Max RC age
+ // no need to torture the DB any further (6h
because the RC might not be cleaned out regularly)
+ wfProfileOut( __METHOD__ );
+ return;
+ }
+ $rc = RecentChange::newFromConds(
+ array(
+ 'rc_this_oldid' =>
$this->getRevIdFetched(),
+ 'rc_timestamp' =>
$this->mRevision->getTimestamp(),
+ 'rc_cur_id' =>
$this->getTitle()->getArticleID(),
+ 'rc_patrolled' => 0
+ ),
+ __METHOD__,
+ array( 'USE INDEX' => 'rc_timestamp' )
+ );
+ } else {
+ // RC patrol is disabled so we have to patrol the first
+ // revision (new page patrol) in case it's in the RC
table.
+ // To achieve this we get the timestamp of the oldest
revison
+ // the revision table holds for the given page. Then we
look
+ // whether it's within the RC lifespan and if it is, we
try
+ // to get the recentchanges row belonging to that entry
+ // (with rc_new = 1).
+
+ // Check for cached results
+ if ( $cache->get( wfMemcKey( 'NotPatrollablePage',
$this->getTitle()->getArticleID() ) ) ) {
+ wfProfileOut( __METHOD__ );
+ return;
+ }
+
+ $dbr = wfGetDB( DB_SLAVE );
+ $oldestRevisionTimestamp = $dbr->selectField(
+ 'revision',
+ 'MIN( rev_timestamp )',
+ array( 'rev_page' =>
$this->getTitle()->getArticleID() ),
+ __METHOD__
+ );
+
+ if ( !$oldestRevisionTimestamp ||
!RecentChange::isInRCLifespan( $oldestRevisionTimestamp, 21600 ) ) {
+ // We either didn't find the oldest revision
for the given page
+ // or it's to old for the RC table (with 6h
tolerance)
+ wfProfileOut( __METHOD__ );
+ return;
+ }
+
+ $rc = RecentChange::newFromConds(
+ array(
+ 'rc_new' => 1,
+ 'rc_timestamp' =>
$oldestRevisionTimestamp,
+ 'rc_namespace' =>
$this->getTitle()->getNamespace(),
+ 'rc_cur_id' =>
$this->getTitle()->getArticleID(),
+ 'rc_patrolled' => 0
+ ),
+ __METHOD__,
+ array( 'USE INDEX' => 'new_name_timestamp' )
+ );
+ }
+
+ wfProfileOut( __METHOD__ );
+
+ if ( !$rc ) {
+ // No RC entry around
+
+ // Cache the information we gathered above in case we
can't patrol
+ // Don't cache in case we can patrol as this could
change
+ if( $useRCPatrol ) {
+ $cache->set( wfMemcKey( 'NotPatrollableRevId',
$this->getRevIdFetched() ), '1', $wgRCMaxAge );
+ } else {
+ $cache->set( wfMemcKey( 'NotPatrollablePage',
$this->getTitle()->getArticleID() ), '1', $wgRCMaxAge );
+ }
+
+ return;
+ }
+
+ $rcid = $rc->getAttribute( 'rc_id' );
+
$token = $user->getEditToken( $rcid );
$outputPage->preventClickjacking();
diff --git a/includes/ChangesList.php b/includes/ChangesList.php
index 1b6b396..796b92a 100644
--- a/includes/ChangesList.php
+++ b/includes/ChangesList.php
@@ -334,10 +334,6 @@
'oldid' => $rc->mAttribs['rc_last_oldid']
);
- if ( $unpatrolled ) {
- $query['rcid'] = $rc->mAttribs['rc_id'];
- };
-
$diffLink = Linker::linkKnown(
$rc->getTitle(),
$this->message['diff'],
@@ -369,10 +365,6 @@
# If it's a new article, there is no diff link, but if it
hasn't been
# patrolled yet, we need to give users a way to do so
$params = array();
-
- if ( $unpatrolled && $rc->mAttribs['rc_type'] == RC_NEW ) {
- $params['rcid'] = $rc->mAttribs['rc_id'];
- }
$articlelink = Linker::linkKnown(
$rc->getTitle(),
@@ -752,8 +744,7 @@
if ( $type == RC_MOVE || $type == RC_MOVE_OVER_REDIRECT ) {
// New unpatrolled pages
} elseif ( $rc->unpatrolled && $type == RC_NEW ) {
- $clink = Linker::linkKnown( $rc->getTitle(), null,
array(),
- array( 'rcid' => $rc->mAttribs['rc_id'] ) );
+ $clink = Linker::linkKnown( $rc->getTitle() );
// Log entries
} elseif ( $type == RC_LOG ) {
if ( $logType ) {
@@ -789,14 +780,9 @@
# called too many times (50% of CPU time on RecentChanges!).
$thisOldid = $rc->mAttribs['rc_this_oldid'];
$lastOldid = $rc->mAttribs['rc_last_oldid'];
- if ( $rc->unpatrolled ) {
- $rcIdQuery = array( 'rcid' => $rc->mAttribs['rc_id'] );
- } else {
- $rcIdQuery = array();
- }
+
$querycur = $curIdEq + array( 'diff' => '0', 'oldid' =>
$thisOldid );
- $querydiff = $curIdEq + array( 'diff' => $thisOldid, 'oldid' =>
- $lastOldid ) + $rcIdQuery;
+ $querydiff = $curIdEq + array( 'diff' => $thisOldid, 'oldid' =>
$lastOldid );
if ( !$showdifflinks ) {
$curLink = $this->message['cur'];
@@ -823,7 +809,7 @@
$lastLink = $this->message['last'];
} else {
$lastLink = Linker::linkKnown( $rc->getTitle(),
$this->message['last'],
- array(), $curIdEq + array( 'diff' =>
$thisOldid, 'oldid' => $lastOldid ) + $rcIdQuery );
+ array(), $curIdEq + array( 'diff' =>
$thisOldid, 'oldid' => $lastOldid ) );
}
# Make user links
@@ -1088,9 +1074,6 @@
} elseif ( !ChangesList::userCan( $rcObj,
Revision::DELETED_TEXT, $this->getUser() ) ) {
$link = '<span class="history-deleted">' .
$rcObj->timestamp . '</span> ';
} else {
- if ( $rcObj->unpatrolled && $type == RC_NEW ) {
- $params['rcid'] =
$rcObj->mAttribs['rc_id'];
- }
$link = Linker::linkKnown(
$rcObj->getTitle(),
diff --git a/includes/RecentChange.php b/includes/RecentChange.php
index b5d4a1c..953d9e9 100644
--- a/includes/RecentChange.php
+++ b/includes/RecentChange.php
@@ -122,11 +122,12 @@
*
* @param array $conds of conditions
* @param $fname Mixed: override the method name in profiling/logs
+ * @param $options Array Query options
* @return RecentChange
*/
- public static function newFromConds( $conds, $fname = __METHOD__ ) {
+ public static function newFromConds( $conds, $fname = __METHOD__,
$options = array() ) {
$dbr = wfGetDB( DB_SLAVE );
- $row = $dbr->selectRow( 'recentchanges', self::selectFields(),
$conds, $fname );
+ $row = $dbr->selectRow( 'recentchanges', self::selectFields(),
$conds, $fname, $options );
if ( $row !== false ) {
return self::newFromRow( $row );
} else {
@@ -409,6 +410,9 @@
),
__METHOD__
);
+ // Invalidate the page cache after the page has been patrolled
+ // to make sure that the Patrol link isn't visible any longer!
+ $this->getTitle()->invalidateCache();
return $dbw->affectedRows();
}
@@ -862,4 +866,18 @@
}
return $ip;
}
+
+ /**
+ * Check whether the given timestamp is new enough to have a RC row
with a given tolerance
+ * as the recentchanges table might not be cleared out regularly (so
older entries might exist)
+ * or rows which will be deleted soon shouldn't be included.
+ *
+ * @param $timestamp mixed MWTimestamp compatible timestamp
+ * @param $tolerance integer Tolerance in seconds
+ * @return bool
+ */
+ public static function isInRCLifespan( $timestamp, $tolerance = 0 ) {
+ global $wgRCMaxAge;
+ return wfTimestamp( TS_UNIX, $timestamp ) > time() - $tolerance
- $wgRCMaxAge;
+ }
}
diff --git a/includes/diff/DifferenceEngine.php
b/includes/diff/DifferenceEngine.php
index 4ee5014..c551107 100644
--- a/includes/diff/DifferenceEngine.php
+++ b/includes/diff/DifferenceEngine.php
@@ -48,7 +48,6 @@
* @var Title
*/
var $mOldPage, $mNewPage;
- var $mRcidMarkPatrolled;
/**
* @var Revision
@@ -80,8 +79,8 @@
* Constructor
* @param $context IContextSource context to use, anything else will be
ignored
* @param $old Integer old ID we want to show and diff with.
- * @param string $new either 'prev' or 'next'.
- * @param $rcid Integer ??? FIXME (default 0)
+ * @param $new String either 'prev' or 'next'.
+ * @param $rcid Integer Deprecated, no longer used!
* @param $refreshCache boolean If set, refreshes the diff cache
* @param $unhide boolean If set, allow viewing deleted revs
*/
@@ -96,7 +95,6 @@
$this->mOldid = $old;
$this->mNewid = $new;
- $this->mRcidMarkPatrolled = intval( $rcid ); # force it to be
an integer
$this->mRefreshCache = $refreshCache;
$this->unhide = $unhide;
}
@@ -412,38 +410,39 @@
* @return String
*/
protected function markPatrolledLink() {
- global $wgUseRCPatrol;
+ global $wgUseRCPatrol, $wgRCMaxAge;
+ $cache = wfGetMainCache();
if ( $this->mMarkPatrolledLink === null ) {
// Prepare a change patrol link, if applicable
- if ( $wgUseRCPatrol && $this->mNewPage->quickUserCan(
'patrol', $this->getUser() ) ) {
- // If we've been given an explicit change
identifier, use it; saves time
- if ( $this->mRcidMarkPatrolled ) {
- $rcid = $this->mRcidMarkPatrolled;
- $rc = RecentChange::newFromId( $rcid );
- // Already patrolled?
- $rcid = is_object( $rc ) &&
!$rc->getAttribute( 'rc_patrolled' ) ? $rcid : 0;
+ if (
+ // Is patrolling enabled and the user allowed
to?
+ $wgUseRCPatrol &&
$this->mNewPage->quickUserCan( 'patrol', $this->getUser() ) &&
+ // Only do this if the revision isn't more than
6 hours older
+ // than the Max RC age (6h because the RC might
not be cleaned out regularly)
+ RecentChange::isInRCLifespan(
$this->mNewRev->getTimestamp(), 21600 ) &&
+ // Maybe the result is cached
+ !$cache->get( wfMemcKey( 'NotPatrollableRevId',
$this->mNewid ) )
+ ) {
+ // Look for an unpatrolled change corresponding
to this diff
+
+ $db = wfGetDB( DB_SLAVE );
+ $change = RecentChange::newFromConds(
+ array(
+ 'rc_timestamp' =>
$db->timestamp( $this->mNewRev->getTimestamp() ),
+ 'rc_this_oldid' =>
$this->mNewid,
+ 'rc_last_oldid' =>
$this->mOldid,
+ 'rc_patrolled' => 0
+ ),
+ __METHOD__,
+ array( 'USE INDEX' => 'rc_timestamp' )
+ );
+
+ if ( $change ) {
+ $rcid = $change->getAttribute( 'rc_id'
);
} else {
- // Look for an unpatrolled change
corresponding to this diff
- $db = wfGetDB( DB_SLAVE );
- $change = RecentChange::newFromConds(
- array(
- // Redundant user,timestamp
condition so we can use the existing index
- 'rc_user_text' =>
$this->mNewRev->getRawUserText(),
- 'rc_timestamp' =>
$db->timestamp( $this->mNewRev->getTimestamp() ),
- 'rc_this_oldid' =>
$this->mNewid,
- 'rc_last_oldid' =>
$this->mOldid,
- 'rc_patrolled' => 0
- ),
- __METHOD__
- );
- if ( $change instanceof RecentChange ) {
- $rcid =
$change->mAttribs['rc_id'];
- $this->mRcidMarkPatrolled =
$rcid;
- } else {
- // None found
- $rcid = 0;
- }
+ // None found
+ $rcid = 0;
}
// Build the link
if ( $rcid ) {
@@ -462,6 +461,7 @@
)
) . ']</span>';
} else {
+ $cache->set( wfMemcKey(
'NotPatrollableRevId', $this->mNewid ), '1', $wgRCMaxAge );
$this->mMarkPatrolledLink = '';
}
} else {
diff --git a/includes/specials/SpecialNewpages.php
b/includes/specials/SpecialNewpages.php
index fc6151f..ae04061 100644
--- a/includes/specials/SpecialNewpages.php
+++ b/includes/specials/SpecialNewpages.php
@@ -334,8 +334,11 @@
$query = array( 'redirect' => 'no' );
- if ( $this->patrollable( $result ) ) {
- $query['rcid'] = $result->rc_id;
+ if( $this->patrollable( $result ) ) {
+ // Tell Article.php that we want to patrol the first
revision
+ // and not the current one. Has effect if both
recentchages and new page
+ // patrolling are enabled, we set it everytime for link
consistency though.
+ $query['patrolpage'] = 1;
}
// Linker::linkKnown() uses 'known' and 'noclasses' options.
--
To view, visit https://gerrit.wikimedia.org/r/41196
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I1e24733cafbfdc51b7a5a9a1c1baf948e760fe1a
Gerrit-PatchSet: 22
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Hoo man <[email protected]>
Gerrit-Reviewer: Aaron Schulz <[email protected]>
Gerrit-Reviewer: Asher <[email protected]>
Gerrit-Reviewer: Aude <[email protected]>
Gerrit-Reviewer: Brian Wolff <[email protected]>
Gerrit-Reviewer: Brion VIBBER <[email protected]>
Gerrit-Reviewer: Catrope <[email protected]>
Gerrit-Reviewer: Hoo man <[email protected]>
Gerrit-Reviewer: Krinkle <[email protected]>
Gerrit-Reviewer: Matmarex <[email protected]>
Gerrit-Reviewer: Nemo bis <[email protected]>
Gerrit-Reviewer: Tim Starling <[email protected]>
Gerrit-Reviewer: jenkins-bot
Gerrit-Reviewer: preilly <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
