diff --git a/docs/hooks.txt b/docs/hooks.txt index b452b94ff3e..708456c9261 100644 --- a/docs/hooks.txt +++ b/docs/hooks.txt @@ -2394,7 +2394,9 @@ $row: the database row for this page (the recentchanges record and a few extras edit. $wikiPage: the WikiPage edited $rev: the new revision -$baseID: the revision ID this was based off, if any +$originalRevId: if the edit restores or repeats an earlier revision (such as a + rollback or a null revision), the ID of that earlier revision. False otherwise. + (Used to be called $baseID.) $user: the editing user &$tags: tags to apply to the edit and recent change @@ -2502,7 +2504,9 @@ $flags: Flags passed to WikiPage::doEditContent() $revision: New Revision of the article (can be null for edits that change nothing) $status: Status object about to be returned by doEditContent() -$baseRevId: the rev ID (or false) this edit was based on +$originalRevId: if the edit restores or repeats an earlier revision (such as a + rollback or a null revision), the ID of that earlier revision. False otherwise. + (Used to be called $baseRevId.) $undidRevId: the rev ID (or 0) this edit undid 'PageHistoryBeforeList': When a history page list is about to be constructed. diff --git a/includes/Storage/PageUpdater.php b/includes/Storage/PageUpdater.php index 10caac401e5..7900210e2dc 100644 --- a/includes/Storage/PageUpdater.php +++ b/includes/Storage/PageUpdater.php @@ -112,15 +112,9 @@ class PageUpdater { private $ajaxEditStash = true; /** - * The ID of the logical base revision the content of the new revision is based on. - * Not to be confused with the immediate parent revision (the current revision before the - * new revision is created). - * The base revision is the last revision known to the client, while the parent revision - * is determined on the server by grabParentRevision(). - * * @var bool|int */ - private $baseRevId = false; + private $originalRevId = false; /** * @var array @@ -247,19 +241,19 @@ class PageUpdater { } /** - * Checks whether this update conflicts with another update performed since the specified base - * revision. A user level "edit conflict" is detected when the base revision known to the client - * and specified via setBaseRevisionId() is not the ID of the current revision before the - * update. If setBaseRevisionId() was not called, this method always returns false. + * Checks whether this update conflicts with another update performed between the client + * loading data to prepare an edit, and the client committing the edit. This is intended to + * detect user level "edit conflict" when the latest revision known to the client + * is no longer the current revision when processing the update. * - * Note that an update expected to be based on a non-existing page will have base revision ID 0, - * and is considered to have a conflict if a current revision exists (that is, the page was - * created since the base revision was determined by the client). + * An update expected to create a new page can be checked by setting $expectedParentRevision = 0. + * Such an update is considered to have a conflict if a current revision exists (that is, + * the page was created since the edit was initiated on the client). * * This method returning true indicates to calling code that edit conflict resolution should * be applied before saving any data. It does not prevent the update from being performed, and * it should not be confused with a "late" conflict indicated by the "edit-conflict" status. - * A "late" conflict is a CAS failure caused by an update being performed concurrently, between + * A "late" conflict is a CAS failure caused by an update being performed concurrently between * the time grabParentRevision() was called and the time saveRevision() trying to insert the * new revision. * @@ -269,22 +263,21 @@ class PageUpdater { * for the update to be fixed to the page's current revision at this point in time. * It acts as a compare-and-swap (CAS) token in that it is guaranteed that saveRevision() * will fail with the "edit-conflict" status if the current revision of the page changes after - * hasEditConflict() was called and before saveRevision() could insert a new revision. + * hasEditConflict() (or grabParentRevision()) was called and before saveRevision() could insert + * a new revision. * * @see grabParentRevision() * + * @param int $expectedParentRevision The ID of the revision the client expects to be the + * current one. Use 0 to indicate that the page is expected to not yet exist. + * * @return bool */ - public function hasEditConflict() { - $baseId = $this->getBaseRevisionId(); - if ( $baseId === false ) { - return false; - } - + public function hasEditConflict( $expectedParentRevision ) { $parent = $this->grabParentRevision(); $parentId = $parent ? $parent->getId() : 0; - return $parentId !== $baseId; + return $parentId !== $expectedParentRevision; } /** @@ -328,18 +321,13 @@ class PageUpdater { /** * Check flags and add EDIT_NEW or EDIT_UPDATE to them as needed. - * This also performs sanity checks against the base revision specified via setBaseRevisionId(). * * @param int $flags * @return int Updated $flags */ private function checkFlags( $flags ) { if ( !( $flags & EDIT_NEW ) && !( $flags & EDIT_UPDATE ) ) { - if ( $this->baseRevId === false ) { - $flags |= ( $this->derivedDataUpdater->pageExisted() ) ? EDIT_UPDATE : EDIT_NEW; - } else { - $flags |= ( $this->baseRevId > 0 ) ? EDIT_UPDATE : EDIT_NEW; - } + $flags |= ( $this->derivedDataUpdater->pageExisted() ) ? EDIT_UPDATE : EDIT_NEW; } return $flags; @@ -398,37 +386,29 @@ class PageUpdater { } /** - * Returns the ID of the logical base revision of the update. Not to be confused with the - * immediate parent revision. The base revision is set via setBaseRevisionId(), - * the parent revision is determined by grabParentRevision(). + * Returns the ID of an earlier revision that is being repeated or restored by this update. * - * Application may use this information to detect user level edit conflicts. Edit conflicts - * can be resolved by performing a 3-way merge, using the revision returned by this method as - * the common base of the conflicting revisions, namely the new revision being saved, - * and the revision returned by grabParentRevision(). - * - * @return bool|int The ID of the base revision, 0 if the base is a non-existing page, false - * if no base revision was specified. + * @return bool|int The original revision id, or false if no earlier revision is known to be + * repeated or restored by this update. */ - public function getBaseRevisionId() { - return $this->baseRevId; + public function getOriginalRevisionId() { + return $this->originalRevId; } /** - * Sets the ID of the revision the content of this update is based on, if any. - * The base revision ID is not to be confused with the new revision's parent revision: - * the parent revision is the page's current revision immediately before the new revision - * is created; the base revision indicates what revision the client based the content of - * the new revision on. If base revision and parent revision are not the same, the update is - * considered to require edit conflict resolution. + * Sets the ID of an earlier revision that is being repeated or restored by this update. + * The new revision is expected to have the exact same content as the given original revision. + * This is used with rollbacks and with dummy "null" revisions which are created to record + * things like page moves. * - * @param int|bool $baseRevId The ID of the base revision, or 0 if the update is expected to be - * performed on a non-existing page. false can be used to indicate that the caller - * doesn't care about the base revision. + * This value is passed to the PageContentSaveComplete and NewRevisionFromEditComplete hooks. + * + * @param int|bool $originalRevId The original revision id, or false if no earlier revision + * is known to be repeated or restored by this update. */ - public function setBaseRevisionId( $baseRevId ) { - Assert::parameterType( 'integer|boolean', $baseRevId, '$baseRevId' ); - $this->baseRevId = $baseRevId; + public function setOriginalRevisionId( $originalRevId ) { + Assert::parameterType( 'integer|boolean', $originalRevId, '$originalRevId' ); + $this->originalRevId = $originalRevId; } /** @@ -589,10 +569,9 @@ class PageUpdater { * changes after grabParentRevision() was called and before saveRevision() can insert * a new revision, as per the CAS mechanism described above. * - * However, the actual parent revision is allowed to be different from the revision set - * with setBaseRevisionId(). The caller is responsible for checking this via - * hasEditConflict() and adjusting the content of the new revision accordingly, - * using a 3-way-merge if desired. + * The caller is however responsible for calling hasEditConflict() to detect a + * user-level edit conflict, and to adjust the content of the new revision accordingly, + * e.g. by using a 3-way-merge. * * MCR migration note: this replaces WikiPage::doEditContent. Callers that change to using * saveRevision() now need to check the "minoredit" themselves before using EDIT_MINOR. @@ -660,9 +639,7 @@ class PageUpdater { // NOTE: This grabs the parent revision as the CAS token, if grabParentRevision // wasn't called yet. If the page is modified by another process before we are done with // it, this method must fail (with status 'edit-conflict')! - // NOTE: The actual parent revision may be different from $this->baseRevisionId. - // The caller is responsible for checking this via hasEditConflict and adjusting the - // content of the new revision accordingly, using a 3-way-merge. + // NOTE: The parent revision may be different from $this->baseRevisionId. $this->grabParentRevision(); $flags = $this->checkFlags( $flags ); @@ -987,7 +964,7 @@ class PageUpdater { $tags = $this->computeEffectiveTags( $flags ); Hooks::run( 'NewRevisionFromEditComplete', - [ $wikiPage, $newLegacyRevision, $this->baseRevId, $user, &$tags ] + [ $wikiPage, $newLegacyRevision, $this->getOriginalRevisionId(), $user, &$tags ] ); // Update recentchanges @@ -1064,7 +1041,7 @@ class PageUpdater { // TODO: replace legacy hook! // TODO: avoid pass-by-reference, see T193950 $params = [ &$wikiPage, &$user, $mainContent, $summary->text, $flags & EDIT_MINOR, - null, null, &$flags, $newLegacyRevision, &$status, $this->baseRevId, + null, null, &$flags, $newLegacyRevision, &$status, $this->getOriginalRevisionId(), $this->undidRevId ]; Hooks::run( 'PageContentSaveComplete', $params ); } @@ -1218,7 +1195,7 @@ class PageUpdater { Hooks::run( 'PageContentInsertComplete', $params ); // Trigger post-save hook // TODO: replace legacy hook! - $params = array_merge( $params, [ &$status, $this->baseRevId, 0 ] ); + $params = array_merge( $params, [ &$status, $this->getOriginalRevisionId(), 0 ] ); Hooks::run( 'PageContentSaveComplete', $params ); } ), diff --git a/includes/page/Article.php b/includes/page/Article.php index 1abf97470fa..51136fff98d 100644 --- a/includes/page/Article.php +++ b/includes/page/Article.php @@ -2114,11 +2114,11 @@ class Article implements Page { * @deprecated since 1.29. Use WikiPage::doEditContent() directly instead * @see WikiPage::doEditContent */ - public function doEditContent( Content $content, $summary, $flags = 0, $baseRevId = false, + public function doEditContent( Content $content, $summary, $flags = 0, $originalRevId = false, User $user = null, $serialFormat = null ) { wfDeprecated( __METHOD__, '1.29' ); - return $this->mPage->doEditContent( $content, $summary, $flags, $baseRevId, + return $this->mPage->doEditContent( $content, $summary, $flags, $originalRevId, $user, $serialFormat ); } diff --git a/includes/page/WikiPage.php b/includes/page/WikiPage.php index 5bbdb6cdeca..fd58a362f58 100644 --- a/includes/page/WikiPage.php +++ b/includes/page/WikiPage.php @@ -1742,9 +1742,10 @@ class WikiPage implements Page, IDBAccessObject { * error will be returned. These two conditions are also possible with * auto-detection due to MediaWiki's performance-optimised locking strategy. * - * @param bool|int $baseRevId The revision ID this edit was based off, if any. - * This is not the parent revision ID, rather the revision ID for older - * content used as the source for a rollback, for example. + * @param bool|int $originalRevId: The ID of an original revision that the edit + * restores or repeats. The new revision is expected to have the exact same content as + * the given original revision. This is used with rollbacks and with dummy "null" revisions + * which are created to record things like page moves. * @param User $user The user doing the edit * @param string $serialFormat IGNORED. * @param array|null $tags Change tags to apply to this edit @@ -1771,7 +1772,7 @@ class WikiPage implements Page, IDBAccessObject { * @throws MWException */ public function doEditContent( - Content $content, $summary, $flags = 0, $baseRevId = false, + Content $content, $summary, $flags = 0, $originalRevId = false, User $user = null, $serialFormat = null, $tags = [], $undidRevId = 0 ) { global $wgUser, $wgUseNPPatrol, $wgUseRCPatrol; @@ -1796,7 +1797,7 @@ class WikiPage implements Page, IDBAccessObject { // used by this PageUpdater. However, there is no guarantee for this. $updater = $this->newPageUpdater( $user ); $updater->setContent( 'main', $content ); - $updater->setBaseRevisionId( $baseRevId ); + $updater->setOriginalRevisionId( $originalRevId ); $updater->setUndidRevisionId( $undidRevId ); $needsPatrol = $wgUseRCPatrol || ( $wgUseNPPatrol && !$this->exists() ); diff --git a/tests/phpunit/includes/Storage/PageUpdaterTest.php b/tests/phpunit/includes/Storage/PageUpdaterTest.php index 24107b1d1f8..bdabf9c0cb5 100644 --- a/tests/phpunit/includes/Storage/PageUpdaterTest.php +++ b/tests/phpunit/includes/Storage/PageUpdaterTest.php @@ -58,12 +58,9 @@ class PageUpdaterTest extends MediaWikiTestCase { $oldStats = $this->db->selectRow( 'site_stats', '*', '1=1' ); $this->assertFalse( $updater->wasCommitted(), 'wasCommitted' ); - $this->assertFalse( $updater->getBaseRevisionId(), 'getBaseRevisionId' ); + $this->assertFalse( $updater->getOriginalRevisionId(), 'getOriginalRevisionId' ); $this->assertSame( 0, $updater->getUndidRevisionId(), 'getUndidRevisionId' ); - $updater->setBaseRevisionId( 0 ); - $this->assertSame( 0, $updater->getBaseRevisionId(), 'getBaseRevisionId' ); - $updater->addTag( 'foo' ); $updater->addTags( [ 'bar', 'qux' ] ); @@ -77,10 +74,12 @@ class PageUpdaterTest extends MediaWikiTestCase { $parent = $updater->grabParentRevision(); - // TODO: test that hasEditConflict() grabs the parent revision $this->assertNull( $parent, 'getParentRevision' ); $this->assertFalse( $updater->wasCommitted(), 'wasCommitted' ); - $this->assertFalse( $updater->hasEditConflict(), 'hasEditConflict' ); + + // TODO: test that hasEditConflict() grabs the parent revision + $this->assertFalse( $updater->hasEditConflict( 0 ), 'hasEditConflict' ); + $this->assertTrue( $updater->hasEditConflict( 1 ), 'hasEditConflict' ); // TODO: test failure with EDIT_UPDATE // TODO: test EDIT_MINOR, EDIT_BOT, etc @@ -158,10 +157,12 @@ class PageUpdaterTest extends MediaWikiTestCase { $oldStats = $this->db->selectRow( 'site_stats', '*', '1=1' ); - // TODO: test page update does not fail with mismatching base rev ID - $baseRev = $title->getLatestRevID( Title::GAID_FOR_UPDATE ); - $updater->setBaseRevisionId( $baseRev ); - $this->assertSame( $baseRev, $updater->getBaseRevisionId(), 'getBaseRevisionId' ); + $updater->setOriginalRevisionId( 7 ); + $this->assertSame( 7, $updater->getOriginalRevisionId(), 'getOriginalRevisionId' ); + + $this->assertFalse( $updater->hasEditConflict( $parentId ), 'hasEditConflict' ); + $this->assertTrue( $updater->hasEditConflict( $parentId - 1 ), 'hasEditConflict' ); + $this->assertTrue( $updater->hasEditConflict( 0 ), 'hasEditConflict' ); // TODO: MCR: test additional slots $updater->setContent( 'main', new TextContent( 'Lorem Ipsum' ) ); @@ -332,48 +333,6 @@ class PageUpdaterTest extends MediaWikiTestCase { $this->assertTrue( $status->hasMessage( 'edit-already-exists' ), 'edit-already-exists' ); } - /** - * @covers \MediaWiki\Storage\PageUpdater::saveRevision() - * @covers \MediaWiki\Storage\PageUpdater::setBaseRevisionId() - */ - public function testFailureOnBaseRevision() { - $user = $this->getTestUser()->getUser(); - - $title = $this->getDummyTitle( __METHOD__ ); - - // start editing non-existing page - $page = WikiPage::factory( $title ); - $updater = $page->newPageUpdater( $user ); - - // update for base revision 7 should fail - $summary = CommentStoreComment::newUnsavedComment( 'udpate?!' ); - $updater->setBaseRevisionId( 7 ); // expect page to exist - $updater->setContent( 'main', new TextContent( 'Lorem ipsum' ) ); - $updater->saveRevision( $summary ); - $status = $updater->getStatus(); - - $this->assertFalse( $updater->wasSuccessful(), 'wasSuccessful()' ); - $this->assertNull( $updater->getNewRevision(), 'getNewRevision()' ); - $this->assertFalse( $status->isOK(), 'getStatus()->isOK()' ); - $this->assertTrue( $status->hasMessage( 'edit-gone-missing' ), 'edit-gone-missing' ); - - // create the page - $this->createRevision( $page, __METHOD__ ); - - // update for base revision 0 should fail - $summary = CommentStoreComment::newUnsavedComment( 'create?!' ); - $updater = $page->newPageUpdater( $user ); - $updater->setBaseRevisionId( 0 ); // expect page to not exist - $updater->setContent( 'main', new TextContent( 'dolor sit amet' ) ); - $updater->saveRevision( $summary ); - $status = $updater->getStatus(); - - $this->assertFalse( $updater->wasSuccessful(), 'wasSuccessful()' ); - $this->assertNull( $updater->getNewRevision(), 'getNewRevision()' ); - $this->assertFalse( $status->isOK(), 'getStatus()->isOK()' ); - $this->assertTrue( $status->hasMessage( 'edit-already-exists' ), 'edit-already-exists' ); - } - public function provideSetRcPatrolStatus( $patrolled ) { yield [ RecentChange::PRC_UNPATROLLED ]; yield [ RecentChange::PRC_AUTOPATROLLED ];