Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Improve handling of content models that do not support redirect.

Browse source 
To properly support content models that do not support redirects
during historymerge we have to do some cleanup, else after merging
all revisions of a page, a corrupted page will be left with a page id
(because it was not deleted) but no live revision (because they have
been all merged to the destination page).

This will lead to cascade of exceptions in Wikipage, RevisionStore,
RevisionStoreRecord, PoolWorkArticleView as well other various paths
that will attempt to interact with these, because page and revision
mismatch is considered a logic error almost everywhere.

The failure does not happen for content models that support redirects
because they are immediately creating new (latest) revision for the
old corrupted page. But we cannot require all content models to support
redirects, may not be feasible and can hinder forward compatibility.

This patch fixes this for content models that do not support redirect.

Now after merging all revisions of a page to another page, and the
source content model does not support redirect, empty content will be
created to aid proper deletion of the page afterwards.

Creating the content before deletion is necessary, else proper
deletion is not possible because many calls to revision-related methods
will throw exception during the deletion if we just use the original
corrupted page which does not have proper revisions now.

Bug: T93469
Bug: T263340
Change-Id: I07109445288633e3ddece4190f0c1c2b10372384
This commit is contained in:
Ammar Abdulhamid 2020-09-28 10:08:00 +01:00 committed by Ammarpad
parent 364f70d6ec
commit fb58d390ea
6 changed files with 185 additions and 49 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
RELEASE-NOTES-1.36
View file

@ -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 ===

154
includes/MergeHistory.php
View file

@ -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;
}
}

7
includes/specials/SpecialMergeHistory.php
View file

@ -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() )
);

5
languages/i18n/en.json
View file

@ -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.",

2
languages/i18n/qqq.json
View file

@ -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",

63
tests/phpunit/includes/MergeHistoryTest.php
View file

@ -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