Brian Wolff has uploaded a new change for review.
https://gerrit.wikimedia.org/r/124137
Change subject: Make doEditContent call $dbw->rollback() if exception happens
......................................................................
Make doEditContent call $dbw->rollback() if exception happens
doEditContent calls a variety of methods in the middle of its
database transaction that can throw exceptions. For example if
$revision->insertOn() throws an exception, it can cause
referential integrity issues unless the transaction is rolled
back by something furhter up the chain (Thus see I5807e, I8f1da5).
It seems like it would be more reliable if doEditContent rolled
back its own transaction in exceptional circumstances. So this
patch puts everything in a try block, and does rollback if
an exception happens.
Bug: 63145
Bug: 32551
Change-Id: Icc44687da4edb1a72f13d2b95bfee2eea3ad6808
---
M includes/WikiPage.php
1 file changed, 120 insertions(+), 108 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/37/124137/1
diff --git a/includes/WikiPage.php b/includes/WikiPage.php
index 18409b0..a5d66df 100644
--- a/includes/WikiPage.php
+++ b/includes/WikiPage.php
@@ -1812,56 +1812,62 @@
}
$dbw->begin( __METHOD__ );
+ try {
- $prepStatus = $content->prepareSave( $this,
$flags, $baseRevId, $user );
- $status->merge( $prepStatus );
+ $prepStatus = $content->prepareSave(
$this, $flags, $baseRevId, $user );
+ $status->merge( $prepStatus );
- if ( !$status->isOK() ) {
- $dbw->rollback( __METHOD__ );
+ if ( !$status->isOK() ) {
+ $dbw->rollback( __METHOD__ );
- wfProfileOut( __METHOD__ );
- return $status;
- }
-
- $revisionId = $revision->insertOn( $dbw );
-
- // Update page
- //
- // Note that we use $this->mLatest instead of
fetching a value from the master DB
- // during the course of this function. This
makes sure that EditPage can detect
- // edit conflicts reliably, either by $ok here,
or by $article->getTimestamp()
- // before this function is called. A previous
function used a separate query, this
- // creates a window where concurrent edits can
cause an ignored edit conflict.
- $ok = $this->updateRevisionOn( $dbw, $revision,
$oldid, $oldIsRedirect );
-
- if ( !$ok ) {
- // Belated edit conflict! Run away!!
- $status->fatal( 'edit-conflict' );
-
- $dbw->rollback( __METHOD__ );
-
- wfProfileOut( __METHOD__ );
- return $status;
- }
-
- wfRunHooks( 'NewRevisionFromEditComplete',
array( $this, $revision, $baseRevId, $user ) );
- // Update recentchanges
- if ( !( $flags & EDIT_SUPPRESS_RC ) ) {
- // Mark as patrolled if the user can do
so
- $patrolled = $wgUseRCPatrol && !count(
-
$this->mTitle->getUserPermissionsErrors( 'autopatrol', $user ) );
- // Add RC row to the DB
- $rc = RecentChange::notifyEdit( $now,
$this->mTitle, $isminor, $user, $summary,
- $oldid, $this->getTimestamp(),
$bot, '', $oldsize, $newsize,
- $revisionId, $patrolled
- );
-
- // Log auto-patrolled edits
- if ( $patrolled ) {
- PatrolLog::record( $rc, true,
$user );
+ wfProfileOut( __METHOD__ );
+ return $status;
}
+ $revisionId = $revision->insertOn( $dbw
);
+
+ // Update page
+ //
+ // Note that we use $this->mLatest
instead of fetching a value from the master DB
+ // during the course of this function.
This makes sure that EditPage can detect
+ // edit conflicts reliably, either by
$ok here, or by $article->getTimestamp()
+ // before this function is called. A
previous function used a separate query, this
+ // creates a window where concurrent
edits can cause an ignored edit conflict.
+ $ok = $this->updateRevisionOn( $dbw,
$revision, $oldid, $oldIsRedirect );
+
+ if ( !$ok ) {
+ // Belated edit conflict! Run
away!!
+ $status->fatal( 'edit-conflict'
);
+
+ $dbw->rollback( __METHOD__ );
+
+ wfProfileOut( __METHOD__ );
+ return $status;
+ }
+
+ wfRunHooks(
'NewRevisionFromEditComplete', array( $this, $revision, $baseRevId, $user ) );
+ // Update recentchanges
+ if ( !( $flags & EDIT_SUPPRESS_RC ) ) {
+ // Mark as patrolled if the
user can do so
+ $patrolled = $wgUseRCPatrol &&
!count(
+
$this->mTitle->getUserPermissionsErrors( 'autopatrol', $user ) );
+ // Add RC row to the DB
+ $rc = RecentChange::notifyEdit(
$now, $this->mTitle, $isminor, $user, $summary,
+ $oldid,
$this->getTimestamp(), $bot, '', $oldsize, $newsize,
+ $revisionId, $patrolled
+ );
+
+ // Log auto-patrolled edits
+ if ( $patrolled ) {
+ PatrolLog::record( $rc,
true, $user );
+ }
+ }
+ $user->incEditCount();
+ } catch ( MWException $e ) {
+ $dbw->rollback( __METHOD__ );
+ // Question: Would it perhaps be better
if this method turned all
+ // exceptions into $status's?
+ throw $e;
}
- $user->incEditCount();
$dbw->commit( __METHOD__ );
} else {
// Bug 32948: revision ID must be set to page
{{REVISIONID}} and
@@ -1891,74 +1897,80 @@
$status->value['new'] = true;
$dbw->begin( __METHOD__ );
+ try {
- $prepStatus = $content->prepareSave( $this, $flags,
$baseRevId, $user );
- $status->merge( $prepStatus );
+ $prepStatus = $content->prepareSave( $this,
$flags, $baseRevId, $user );
+ $status->merge( $prepStatus );
- if ( !$status->isOK() ) {
- $dbw->rollback( __METHOD__ );
+ if ( !$status->isOK() ) {
+ $dbw->rollback( __METHOD__ );
- wfProfileOut( __METHOD__ );
- return $status;
- }
-
- $status->merge( $prepStatus );
-
- // Add the page record; stake our claim on this title!
- // This will return false if the article already exists
- $newid = $this->insertOn( $dbw );
-
- if ( $newid === false ) {
- $dbw->rollback( __METHOD__ );
- $status->fatal( 'edit-already-exists' );
-
- wfProfileOut( __METHOD__ );
- return $status;
- }
-
- // Save the revision text...
- $revision = new Revision( array(
- 'page' => $newid,
- 'title' => $this->getTitle(), // for
determining the default content model
- 'comment' => $summary,
- 'minor_edit' => $isminor,
- 'text' => $serialized,
- 'len' => $newsize,
- 'user' => $user->getId(),
- 'user_text' => $user->getName(),
- 'timestamp' => $now,
- 'content_model' => $content->getModel(),
- 'content_format' => $serialisation_format,
- ) );
- $revisionId = $revision->insertOn( $dbw );
-
- // Bug 37225: use accessor to get the text as Revision
may trim it
- $content = $revision->getContent(); // sanity; get
normalized version
-
- if ( $content ) {
- $newsize = $content->getSize();
- }
-
- // Update the page record with revision data
- $this->updateRevisionOn( $dbw, $revision, 0 );
-
- wfRunHooks( 'NewRevisionFromEditComplete', array(
$this, $revision, false, $user ) );
-
- // Update recentchanges
- if ( !( $flags & EDIT_SUPPRESS_RC ) ) {
- // Mark as patrolled if the user can do so
- $patrolled = ( $wgUseRCPatrol || $wgUseNPPatrol
) && !count(
-
$this->mTitle->getUserPermissionsErrors( 'autopatrol', $user ) );
- // Add RC row to the DB
- $rc = RecentChange::notifyNew( $now,
$this->mTitle, $isminor, $user, $summary, $bot,
- '', $newsize, $revisionId, $patrolled );
-
- // Log auto-patrolled edits
- if ( $patrolled ) {
- PatrolLog::record( $rc, true, $user );
+ wfProfileOut( __METHOD__ );
+ return $status;
}
+
+ $status->merge( $prepStatus );
+
+ // Add the page record; stake our claim on this
title!
+ // This will return false if the article
already exists
+ $newid = $this->insertOn( $dbw );
+
+ if ( $newid === false ) {
+ $dbw->rollback( __METHOD__ );
+ $status->fatal( 'edit-already-exists' );
+
+ wfProfileOut( __METHOD__ );
+ return $status;
+ }
+
+ // Save the revision text...
+ $revision = new Revision( array(
+ 'page' => $newid,
+ 'title' => $this->getTitle(), //
for determining the default content model
+ 'comment' => $summary,
+ 'minor_edit' => $isminor,
+ 'text' => $serialized,
+ 'len' => $newsize,
+ 'user' => $user->getId(),
+ 'user_text' => $user->getName(),
+ 'timestamp' => $now,
+ 'content_model' => $content->getModel(),
+ 'content_format' =>
$serialisation_format,
+ ) );
+ $revisionId = $revision->insertOn( $dbw );
+
+ // Bug 37225: use accessor to get the text as
Revision may trim it
+ $content = $revision->getContent(); // sanity;
get normalized version
+
+ if ( $content ) {
+ $newsize = $content->getSize();
+ }
+
+ // Update the page record with revision data
+ $this->updateRevisionOn( $dbw, $revision, 0 );
+
+ wfRunHooks( 'NewRevisionFromEditComplete',
array( $this, $revision, false, $user ) );
+
+ // Update recentchanges
+ if ( !( $flags & EDIT_SUPPRESS_RC ) ) {
+ // Mark as patrolled if the user can do
so
+ $patrolled = ( $wgUseRCPatrol ||
$wgUseNPPatrol ) && !count(
+
$this->mTitle->getUserPermissionsErrors( 'autopatrol', $user ) );
+ // Add RC row to the DB
+ $rc = RecentChange::notifyNew( $now,
$this->mTitle, $isminor, $user, $summary, $bot,
+ '', $newsize, $revisionId,
$patrolled );
+
+ // Log auto-patrolled edits
+ if ( $patrolled ) {
+ PatrolLog::record( $rc, true,
$user );
+ }
+ }
+ $user->incEditCount();
+
+ } catch ( MWException $e ) {
+ $dbw->rollback( __METHOD__ );
+ throw $e;
}
- $user->incEditCount();
$dbw->commit( __METHOD__ );
// Update links, etc.
--
To view, visit https://gerrit.wikimedia.org/r/124137
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icc44687da4edb1a72f13d2b95bfee2eea3ad6808
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Brian Wolff <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
