Convert page creation to using startAtomic()/endAtomic()

A few semantic changes result from this:

* If multiple pages are created in a request, the updates happen
  in the same order relative to each other, but all in one second
  step instead of after each page edit.
* If an extension set some extra Status info or errors via the
  PageContentInsertComplete hook, they will not be seen by the
  caller (unless it was a CLI script possibly). Few callers use
  $status at all, and I did not see any that mutated it. Since
  the page is already committed when this hooks run (as has always
  been) they cannot veto edits and callers do not care or know what
  to do with random hook-set status errors; there was never much use
  in changing the Status anyway.

Bug: T120718
Change-Id: Ieba35056be31b2f648c57f59d19d3cbbe58f1b05

diff --git a/includes/db/IDatabase.php b/includes/db/IDatabase.php
index c72218a..cecb643 100644 (file)
--- a/includes/db/IDatabase.php
+++ b/includes/db/IDatabase.php
@@ -1219,6 +1219,8 @@ interface IDatabase {
         * after the database is updated so that the jobs will see the data when they actually run.
         * It can also be used for updates that easily cause deadlocks if locks are held too long.
         *
+        * Updates will execute in the order they were enqueued.
+        *
         * @param callable $callback
         * @since 1.20
         */
@@ -1232,6 +1234,8 @@ interface IDatabase {
         * This is useful for updates that easily cause deadlocks if locks are held too long
         * but where atomicity is strongly desired for these updates and some related updates.
         *
+        * Updates will execute in the order they were enqueued.
+        *
         * @param callable $callback
         * @since 1.22
         */

diff --git a/includes/page/WikiPage.php b/includes/page/WikiPage.php
index 6f4c296..c02f975 100644 (file)
--- a/includes/page/WikiPage.php
+++ b/includes/page/WikiPage.php
@@ -1823,7 +1823,7 @@ class WikiPage implements Page, IDBAccessObject {
                        $revisionId = $revision->insertOn( $dbw );
                        // Update page_latest and friends to reflect the new revision
                        if ( !$this->updateRevisionOn( $dbw, $revision, null, $meta['oldIsRedirect'] ) ) {
-                               $dbw->rollback( __METHOD__ );
+                               $dbw->rollback( __METHOD__ ); // sanity; this should never happen
                                throw new MWException( "Failed to update page row to use new revision." );
                        }
 
@@ -1921,12 +1921,12 @@ class WikiPage implements Page, IDBAccessObject {
                }
 
                $dbw = wfGetDB( DB_MASTER );
-               $dbw->begin( __METHOD__ );
+               $dbw->startAtomic( __METHOD__ );
 
                // Add the page record unless one already exists for the title
                $newid = $this->insertOn( $dbw );
                if ( $newid === false ) {
-                       $dbw->commit( __METHOD__ ); // nothing inserted
+                       $dbw->endAtomic( __METHOD__ ); // nothing inserted
                        $status->fatal( 'edit-already-exists' );
 
                        return $status; // nothing done
@@ -1956,7 +1956,7 @@ class WikiPage implements Page, IDBAccessObject {
                $revisionId = $revision->insertOn( $dbw );
                // Update the page record with revision data
                if ( !$this->updateRevisionOn( $dbw, $revision, 0 ) ) {
-                       $dbw->rollback( __METHOD__ );
+                       $dbw->rollback( __METHOD__ ); // sanity; this should never happen
                        throw new MWException( "Failed to update page row to use new revision." );
                }
 
@@ -1984,25 +1984,34 @@ class WikiPage implements Page, IDBAccessObject {
 
                $user->incEditCount();
 
-               $dbw->commit( __METHOD__ );
+               $dbw->endAtomic( __METHOD__ );
                $this->mTimestamp = $now;
 
-               // Update links, etc.
-               $this->doEditUpdates( $revision, $user, array( 'created' => true ) );
-
-               $hook_args = array( &$this, &$user, $content, $summary,
-                       $flags & EDIT_MINOR, null, null, &$flags, $revision );
-               ContentHandler::runLegacyHooks( 'ArticleInsertComplete', $hook_args );
-               Hooks::run( 'PageContentInsertComplete', $hook_args );
-
                // Return the new revision to the caller
                $status->value['revision'] = $revision;
 
-               // Trigger post-save hook
-               $hook_args = array( &$this, &$user, $content, $summary,
-                       $flags & EDIT_MINOR, null, null, &$flags, $revision, &$status, $meta['baseRevId'] );
-               ContentHandler::runLegacyHooks( 'ArticleSaveComplete', $hook_args );
-               Hooks::run( 'PageContentSaveComplete', $hook_args );
+               // Do secondary updates once the main changes have been committed...
+               $that = $this;
+               $dbw->onTransactionIdle(
+                       function () use (
+                               &$that, $dbw, $revision, &$user, $content, $summary, &$flags, $meta, &$status
+                       ) {
+                               // Do per-page updates in a transaction
+                               $dbw->setFlag( DBO_TRX );
+                               // Update links, etc.
+                               $that->doEditUpdates( $revision, $user, array( 'created' => true ) );
+                               // Trigger post-create hook
+                               $params = array( &$that, &$user, $content, $summary,
+                                       $flags & EDIT_MINOR, null, null, &$flags, $revision );
+                               ContentHandler::runLegacyHooks( 'ArticleInsertComplete', $params );
+                               Hooks::run( 'PageContentInsertComplete', $params );
+                               // Trigger post-save hook
+                               $params = array_merge( $params, array( &$status, $meta['baseRevId'] ) );
+                               ContentHandler::runLegacyHooks( 'ArticleSaveComplete', $params );
+                               Hooks::run( 'PageContentSaveComplete', $params );
+
+                       }
+               );
 
                return $status;
        }

diff --git a/tests/phpunit/includes/EditPageTest.php b/tests/phpunit/includes/EditPageTest.php
index 804b6ba..f113b1f 100644 (file)
--- a/tests/phpunit/includes/EditPageTest.php
+++ b/tests/phpunit/includes/EditPageTest.php
@@ -272,6 +272,25 @@ class EditPageTest extends MediaWikiLangTestCase {
        public function testCreatePage(
                $desc, $pageTitle, $user, $editText, $expectedCode, $expectedText, $ignoreBlank = false
        ) {
+               $checkId = null;
+
+               $this->setMwGlobals( 'wgHooks', array(
+                       'PageContentInsertComplete' => array( function (
+                               WikiPage &$page, User &$user, Content $content,
+                               $summary, $minor, $u1, $u2, &$flags, Revision $revision
+                       ) {
+                               // types/refs checked
+                       } ),
+                       'PageContentSaveComplete' => array( function (
+                               WikiPage &$page, User &$user, Content $content,
+                               $summary, $minor, $u1, $u2, &$flags, Revision $revision,
+                               Status &$status, $baseRevId
+                       ) use ( &$checkId ) {
+                               $checkId = $status->value['revision']->getId();
+                               // types/refs checked
+                       } ),
+               ) );
+
                $edit = array( 'wpTextbox1' => $editText );
                if ( $ignoreBlank ) {
                        $edit['wpIgnoreBlankArticle'] = 1;
@@ -280,7 +299,67 @@ class EditPageTest extends MediaWikiLangTestCase {
                $page = $this->assertEdit( $pageTitle, null, $user, $edit, $expectedCode, $expectedText, $desc );
 
                if ( $expectedCode != EditPage::AS_BLANK_ARTICLE ) {
+                       $latest = $page->getLatest();
+                       $page->doDeleteArticleReal( $pageTitle );
+
+                       $this->assertGreaterThan( 0, $latest, "Page revision ID updated in object" );
+                       $this->assertEquals( $latest, $checkId, "Revision in Status for hook" );
+               }
+       }
+
+       /**
+        * @dataProvider provideCreatePages
+        * @covers EditPage
+        */
+       public function testCreatePageTrx(
+               $desc, $pageTitle, $user, $editText, $expectedCode, $expectedText, $ignoreBlank = false
+       ) {
+               $checkIds = array();
+               $this->setMwGlobals( 'wgHooks', array(
+                       'PageContentInsertComplete' => array( function (
+                               WikiPage &$page, User &$user, Content $content,
+                               $summary, $minor, $u1, $u2, &$flags, Revision $revision
+                       ) {
+                               // types/refs checked
+                       } ),
+                       'PageContentSaveComplete' => array( function (
+                               WikiPage &$page, User &$user, Content $content,
+                               $summary, $minor, $u1, $u2, &$flags, Revision $revision,
+                               Status &$status, $baseRevId
+                       ) use ( &$checkIds ) {
+                               $checkIds[] = $status->value['revision']->getId();
+                               // types/refs checked
+                       } ),
+               ) );
+
+               wfGetDB( DB_MASTER )->begin( __METHOD__ );
+
+               $edit = array( 'wpTextbox1' => $editText );
+               if ( $ignoreBlank ) {
+                       $edit['wpIgnoreBlankArticle'] = 1;
+               }
+
+               $page = $this->assertEdit(
+                       $pageTitle, null, $user, $edit, $expectedCode, $expectedText, $desc );
+
+               $pageTitle2 = (string)$pageTitle . '/x';
+               $page2 = $this->assertEdit(
+                       $pageTitle2, null, $user, $edit, $expectedCode, $expectedText, $desc );
+
+               wfGetDB( DB_MASTER )->commit( __METHOD__ );
+
+               if ( $expectedCode != EditPage::AS_BLANK_ARTICLE ) {
+                       $latest = $page->getLatest();
                        $page->doDeleteArticleReal( $pageTitle );
+
+                       $this->assertGreaterThan( 0, $latest, "Page #1 revision ID updated in object" );
+                       $this->assertEquals( $latest, $checkIds[0], "Revision #1 in Status for hook" );
+
+                       $latest2 = $page2->getLatest();
+                       $page2->doDeleteArticleReal( $pageTitle2 );
+
+                       $this->assertGreaterThan( 0, $latest2, "Page #2 revision ID updated in object" );
+                       $this->assertEquals( $latest2, $checkIds[1], "Revision #2 in Status for hook" );
                }
        }