Cen.temp has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/329545 )
Change subject: Refactor maybeMakeEditReviewed to handle all self-reverts,
avoid db hits
......................................................................
Refactor maybeMakeEditReviewed to handle all self-reverts, avoid db hits
This refactors maybeMakeEditReviewed to check text hashes rather than
baseRevIds which are presently only given for rollbacks. They could
also be given for undoes, but it still wouldn't detect manual reverts.
This allows to simplify the function. In addition, some master reads
are avoided.
Change-Id: I37ebd972775510d3b8515c76237920ae8314ae4e
---
M backend/FlaggedRevs.hooks.php
1 file changed, 46 insertions(+), 101 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/FlaggedRevs
refs/changes/45/329545/1
diff --git a/backend/FlaggedRevs.hooks.php b/backend/FlaggedRevs.hooks.php
index 593906c..93b6756 100755
--- a/backend/FlaggedRevs.hooks.php
+++ b/backend/FlaggedRevs.hooks.php
@@ -384,8 +384,6 @@
public static function maybeMakeEditReviewed(
Page $article, $rev, $baseRevId = false, $user = null
) {
- global $wgRequest;
-
$title = $article->getTitle(); // convenience
# Edit must be non-null, to a reviewable page, with $user set
$fa = FlaggableWikiPage::getTitleInstance( $title );
@@ -397,100 +395,66 @@
$title->resetArticleID( $rev->getPage() ); // Avoid extra DB
hit and lag issues
# Get what was just the current revision ID
$prevRevId = $rev->getParentId();
- # Get edit timestamp. Existance already validated by
EditPage.php.
- $editTimestamp = $wgRequest->getVal( 'wpEdittime' );
- # Is the page manually checked off to be reviewed?
- if ( $editTimestamp
- && $wgRequest->getCheck( 'wpReviewEdit' )
- && $title->getUserPermissionsErrors( 'review', $user )
=== array() )
+ # Get edit timestamp. Existence already validated by
EditPage.php.
+ $editTimestamp = $user->getRequest()->getVal( 'wpEdittime' );
+ # Case A: The page is manually checked off to be reviewed by a
reviewer.
+ # Check that the timestamps match to guard against auto-merges
of unreviewed edits
+ if ( $editTimestamp && $user->getRequest()->getCheck(
'wpReviewEdit' )
+ && $title->getUserPermissionsErrors( 'review', $user )
=== array()
+ && self::editCheckReview( $fa, $rev, $user,
$editTimestamp ) )
{
- if ( self::editCheckReview( $fa, $rev, $user,
$editTimestamp ) ) {
- return true; // reviewed...done!
- }
+ $flags = null; // review flags (null => default flags)
+ FlaggedRevs::autoReviewEdit( $article, $user, $rev,
$flags, false /* manual */ );
+ return true; // reviewed...done!
}
# All cases below require auto-review of edits to be enabled
if ( !FlaggedRevs::autoReviewEnabled() ) {
return true; // short-circuit
}
- # If a $baseRevId is passed in, the edit is using an old
revision's text
- $isOldRevCopy = (bool)$baseRevId; // null edit or rollback
- # Get the revision ID the incoming one was based off...
- if ( !$baseRevId && $prevRevId ) {
- $prevTimestamp = Revision::getTimestampFromId(
- $title, $prevRevId, Revision::READ_LATEST );
- # The user just made an edit. The one before that
should have
- # been the current version. If not reflected in
wpEdittime, an
- # edit may have been auto-merged in between, in that
case, discard
- # the baseRevId given from the client.
- if ( $editTimestamp && $prevTimestamp ===
$editTimestamp ) {
- $baseRevId = $wgRequest->getInt( 'baseRevId' );
- }
- # If baseRevId not given, assume the previous revision
ID (for bots).
- # For auto-merges, this also occurs since the given ID
is ignored.
- if ( !$baseRevId ) {
- $baseRevId = $prevRevId;
- }
- }
- $frev = null; // flagged rev this edit was based on
- $flags = null; // review flags (null => default flags)
$srev = $fa->getStableRev();
- # Case A: this user can auto-review edits. Check if either:
+ # Case B: this user can auto-review edits. Check if either:
# (a) this new revision creates a new page and new page
autoreview is enabled
- # (b) this new revision is based on an old, reviewed, revision
+ # (b) this new revision is based on an old, reviewed, revision,
in that case
+ # either it is an edit to a stable version or it is a revert to
a stable version
if ( $title->getUserPermissionsErrors( 'autoreview', $user )
=== array() ) {
- # For rollback/null edits, use the previous ID as the
alternate base ID.
- # Otherwise, use the 'altBaseRevId' parameter passed in
by the request.
- $altBaseRevId = $isOldRevCopy ? $prevRevId :
$wgRequest->getInt( 'altBaseRevId' );
if ( !$prevRevId ) { // New pages
- $reviewableNewPage =
FlaggedRevs::autoReviewNewPages();
- $reviewableChange = false;
- } else { // Edits to existing pages
- $reviewableNewPage = false; // had previous rev
- # If a edit was automatically merged, do not
trust 'baseRevId' (bug 33481).
- # Do this by verifying the user-provided
edittime against the prior revision.
- $prevRevTimestamp =
Revision::getTimestampFromId(
- $title, $prevRevId,
Revision::READ_LATEST );
- if ( $editTimestamp && $editTimestamp !==
$prevRevTimestamp ) {
- $baseRevId = $prevRevId;
- $altBaseRevId = 0;
+ if ( FlaggedRevs::autoReviewNewPages() ) {
+ $flags = null; // review flags (null =>
default flags)
+ # This is a new page, review it.
+ FlaggedRevs::autoReviewEdit( $fa,
$user, $rev, $flags );
}
- # Check if the base revision was reviewed...
- if ( FlaggedRevs::autoReviewEdits() ) {
- $frev = FlaggedRevision::newFromTitle(
$title, $baseRevId, FR_MASTER );
- if ( !$frev && $altBaseRevId ) {
- $frev =
FlaggedRevision::newFromTitle( $title, $altBaseRevId, FR_MASTER );
+ } else { // Edits to existing pages
+ if ( $srev ) {
+ if ( $prevRevId ===
$srev->getRevision()->getId() ) {
+ # This is an edit directly to a
stable version, review it.
+ $flags = null; // review flags
(null => default flags)
+ FlaggedRevs::autoReviewEdit(
$fa, $user, $rev, $flags );
+ } elseif ( $rev->getSha1() ===
$srev->getRevision()->getSha1() ) {
+ # T59073: If a user with
autoreview returns the page to its last stable
+ # version, it should be marked
stable, regardless of the method used to do so.
+ # Hence, don't rely on
baseRevId, instead check text hashes.
+ $flags = $srev->getTags(); //
null edits & rollbacks keep previous tags
+ FlaggedRevs::autoReviewEdit(
$fa, $user, $rev, $flags );
}
}
- $reviewableChange = $frev ||
- # Bug 57073: If a user with autoreview
returns the page to its last stable
- # version, it should be marked stable,
regardless of the method used to do so.
- ( $srev && $rev->getSha1() ===
$srev->getRevision()->getSha1() );
}
- # Is this an edit directly to a reviewed version or a
new page?
- if ( $reviewableNewPage || $reviewableChange ) {
- if ( $isOldRevCopy && $frev ) {
- $flags = $frev->getTags(); // null
edits & rollbacks keep previous tags
- }
- # Review this revision of the page...
- FlaggedRevs::autoReviewEdit( $fa, $user, $rev,
$flags );
- }
- # Case B: the user cannot autoreview edits. Check if either:
- # (a) this is a rollback to the stable version
+ # Case C: the user cannot autoreview edits. Check if either:
+ # (a) this is a reversion to the stable version and the user
can restore stable versions
# (b) this is a self-reversion to the stable version
# These are subcases of making a new revision based on an old,
reviewed, revision.
} elseif ( FlaggedRevs::autoReviewEdits() && $srev ) {
- # Check for rollbacks...
+ $srevId = $srev->getRevision()->getId();
+ # Check for reversions to the stable rev...
$reviewableChange = (
- $isOldRevCopy && // rollback or null edit
- $baseRevId != $prevRevId && // not a null edit
- $baseRevId == $srev->getRevId() && // restored
stable rev
+ $prevRevId > $srevId && // a revert requires at
least two edits since the stable rev
+ $rev->getSha1() ===
$srev->getRevision()->getSha1() && // restored stable rev
$title->getUserPermissionsErrors(
'autoreviewrestore', $user ) === array()
);
- # Check for self-reversions (checks text hashes)...
- if ( !$reviewableChange ) {
- $reviewableChange = self::isSelfRevertToStable(
$rev, $srev, $baseRevId, $user );
+ # Check for self-reversions to the stable rev...
+ if ( !$reviewableChange && $prevRevId > $srevId ) {
+ $reviewableChange = self::isSelfRevertToStable(
$rev, $srev, $user );
}
- # Is this a rollback or self-reversion to the stable
rev?
+ # Is this a reversion or self-reversion to the stable
rev?
if ( $reviewableChange ) {
$flags = $srev->getTags(); // use old tags
# Review this revision of the page...
@@ -503,7 +467,7 @@
// Review $rev if $editTimestamp matches the previous revision's
timestamp.
// Otherwise, review the revision that has $editTimestamp as its
timestamp value.
protected static function editCheckReview(
- Page $article, $rev, $user, $editTimestamp
+ Page $article, Revision $rev, User $user, $editTimestamp
) {
$prevTimestamp = null;
$prevRevId = $rev->getParentId(); // revision before $rev
@@ -525,46 +489,27 @@
return false; // not flagged?
}
}
- $flags = null;
- # Review this revision of the page...
- return FlaggedRevs::autoReviewEdit( $article, $user, $rev,
$flags, false /* manual */ );
+ return true;
}
/**
* Check if a user reverted himself to the stable version
*/
protected static function isSelfRevertToStable(
- Revision $rev, $srev, $baseRevId, $user
+ Revision $rev, FlaggedRevision $srev, User $user
) {
- if ( !$srev || $baseRevId != $srev->getRevId() ) {
- return false; // user reports they are not the same
+ if ( $rev->getSha1() !== $srev->getRevision()->getSha1() ) {
+ return false; // texts do not coincide
}
$dbw = wfGetDB( DB_MASTER );
- # Such a revert requires 1+ revs between it and the stable
- $revertedRevs = $dbw->selectField( 'revision', '1',
- array(
- 'rev_page' => $rev->getPage(),
- 'rev_id > ' . intval( $baseRevId ), // stable
rev
- 'rev_id < ' . intval( $rev->getId() ), // this
rev
- 'rev_user_text' => $user->getName()
- ), __METHOD__
- );
- if ( !$revertedRevs ) {
- return false; // can't be a revert
- }
# Check that this user is ONLY reverting his/herself.
- $otherUsers = $dbw->selectField( 'revision', '1',
+ return !$dbw->selectField( 'revision', '1',
array(
'rev_page' => $rev->getPage(),
- 'rev_id > ' . intval( $baseRevId ),
+ 'rev_id > ' . intval(
$srev->getRevision()->getId() ),
'rev_user_text != ' . $dbw->addQuotes(
$user->getName() )
), __METHOD__
);
- if ( $otherUsers ) {
- return false; // only looking for self-reverts
- }
- # Confirm the text because we can't trust this user.
- return ( $rev->getSha1() === $srev->getRevision()->getSha1() );
}
/**
--
To view, visit https://gerrit.wikimedia.org/r/329545
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I37ebd972775510d3b8515c76237920ae8314ae4e
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/FlaggedRevs
Gerrit-Branch: master
Gerrit-Owner: Cen.temp <cenarium.t...@gmail.com>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
