diff --git a/RELEASE-NOTES-1.36 b/RELEASE-NOTES-1.36 index 424d543f44b..c4c3be8ce96 100644 --- a/RELEASE-NOTES-1.36 +++ b/RELEASE-NOTES-1.36 @@ -106,6 +106,9 @@ For notes on 1.35.x and older releases, see HISTORY. indicates a later revision). The assumption is not always true and is hindering proper edit undoing in some cases, hence the logic has been removed. Reversing the paramaters will now lead to edit conflict or undefined behavior. +* (T263340) In history merging, pages with a content model that does not support + redirects will now be recorded as deleted if no revision is being left in the + source page (that's if all revisions of the page have been merged to another). * … === Action API changes in 1.36 === diff --git a/includes/MergeHistory.php b/includes/MergeHistory.php index 3e414735411..217be3946e6 100644 --- a/includes/MergeHistory.php +++ b/includes/MergeHistory.php @@ -354,13 +354,13 @@ class MergeHistory { __METHOD__ ); - // Make the source page a redirect if no revisions are left $haveRevisions = $this->dbw->lockForUpdate( 'revision', [ 'rev_page' => $this->source->getArticleID() ], __METHOD__ ); + // Update source page, histories and invalidate caches if ( !$haveRevisions ) { if ( $reason ) { $reason = wfMessage( @@ -377,56 +377,14 @@ class MergeHistory { )->inContentLanguage()->text(); } - $redirectContent = $this->contentHandlerFactory - ->getContentHandler( $this->source->getContentModel() ) - ->makeRedirectContent( - $this->dest, - wfMessage( 'mergehistory-redirect-text' )->inContentLanguage()->plain() - ); + $this->updateSourcePage( $status, $user, $reason ); - if ( $redirectContent ) { - $redirectComment = CommentStoreComment::newUnsavedComment( $reason ); - - $redirectRevRecord = new MutableRevisionRecord( $this->source ); - $redirectRevRecord->setContent( SlotRecord::MAIN, $redirectContent ); - $redirectRevRecord->setPageId( $this->source->getArticleID() ); - $redirectRevRecord->setComment( $redirectComment ); - $redirectRevRecord->setUser( $user ); - $redirectRevRecord->setTimestamp( wfTimestampNow() ); - - $insertedRevRecord = $this->revisionStore->insertRevisionOn( - $redirectRevRecord, - $this->dbw - ); - - $redirectPage = WikiPage::factory( $this->source ); - $redirectPage->updateRevisionOn( $this->dbw, $insertedRevRecord ); - - // Now, we record the link from the redirect to the new title. - // It should have no other outgoing links... - $this->dbw->delete( - 'pagelinks', - [ 'pl_from' => $this->dest->getArticleID() ], - __METHOD__ - ); - $this->dbw->insert( 'pagelinks', - [ - 'pl_from' => $this->dest->getArticleID(), - 'pl_from_namespace' => $this->dest->getNamespace(), - 'pl_namespace' => $this->dest->getNamespace(), - 'pl_title' => $this->dest->getDBkey() ], - __METHOD__ - ); - } else { - // Warning if we couldn't create the redirect - $status->warning( 'mergehistory-warning-redirect-not-created' ); - } } else { - $this->source->invalidateCache(); // update histories + $this->source->invalidateCache(); } - $this->dest->invalidateCache(); // update histories + $this->dest->invalidateCache(); - // Duplicate watchers of the old article to the new article on history merge + // Duplicate watchers of the old article to the new article $this->watchedItemStore->duplicateAllAssociatedEntries( $this->source, $this->dest ); // Update our logs @@ -447,4 +405,106 @@ class MergeHistory { return $status; } + + /** + * Do various cleanup work and updates to the source page. This method + * will only be called if no revision is remaining on the page. + * + * At the end, there would be either a redirect page or a deleted page, + * depending on whether the content model of the page supports redirects or not. + * + * @param Status $status + * @param User $user + * @param string $reason + * + * @return Status + */ + private function updateSourcePage( $status, $user, $reason ) { + $deleteSource = false; + $sourceModel = $this->source->getContentModel(); + $contentHandler = $this->contentHandlerFactory->getContentHandler( $sourceModel ); + + if ( !$contentHandler->supportsRedirects() ) { + $deleteSource = true; + $newContent = $contentHandler->makeEmptyContent(); + } else { + $msg = wfMessage( 'mergehistory-redirect-text' )->inContentLanguage()->plain(); + $newContent = $contentHandler->makeRedirectContent( $this->dest, $msg ); + } + + if ( !$newContent instanceof Content ) { + // Handler supports redirect but cannot create new redirect content? + // Not possible to proceed without Content. + + // @todo. Remove this once there's no evidence it's happening or if it's + // determined all violating handlers have been fixed. + // This is mostly kept because previous code was also blindly checking + // existing of the Content for both content models that supports redirects + // and those that that don't, so it's hard to know what it was masking. + $logger = MediaWiki\Logger\LoggerFactory::getInstance( 'ContentHandler' ); + $logger->warning( + 'ContentHandler for {model} says it supports redirects but failed ' + . 'to return Content object from ContentHandler::makeRedirectContent().' + . ' {value} returned instead.', + [ + 'value' => gettype( $newContent ), + 'model' => $sourceModel + ] + ); + + throw new InvalidArgumentException( + "ContentHandler for '$sourceModel' supports redirects" . + ' but cannot create redirect content during History merge.' + ); + } + + // T263340/T93469: Create revision record to also serve as the page revision. + // This revision will be used to create page content. If the source page's + // content model supports redirects, then it will be the redirect content. + // If the content model does not supports redirect, this content will aid + // proper deletion of the page below. + $comment = CommentStoreComment::newUnsavedComment( $reason ); + $newRevRecord = new MutableRevisionRecord( $this->source ); + $newRevRecord->setContent( SlotRecord::MAIN, $newContent ); + $newRevRecord->setPageId( $this->source->getArticleID() ); + $newRevRecord->setComment( $comment ); + $newRevRecord->setUser( $user ); + $newRevRecord->setTimestamp( wfTimestampNow() ); + + $insertedRevRecord = $this->revisionStore->insertRevisionOn( $newRevRecord, $this->dbw ); + + $newPage = WikiPage::factory( $this->source ); + $newPage->updateRevisionOn( $this->dbw, $insertedRevRecord ); + + if ( !$deleteSource ) { + // We have created a redirect page so let's + // record the link from the page to the new title. + // It should have no other outgoing links... + $this->dbw->delete( + 'pagelinks', + [ 'pl_from' => $this->dest->getArticleID() ], + __METHOD__ + ); + $this->dbw->insert( 'pagelinks', + [ + 'pl_from' => $this->dest->getArticleID(), + 'pl_from_namespace' => $this->dest->getNamespace(), + 'pl_namespace' => $this->dest->getNamespace(), + 'pl_title' => $this->dest->getDBkey() ], + __METHOD__ + ); + + } else { + // T263340/T93469: Delete the source page to prevent errors because its + // revisions are now tied to a different title and its content model + // does not support redirects, so we cannot leave a new revision on it. + // This deletion does not depend on userright but may still fails. If it + // fails, it will be communicated in the status reponse. + $reason = wfMessage( 'mergehistory-source-deleted-reason' )->inContentLanguage()->plain(); + $deletionStatus = $newPage->doDeleteArticleReal( $reason, $user ); + $status->merge( $deletionStatus ); + } + + return $status; + } } diff --git a/includes/specials/SpecialMergeHistory.php b/includes/specials/SpecialMergeHistory.php index 6e8ece35565..960c108b30b 100644 --- a/includes/specials/SpecialMergeHistory.php +++ b/includes/specials/SpecialMergeHistory.php @@ -413,9 +413,14 @@ class SpecialMergeHistory extends SpecialPage { [ 'redirect' => 'no' ] ); + // In some cases the target page will be deleted + $append = $targetTitle->exists( Title::READ_LATEST ) + ? '' + : $this->msg( 'mergehistory-source-deleted', $targetLink ); + $this->getOutput()->addWikiMsg( $this->msg( 'mergehistory-done' ) ->rawParams( $targetLink ) - ->params( $destTitle->getPrefixedText() ) + ->params( $destTitle->getPrefixedText(), $append ) ->numParams( $mh->getMergedRevisionCount() ) ); diff --git a/languages/i18n/en.json b/languages/i18n/en.json index d15a56b92d3..6e1bc880795 100644 --- a/languages/i18n/en.json +++ b/languages/i18n/en.json @@ -951,7 +951,7 @@ "mergehistory-go": "Show mergeable edits", "mergehistory-submit": "Merge revisions", "mergehistory-empty": "No revisions can be merged.", - "mergehistory-done": "$3 {{PLURAL:$3|revision|revisions}} of $1 {{PLURAL:$3|was|were}} merged into [[:$2]].", + "mergehistory-done": "$4 {{PLURAL:$4|revision|revisions}} of $1 {{PLURAL:$4|was|were}} merged into [[:$2]].\n$3.", "mergehistory-fail": "Unable to perform history merge, please recheck the page and time parameters.", "mergehistory-fail-bad-timestamp": "Timestamp is invalid.", "mergehistory-fail-invalid-source": "Source page is invalid.", @@ -970,9 +970,12 @@ "mergehistory-comment": "Merged [[:$1]] into [[:$2]]: $3", "mergehistory-same-destination": "Source and destination pages cannot be the same", "mergehistory-reason": "Reason:", + "mergehistory-source-deleted-reason": "Source page automatically deleted after history merge because its content model does not support redirects and no remaining revisions", + "mergehistory-source-deleted": "Additionally, [[:$1]] has been deleted because it no longer has any visible revisions and its content model does not support leaving redirects.", "mergehistory-revisionrow": "$1 ($2) $3 . . $4 $5 $6", "mergehistory-redirect-text": "", "mergelog": "Merge log", + "pagemerge-logentry": "merged [[$1]] into [[$2]] (revisions up to $3)", "revertmerge": "Unmerge", "mergelogpagetext": "Below is a list of the most recent merges of one page history into another.", diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json index a736a345719..c16be0a1a70 100644 --- a/languages/i18n/qqq.json +++ b/languages/i18n/qqq.json @@ -1191,6 +1191,8 @@ "mergehistory-reason": "{{Identical|Reason}}", "mergehistory-revisionrow": "{{Optional}}\nA revision row in the merge history page. Parameters:\n* $1 - a radio button to indicate a merge point\n* $2 - a link to the last revision of a page ({{msg-mw|Last}})\n* $3 - a page link\n* $4 - a user link\n* $5 - a revision size\n* $6 - a revision comment", "mergehistory-redirect-text": "{{ignored}}The text that's added to a redirected page when that redirect is created as part of a history merge.", + "mergehistory-source-deleted-reason": "Reason text given for deletion of a page after history merge if the source page will end up with no revision", + "mergehistory-source-deleted": "Additional note appended to {{msg-mw|mergehistory-done}} where applicable.", "mergelog": "{{doc-logpage}}\n\nThis is the name of a log of merge actions done on [[Special:MergeHistory]]. This special page and this log is not enabled by default.", "pagemerge-logentry": "{{ignored}}This is a ''logentry'' message only used on IRC.\n\nParameters:\n* $1 - the page name of the source of the content to be merged\n* $2 - the page into which the content is merged\n* $3 - a timestamp of limit\n\nThe log and its associated special page 'MergeHistory' is not enabled by default.\n\nPlease note that the parameters in a log entry will appear in the log only in the default language of the wiki. View [[Special:Log]] for examples on translatewiki.net with English default language.", "revertmerge": "Used as link text", diff --git a/tests/phpunit/includes/MergeHistoryTest.php b/tests/phpunit/includes/MergeHistoryTest.php index a2fd2b1ec63..e9af43be3fa 100644 --- a/tests/phpunit/includes/MergeHistoryTest.php +++ b/tests/phpunit/includes/MergeHistoryTest.php @@ -18,6 +18,10 @@ class MergeHistoryTest extends MediaWikiIntegrationTestCase { // Pages that will be merged $this->insertPage( 'Merge1' ); $this->insertPage( 'Merge2' ); + + // Exclusive for testSourceUpdateForNoRedirectSupport() + $this->insertPage( 'Merge3' ); + $this->insertPage( 'Merge4' ); } /** @@ -132,6 +136,65 @@ class MergeHistoryTest extends MediaWikiIntegrationTestCase { $this->assertEquals( $mh->getMergedRevisionCount(), 1 ); } + /** + * Test update to source page for pages with + * content model that supports redirects + * + * @covers MergeHistory::merge + */ + public function testSourceUpdateWithRedirectSupport() { + $title = Title::newFromText( 'Merge1' ); + $title2 = Title::newFromText( 'Merge2' ); + + $factory = MediaWikiServices::getInstance()->getMergeHistoryFactory(); + $mh = $factory->newMergeHistory( $title, $title2 ); + + $this->assertTrue( $title->exists() ); + + $status = $mh->merge( static::getTestSysop()->getUser() ); + + $this->assertTrue( $title->exists() ); + } + + /** + * Test update to source page for pages with + * content model that does not support redirects + * + * @covers MergeHistory::merge + */ + public function testSourceUpdateForNoRedirectSupport() { + $this->setMwGlobals( [ + 'wgExtraNamespaces' => [ + 2030 => 'NoRedirect', + 2030 => 'NoRedirect_talk' + ], + + 'wgNamespaceContentModels' => [ + 2030 => 'testing' + ], + 'wgContentHandlers' => [ + // Relies on the DummyContentHandlerForTesting not + // supporting redirects by default. If this ever gets + // changed this test has to be fixed. + 'testing' => DummyContentHandlerForTesting::class + ] + ] ); + + $title = Title::newFromText( 'Merge3' ); + $title->setContentModel( 'testing' ); + $title2 = Title::newFromText( 'Merge4' ); + $title2->setContentModel( 'testing' ); + + $factory = MediaWikiServices::getInstance()->getMergeHistoryFactory(); + $mh = $factory->newMergeHistory( $title, $title2 ); + + $this->assertTrue( $title->exists() ); + + $status = $mh->merge( static::getTestSysop()->getUser() ); + + $this->assertFalse( $title->exists() ); + } + /** * Test the old and new constructors work (though the old is deprecated) * @covers MergeHistory::__construct