aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorTim Starling <tstarling@wikimedia.org>2022-11-29 14:38:29 +1100
committerTim Starling <tstarling@wikimedia.org>2022-12-05 22:03:45 +0000
commit540bddfb1fb3ec7172ba556a507b5ceffda72754 (patch)
tree8bdd07d2b7065012d90752c4795b9b21e350f06e
parent6c175865f40982aee9c6657280d097ece81d69f6 (diff)
downloadmediawikicore-540bddfb1fb3ec7172ba556a507b5ceffda72754.tar.gz
mediawikicore-540bddfb1fb3ec7172ba556a507b5ceffda72754.zip
When content is marked bad, show an error, don't pretend it is empty
It misrepresents the users contribution to show empty text for a revision when in fact the revision contained some text which we later lost. Also, errors from SqlBlobStore::fetchBlobs() did not stop a cache entry from being written, so a subsequent cache hit would show the bad revision as empty. So, in Storage: * Add BadBlobException, which is thrown by the Storage layer to indicate that a revision is marked as bad. * Have SqlBlobStore::getBlobStore() return an error for bad blobs instead of an empty string. * Duplicate the check for flags=error into SqlBlobStore::expandBlob(). This avoids an unnecessary cache fetch, and avoids making decompressData() throw on error, which would be a b/c break. * In SqlBlobStore::getBlob(), suppress the cache when there was an error. In Revision: * Add BadRevisionException, to wrap BadBlobException in the Revision layer. * Return null from RevisionRecord::getContent() on a broader set of errors. Make it mostly non-throwing. * Add RevisionRecord::getContentOrThrow() which returns a non-nullable Content. * Note that SlotRecord::getContent() returns a non-nullable Content so now throws in more cases. In the UI: * In Article::view(), catch the exception and show an error message. * In DifferenceEngine, catch the exception and make a suitable error message available via getRevisionLoadErrors(). In the diff page, show the error message in a box. * In ApiComparePages and the legacy rvdiffto, show a warning. * In RawAction, show a 404 by analogy with other error cases. * In EditPage, there was already handling for $content=null with an appropriate error message (missing-revision-content). But having $this->textbox1 = null caused PHP 8.1 deprecation warnings, so I fixed that. * In EditPage undo, there was already handling for null content, but I improved the error message: "does not exist or was deleted" seems more appropriate than "conflicting intermediate edits". Change-Id: Idd1278d6d756ef37d64addb7b5f3be30747ea603
-rw-r--r--autoload.php2
-rw-r--r--includes/EditPage.php19
-rw-r--r--includes/Revision/BadRevisionException.php12
-rw-r--r--includes/Revision/RenderedRevision.php32
-rw-r--r--includes/Revision/RevisionRecord.php31
-rw-r--r--includes/Revision/RevisionStore.php10
-rw-r--r--includes/Revision/SlotRecord.php2
-rw-r--r--includes/Storage/BadBlobException.php11
-rw-r--r--includes/Storage/SqlBlobStore.php61
-rw-r--r--includes/api/ApiComparePages.php3
-rw-r--r--includes/api/ApiQueryRevisionsBase.php3
-rw-r--r--includes/diff/DifferenceEngine.php84
-rw-r--r--includes/page/Article.php10
-rw-r--r--languages/i18n/en.json3
-rw-r--r--languages/i18n/qqq.json3
-rw-r--r--tests/phpunit/includes/Storage/SqlBlobStoreTest.php4
-rw-r--r--tests/phpunit/unit/includes/Revision/MutableRevisionRecordTest.php30
-rw-r--r--tests/phpunit/unit/includes/Revision/RevisionRecordTests.php32
18 files changed, 297 insertions, 55 deletions
diff --git a/autoload.php b/autoload.php
index 970505ed062e..b136a23125a3 100644
--- a/autoload.php
+++ b/autoload.php
@@ -1732,6 +1732,7 @@ $wgAutoloadLocalClasses = [
'MediaWiki\\Rest\\Validator\\ParamValidatorCallbacks' => __DIR__ . '/includes/Rest/Validator/ParamValidatorCallbacks.php',
'MediaWiki\\Rest\\Validator\\Validator' => __DIR__ . '/includes/Rest/Validator/Validator.php',
'MediaWiki\\Revision\\ArchivedRevisionLookup' => __DIR__ . '/includes/Revision/ArchivedRevisionLookup.php',
+ 'MediaWiki\\Revision\\BadRevisionException' => __DIR__ . '/includes/Revision/BadRevisionException.php',
'MediaWiki\\Revision\\ContributionsLookup' => __DIR__ . '/includes/Revision/ContributionsLookup.php',
'MediaWiki\\Revision\\ContributionsSegment' => __DIR__ . '/includes/Revision/ContributionsSegment.php',
'MediaWiki\\Revision\\FallbackSlotRoleHandler' => __DIR__ . '/includes/Revision/FallbackSlotRoleHandler.php',
@@ -1873,6 +1874,7 @@ $wgAutoloadLocalClasses = [
'MediaWiki\\Specials\\Contribute\\Card\\ContributeCardActionLink' => __DIR__ . '/includes/specials/Contribute/Card/ContributeCardActionLink.php',
'MediaWiki\\Specials\\Contribute\\ContributeFactory' => __DIR__ . '/includes/specials/Contribute/ContributeFactory.php',
'MediaWiki\\Specials\\Contribute\\Hook\\ContributeCardsHook' => __DIR__ . '/includes/specials/Contribute/Hook/ContributeCardsHook.php',
+ 'MediaWiki\\Storage\\BadBlobException' => __DIR__ . '/includes/Storage/BadBlobException.php',
'MediaWiki\\Storage\\BlobAccessException' => __DIR__ . '/includes/Storage/BlobAccessException.php',
'MediaWiki\\Storage\\BlobStore' => __DIR__ . '/includes/Storage/BlobStore.php',
'MediaWiki\\Storage\\BlobStoreFactory' => __DIR__ . '/includes/Storage/BlobStoreFactory.php',
diff --git a/includes/EditPage.php b/includes/EditPage.php
index a033994abc39..b36a65979763 100644
--- a/includes/EditPage.php
+++ b/includes/EditPage.php
@@ -1518,7 +1518,7 @@ class EditPage implements IEditObject {
if ( $content === false ) {
# Warn the user that something went wrong
- $undoMsg = 'failure';
+ $undoMsg = 'norev';
}
}
@@ -1664,13 +1664,16 @@ class EditPage implements IEditObject {
$undoContent = $undoRev->getContent( SlotRecord::MAIN );
$undoAfterContent = $oldRev->getContent( SlotRecord::MAIN );
$undoIsLatest = $this->page->getRevisionRecord()->getId() === $undoRev->getId();
+ if ( $currentContent === null
+ || $undoContent === null
+ || $undoAfterContent === null
+ ) {
+ return false;
+ }
return $handler->getUndoContent(
- // @phan-suppress-next-line PhanTypeMismatchArgumentNullable Content is for public use
$currentContent,
- // @phan-suppress-next-line PhanTypeMismatchArgumentNullable Content is for public use
$undoContent,
- // @phan-suppress-next-line PhanTypeMismatchArgumentNullable Content is for public use
$undoAfterContent,
$undoIsLatest
);
@@ -3074,13 +3077,16 @@ class EditPage implements IEditObject {
* content.
*
* @param Content|null|false|string $content
- * @return string|false|null The editable text form of the content.
+ * @return string The editable text form of the content.
*
* @throws MWException If $content is not an instance of TextContent and
* $this->allowNonTextContent is not true.
*/
private function toEditText( $content ) {
- if ( $content === null || $content === false || is_string( $content ) ) {
+ if ( $content === null || $content === false ) {
+ return '';
+ }
+ if ( is_string( $content ) ) {
return $content;
}
@@ -3876,7 +3882,6 @@ class EditPage implements IEditObject {
);
$this->context->getOutput()->addHTML(
- // @phan-suppress-next-line PhanTypeMismatchArgumentNullable False positive, text is not null
Html::textarea( $name, $builder->addNewLineAtEnd( $text ), $attribs )
);
}
diff --git a/includes/Revision/BadRevisionException.php b/includes/Revision/BadRevisionException.php
new file mode 100644
index 000000000000..fc99ef1a5084
--- /dev/null
+++ b/includes/Revision/BadRevisionException.php
@@ -0,0 +1,12 @@
+<?php
+
+namespace MediaWiki\Revision;
+
+/**
+ * Exception raised when the text of a revision is permanently missing or
+ * corrupt. This wraps BadBlobException which is thrown by the Storage layer.
+ * To mark a revision as permanently missing, use findBadBlobs.php.
+ */
+class BadRevisionException extends RevisionAccessException {
+
+}
diff --git a/includes/Revision/RenderedRevision.php b/includes/Revision/RenderedRevision.php
index ae0b84b97937..b6d87a2188ef 100644
--- a/includes/Revision/RenderedRevision.php
+++ b/includes/Revision/RenderedRevision.php
@@ -226,29 +226,23 @@ class RenderedRevision implements SlotRenderingProvider {
if ( !isset( $this->slotsOutput[ $role ] )
|| ( $withHtml && !$this->slotsOutput[ $role ]->hasText() )
) {
- $content = $this->revision->getContent( $role, $this->audience, $this->performer );
+ $content = $this->revision->getContentOrThrow( $role, $this->audience, $this->performer );
- if ( !$content ) {
- throw new SuppressedDataException(
- 'Access to the content has been suppressed for this audience'
- );
- } else {
- // XXX: allow SlotRoleHandler to control the ParserOutput?
- $output = $this->getSlotParserOutputUncached( $content, $withHtml );
-
- if ( $withHtml && !$output->hasText() ) {
- throw new LogicException(
- 'HTML generation was requested, but '
- . get_class( $content )
- . ' that passed to '
- . 'ContentRenderer::getParserOutput() returns a ParserOutput with no text set.'
- );
- }
+ // XXX: allow SlotRoleHandler to control the ParserOutput?
+ $output = $this->getSlotParserOutputUncached( $content, $withHtml );
- // Detach watcher, to ensure option use is not recorded in the wrong ParserOutput.
- $this->options->registerWatcher( null );
+ if ( $withHtml && !$output->hasText() ) {
+ throw new LogicException(
+ 'HTML generation was requested, but '
+ . get_class( $content )
+ . ' that passed to '
+ . 'ContentRenderer::getParserOutput() returns a ParserOutput with no text set.'
+ );
}
+ // Detach watcher, to ensure option use is not recorded in the wrong ParserOutput.
+ $this->options->registerWatcher( null );
+
$this->slotsOutput[ $role ] = $output;
}
diff --git a/includes/Revision/RevisionRecord.php b/includes/Revision/RevisionRecord.php
index 0953a07a0cd5..f089f772ce09 100644
--- a/includes/Revision/RevisionRecord.php
+++ b/includes/Revision/RevisionRecord.php
@@ -145,21 +145,40 @@ abstract class RevisionRecord implements WikiAwareEntity {
* Note that for mutable Content objects, each call to this method will return a
* fresh clone.
*
- * MCR migration note: this replaced Revision::getContent
+ * Use getContentOrThrow() for more specific error information.
*
* @param string $role The role name of the desired slot
* @param int $audience
* @param Authority|null $performer user on whose behalf to check
*
- * @return Content|null The content of the given slot, or null if access is forbidden.
+ * @return Content|null The content of the given slot, or null on error
*/
public function getContent( $role, $audience = self::FOR_PUBLIC, Authority $performer = null ): ?Content {
- // XXX: throwing an exception would be nicer, but would a further
- // departure from the old signature of Revision::getContent() when it existed,
- // and thus result in more complex and error prone refactoring.
- if ( !$this->audienceCan( self::DELETED_TEXT, $audience, $performer ) ) {
+ try {
+ $content = $this->getSlot( $role, $audience, $performer )->getContent();
+ } catch ( BadRevisionException | SuppressedDataException $e ) {
return null;
}
+ return $content->copy();
+ }
+
+ /**
+ * Get the Content of the given slot of this revision.
+ *
+ * @param string $role The role name of the desired slot
+ * @param int $audience
+ * @param Authority|null $performer user on whose behalf to check
+ *
+ * @return Content
+ * @throws SuppressedDataException if the content is not viewable by the given audience
+ * @throws BadRevisionException if the content is missing or corrupted
+ * @throws RevisionAccessException
+ */
+ public function getContentOrThrow( $role, $audience = self::FOR_PUBLIC, Authority $performer = null ): Content {
+ if ( !$this->audienceCan( self::DELETED_TEXT, $audience, $performer ) ) {
+ throw new SuppressedDataException(
+ 'Access to the content has been suppressed for this audience' );
+ }
$content = $this->getSlot( $role, $audience, $performer )->getContent();
return $content->copy();
diff --git a/includes/Revision/RevisionStore.php b/includes/Revision/RevisionStore.php
index 910c6f9df6e0..9d1c98a16d0b 100644
--- a/includes/Revision/RevisionStore.php
+++ b/includes/Revision/RevisionStore.php
@@ -45,6 +45,7 @@ use MediaWiki\Page\PageIdentity;
use MediaWiki\Page\PageIdentityValue;
use MediaWiki\Page\PageStore;
use MediaWiki\Permissions\Authority;
+use MediaWiki\Storage\BadBlobException;
use MediaWiki\Storage\BlobAccessException;
use MediaWiki\Storage\BlobStore;
use MediaWiki\Storage\NameTableStore;
@@ -1166,7 +1167,12 @@ class RevisionStore
// No blob flags, so use the blob verbatim.
$data = $blobData;
} else {
- $data = $this->blobStore->expandBlob( $blobData, $blobFlags, $blobAddress );
+ try {
+ $data = $this->blobStore->expandBlob( $blobData, $blobFlags, $blobAddress );
+ } catch ( BadBlobException $e ) {
+ throw new BadRevisionException( $e->getMessage(), [], 0, $e );
+ }
+
if ( $data === false ) {
throw new RevisionAccessException(
'Failed to expand blob data using flags {flags} (key: {cache_key})',
@@ -1182,6 +1188,8 @@ class RevisionStore
$address = $slot->getAddress();
try {
$data = $this->blobStore->getBlob( $address, $queryFlags );
+ } catch ( BadBlobException $e ) {
+ throw new BadRevisionException( $e->getMessage(), [], 0, $e );
} catch ( BlobAccessException $e ) {
throw new RevisionAccessException(
'Failed to load data blob from {address}'
diff --git a/includes/Revision/SlotRecord.php b/includes/Revision/SlotRecord.php
index 2059dd3dcd1b..0435680713ec 100644
--- a/includes/Revision/SlotRecord.php
+++ b/includes/Revision/SlotRecord.php
@@ -310,6 +310,8 @@ class SlotRecord {
*
* @throws SuppressedDataException if access to the content is not allowed according
* to the audience check performed by RevisionRecord::getSlot().
+ * @throws BadRevisionException if the revision is permanently missing
+ * @throws RevisionAccessException for other storage access errors
*
* @return Content The slot's content. This is a direct reference to the internal instance,
* copy before exposing to application logic!
diff --git a/includes/Storage/BadBlobException.php b/includes/Storage/BadBlobException.php
new file mode 100644
index 000000000000..e32dc33489ea
--- /dev/null
+++ b/includes/Storage/BadBlobException.php
@@ -0,0 +1,11 @@
+<?php
+
+namespace MediaWiki\Storage;
+
+/**
+ * Exception thrown when a blob has the "bad" content address schema, or has
+ * "error" in its old_flags, meaning it is permanently missing.
+ */
+class BadBlobException extends BlobAccessException {
+
+}
diff --git a/includes/Storage/SqlBlobStore.php b/includes/Storage/SqlBlobStore.php
index 58e2565d235b..20e12dd861ed 100644
--- a/includes/Storage/SqlBlobStore.php
+++ b/includes/Storage/SqlBlobStore.php
@@ -265,13 +265,20 @@ class SqlBlobStore implements IDBAccessObject, BlobStore {
[ $result, $errors ] = $this->fetchBlobs( [ $blobAddress ], $queryFlags );
// No negative caching; negative hits on text rows may be due to corrupted replica DBs
$error = $errors[$blobAddress] ?? null;
+ if ( $error ) {
+ $ttl = WANObjectCache::TTL_UNCACHEABLE;
+ }
return $result[$blobAddress];
},
$this->getCacheOptions()
);
if ( $error ) {
- throw new BlobAccessException( $error );
+ if ( $error[0] === 'badrevision' ) {
+ throw new BadBlobException( $error[1] );
+ } else {
+ throw new BlobAccessException( $error[1] );
+ }
}
Assert::postcondition( is_string( $blob ), 'Blob must not be null' );
@@ -301,10 +308,9 @@ class SqlBlobStore implements IDBAccessObject, BlobStore {
}, $blobsByAddress );
$result = StatusValue::newGood( $blobsByAddress );
- if ( $errors ) {
- foreach ( $errors as $error ) {
- $result->warning( 'internalerror', $error );
- }
+ foreach ( $errors as $error ) {
+ // @phan-suppress-next-line PhanParamTooFewUnpack
+ $result->warning( ...$error );
}
return $result;
}
@@ -316,8 +322,12 @@ class SqlBlobStore implements IDBAccessObject, BlobStore {
* @param int $queryFlags
*
* @throws BlobAccessException
- * @return array [ $result, $errors ] A map of blob addresses to successfully fetched blobs
- * or false if fetch failed, plus and array of errors
+ * @return array [ $result, $errors ] A list with the following elements:
+ * - The result: a map of blob addresses to successfully fetched blobs
+ * or false if fetch failed
+ * - Errors: a map of blob addresses to error information about the blob.
+ * On success, the relevant key will be absent. Each error is a list of
+ * parameters to be passed to StatusValue::warning().
*/
private function fetchBlobs( $blobAddresses, $queryFlags ) {
$textIdToBlobAddress = [];
@@ -336,26 +346,33 @@ class SqlBlobStore implements IDBAccessObject, BlobStore {
// TODO: MCR: also support 'ex' schema with ExternalStore URLs, plus flags encoded in the URL!
if ( $schema === 'bad' ) {
- // Database row was marked as "known bad", no need to trigger an error.
+ // Database row was marked as "known bad"
wfDebug(
__METHOD__
. ": loading known-bad content ($blobAddress), returning empty string"
);
$result[$blobAddress] = '';
- continue;
+ $errors[$blobAddress] = [
+ 'badrevision',
+ 'The content of this revision is missing or corrupted (bad schema)'
+ ];
} elseif ( $schema === 'tt' ) {
$textId = intval( $id );
if ( $textId < 1 || $id !== (string)$textId ) {
- $errors[$blobAddress] = "Bad blob address: $blobAddress."
- . ' Use findBadBlobs.php to remedy.';
+ $errors[$blobAddress] = [
+ 'internalerror',
+ "Bad blob address: $blobAddress. Use findBadBlobs.php to remedy."
+ ];
$result[$blobAddress] = false;
}
$textIdToBlobAddress[$textId] = $blobAddress;
} else {
- $errors[$blobAddress] = "Unknown blob address schema: $schema."
- . ' Use findBadBlobs.php to remedy.';
+ $errors[$blobAddress] = [
+ 'internalerror',
+ "Unknown blob address schema: $schema. Use findBadBlobs.php to remedy."
+ ];
$result[$blobAddress] = false;
}
}
@@ -414,8 +431,10 @@ class SqlBlobStore implements IDBAccessObject, BlobStore {
$blob = $this->expandBlob( $row->old_text, $row->old_flags, $blobAddress );
}
if ( $blob === false ) {
- $errors[$blobAddress] = "Bad data in text row {$row->old_id}."
- . ' Use findBadBlobs.php to remedy.';
+ $errors[$blobAddress] = [
+ 'internalerror',
+ "Bad data in text row {$row->old_id}. Use findBadBlobs.php to remedy."
+ ];
}
$result[$blobAddress] = $blob;
}
@@ -424,8 +443,10 @@ class SqlBlobStore implements IDBAccessObject, BlobStore {
if ( count( $result ) !== count( $blobAddresses ) ) {
foreach ( $blobAddresses as $blobAddress ) {
if ( !isset( $result[$blobAddress ] ) ) {
- $errors[$blobAddress] = "Unable to fetch blob at $blobAddress."
- . ' Use findBadBlobs.php to remedy.';
+ $errors[$blobAddress] = [
+ 'internalerror',
+ "Unable to fetch blob at $blobAddress. Use findBadBlobs.php to remedy."
+ ];
$result[$blobAddress] = false;
}
}
@@ -482,11 +503,17 @@ class SqlBlobStore implements IDBAccessObject, BlobStore {
* caching is disabled.
*
* @return false|string The expanded blob or false on failure
+ * @throws BlobAccessException
*/
public function expandBlob( $raw, $flags, $blobAddress = null ) {
if ( is_string( $flags ) ) {
$flags = explode( ',', $flags );
}
+ if ( in_array( 'error', $flags ) ) {
+ throw new BadBlobException(
+ "The content of this revision is missing or corrupted (error flag)"
+ );
+ }
// Use external methods for external objects, text in table is URL-only then
if ( in_array( 'external', $flags ) ) {
diff --git a/includes/api/ApiComparePages.php b/includes/api/ApiComparePages.php
index 4275fc6242fb..7341bca2109d 100644
--- a/includes/api/ApiComparePages.php
+++ b/includes/api/ApiComparePages.php
@@ -216,6 +216,9 @@ class ApiComparePages extends ApiBase {
$difftext[$role] = $de->getDiffBodyForRole( $role );
}
}
+ foreach ( $de->getRevisionLoadErrors() as $msg ) {
+ $this->addWarning( $msg );
+ }
// Fill in the response
$vals = [];
diff --git a/includes/api/ApiQueryRevisionsBase.php b/includes/api/ApiQueryRevisionsBase.php
index cb55ac07527e..5a9bd450ada9 100644
--- a/includes/api/ApiQueryRevisionsBase.php
+++ b/includes/api/ApiQueryRevisionsBase.php
@@ -711,6 +711,9 @@ abstract class ApiQueryRevisionsBase extends ApiQueryGeneratorBase {
if ( !$engine->wasCacheHit() ) {
$this->numUncachedDiffs++;
}
+ foreach ( $engine->getRevisionLoadErrors() as $msg ) {
+ $this->addWarning( $msg );
+ }
}
} else {
$vals['diff']['notcached'] = true;
diff --git a/includes/diff/DifferenceEngine.php b/includes/diff/DifferenceEngine.php
index d316082d3112..11f898e61d9b 100644
--- a/includes/diff/DifferenceEngine.php
+++ b/includes/diff/DifferenceEngine.php
@@ -29,6 +29,7 @@ use MediaWiki\MediaWikiServices;
use MediaWiki\Page\WikiPageFactory;
use MediaWiki\Permissions\Authority;
use MediaWiki\Permissions\PermissionStatus;
+use MediaWiki\Revision\BadRevisionException;
use MediaWiki\Revision\RevisionRecord;
use MediaWiki\Revision\RevisionStore;
use MediaWiki\Revision\SlotRecord;
@@ -235,6 +236,9 @@ class DifferenceEngine extends ContextSource {
/** @var UserOptionsLookup */
private $userOptionsLookup;
+ /** @var Message[] */
+ private $revisionLoadErrors = [];
+
/** #@- */
/**
@@ -343,8 +347,14 @@ class DifferenceEngine extends ContextSource {
$slots = [];
foreach ( $roles as $role ) {
$slots[$role] = [
- 'old' => isset( $oldSlots[$role] ) ? $oldSlots[$role]->getContent() : null,
- 'new' => isset( $newSlots[$role] ) ? $newSlots[$role]->getContent() : null,
+ 'old' => $this->loadSingleSlot(
+ $oldSlots[$role] ?? null,
+ 'old'
+ ),
+ 'new' => $this->loadSingleSlot(
+ $newSlots[$role] ?? null,
+ 'new'
+ )
];
}
// move main slot to front
@@ -354,6 +364,59 @@ class DifferenceEngine extends ContextSource {
return $slots;
}
+ /**
+ * Load the content of a single slot record
+ *
+ * @param SlotRecord|null $slot
+ * @param string $which "new" or "old"
+ * @return Content|null
+ */
+ private function loadSingleSlot( ?SlotRecord $slot, string $which ) {
+ if ( !$slot ) {
+ return null;
+ }
+ try {
+ return $slot->getContent();
+ } catch ( BadRevisionException $e ) {
+ $this->addRevisionLoadError( $which );
+ return null;
+ }
+ }
+
+ /**
+ * Set a message to show as a notice at the top of the page
+ *
+ * @param string $which "new" or "old"
+ */
+ private function addRevisionLoadError( $which ) {
+ $this->revisionLoadErrors[] = $this->msg( $which === 'new'
+ ? 'difference-bad-new-revision' : 'difference-bad-old-revision'
+ );
+ }
+
+ /**
+ * If errors were encountered while loading the revision contents, this
+ * will return an array of Messages describing the errors.
+ *
+ * @return Message[]
+ */
+ public function getRevisionLoadErrors() {
+ return $this->revisionLoadErrors;
+ }
+
+ /**
+ * Determine whether there was an error loading the new revision
+ * @return bool
+ */
+ private function hasNewRevisionLoadError() {
+ foreach ( $this->revisionLoadErrors as $error ) {
+ if ( $error->getKey() === 'difference-bad-new-revision' ) {
+ return true;
+ }
+ }
+ return false;
+ }
+
/** @inheritDoc */
public function getTitle() {
// T202454 avoid errors when there is no title
@@ -841,6 +904,13 @@ class DifferenceEngine extends ContextSource {
$msg = $suppressed ? 'rev-suppressed-diff-view' : 'rev-deleted-diff-view';
$notice = Html::warningBox( $this->msg( $msg )->parse(), 'plainlinks' );
}
+
+ # Add an error if the content can't be loaded
+ $this->getSlotContents();
+ foreach ( $this->getRevisionLoadErrors() as $msg ) {
+ $notice .= Html::warningBox( $msg->parse() );
+ }
+
$this->showDiff( $oldHeader, $newHeader, $notice );
if ( !$diffOnly ) {
$this->renderNewRevision();
@@ -990,6 +1060,10 @@ class DifferenceEngine extends ContextSource {
// even if it's unsaved, but a lot of untangling is required to do it safely.
return;
}
+ if ( $this->hasNewRevisionLoadError() ) {
+ // There was an error loading the new revision
+ return;
+ }
$out->setRevisionId( $this->mNewid );
$out->setRevisionIsCurrent( $this->mNewRevisionRecord->isCurrent() );
@@ -1815,6 +1889,9 @@ class DifferenceEngine extends ContextSource {
// revision that's not readable to the user, but check it just in case.
$this->mOldContent = $oldRevision->getContent( SlotRecord::MAIN,
RevisionRecord::FOR_THIS_USER, $this->getAuthority() );
+ if ( !$this->mOldContent ) {
+ $this->addRevisionLoadError( 'old' );
+ }
} else {
$this->mOldPage = null;
$this->mOldRevisionRecord = $this->mOldid = false;
@@ -1824,6 +1901,9 @@ class DifferenceEngine extends ContextSource {
$this->mNewPage = Title::newFromLinkTarget( $newRevision->getPageAsLinkTarget() );
$this->mNewContent = $newRevision->getContent( SlotRecord::MAIN,
RevisionRecord::FOR_THIS_USER, $this->getAuthority() );
+ if ( !$this->mNewContent ) {
+ $this->addRevisionLoadError( 'new' );
+ }
$this->mRevisionsIdsLoaded = $this->mRevisionsLoaded = true;
$this->mTextLoaded = $oldRevision ? 2 : 1;
diff --git a/includes/page/Article.php b/includes/page/Article.php
index 570a521c07c2..ddad836be070 100644
--- a/includes/page/Article.php
+++ b/includes/page/Article.php
@@ -26,6 +26,7 @@ use MediaWiki\MediaWikiServices;
use MediaWiki\Page\ParserOutputAccess;
use MediaWiki\Permissions\Authority;
use MediaWiki\Permissions\PermissionStatus;
+use MediaWiki\Revision\BadRevisionException;
use MediaWiki\Revision\RevisionRecord;
use MediaWiki\Revision\RevisionStore;
use MediaWiki\Revision\SlotRecord;
@@ -521,8 +522,13 @@ class Article implements Page {
}
$poOptions += [ 'includeDebugInfo' => true ];
- $continue =
- $this->generateContentOutput( $authority, $parserOptions, $oldid, $outputPage, $poOptions );
+ try {
+ $continue =
+ $this->generateContentOutput( $authority, $parserOptions, $oldid, $outputPage, $poOptions );
+ } catch ( BadRevisionException $e ) {
+ $continue = false;
+ $this->showViewError( wfMessage( 'badrevision' )->text() );
+ }
if ( !$continue ) {
return;
diff --git a/languages/i18n/en.json b/languages/i18n/en.json
index 796788c38177..5730f0dc55fa 100644
--- a/languages/i18n/en.json
+++ b/languages/i18n/en.json
@@ -1006,6 +1006,8 @@
"diff-paragraph-moved-tonew": "Paragraph was moved. Click to jump to new location.",
"diff-paragraph-moved-toold": "Paragraph was moved. Click to jump to old location.",
"difference-missing-revision": "{{PLURAL:$2|One revision|$2 revisions}} of this difference ($1) {{PLURAL:$2|was|were}} not found.\n\nThis is usually caused by following an outdated diff link to a page that has been deleted.\nDetails can be found in the [{{fullurl:{{#Special:Log}}/delete|page={{FULLPAGENAMEE}}}} deletion log].",
+ "difference-bad-old-revision": "The content of the old revision is missing or corrupted.",
+ "difference-bad-new-revision": "The content of the new revision is missing or corrupted.",
"search-summary": "",
"searchresults": "Search results",
"search-filter-title-prefix": "Only searching in pages whose title starts with \"$1\"",
@@ -2211,6 +2213,7 @@
"notargettext": "You have not specified a target page or user to perform this function on.",
"nopagetitle": "No such target page",
"nopagetext": "The target page you have specified does not exist.",
+ "badrevision": "The text of this revision is missing or corrupted.",
"pager-newer-n": "{{PLURAL:$1|newer 1|newer $1}}",
"pager-older-n": "{{PLURAL:$1|older 1|older $1}}",
"suppress": "Suppress",
diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json
index 1cbfe23170b8..d4560bee849f 100644
--- a/languages/i18n/qqq.json
+++ b/languages/i18n/qqq.json
@@ -1249,6 +1249,8 @@
"diff-paragraph-moved-tonew": "Explaining title tag for the indicating symbols when a paragraph was moved hinting to the new location in the Diff view.",
"diff-paragraph-moved-toold": "Explaining title tag for the indicating symbols when a paragraph was moved hinting to the old location in the Diff view.",
"difference-missing-revision": "Text displayed when the requested revision does not exist using a diff link.\n\nExample: [{{canonicalurl:Project:News|diff=426850&oldid=99999999}} Diff with invalid revision#]\n\nParameters:\n* $1 - the list of missing revision IDs\n* $2 - the number of items in $1 (one or two)",
+ "difference-bad-old-revision": "Text displayed when the old (left-hand side) revision of a diff cannot be loaded due to database corruption.",
+ "difference-bad-new-revision": "Text displayed when the new (right-hand side) revision of a diff cannot be loaded due to database corruption.",
"search-summary": "{{doc-specialpagesummary|search}}",
"searchresults": "This is the title of the page that contains the results of a search.\n\n{{Identical|Search results}}",
"search-filter-title-prefix": "Subtitle added to indicate that the user is filtering for pages whose title starts with $1, \n* $1 - the title prefix",
@@ -2454,6 +2456,7 @@
"notargettext": "Used as error message in [[Special:MovePage]].\n\nSee also:\n* {{msg-mw|Notargettitle|title}}\n* {{msg-mw|Notargettext|text}}",
"nopagetitle": "Used as title on special pages like [[Special:MovePage]] (when the oldtitle does not exist), [[Special:Diff]] or [[Special:PermaLink]].\n\nThe text is {{msg-mw|nopagetext}}.\n\nSee also:\n* {{msg-mw|Nopagetitle|title}}\n* {{msg-mw|Nopagetext|text}}",
"nopagetext": "Used as text on special pages like [[Special:MovePage]] (when the oldtitle does not exist) or [[Special:PermaLink]].\n\nThe title is {{msg-mw|nopagetitle}}.\n\nSee also:\n* {{msg-mw|Nopagetitle|title}}\n* {{msg-mw|Nopagetext|text}}",
+ "badrevision": "Used as the text of the page if the real text cannot be loaded due to database corruption.",
"pager-newer-n": "This is part of the navigation message on the top and bottom of Special pages which are lists of things in date order, e.g. the User's contributions page. It is passed as the second argument of {{msg-mw|Viewprevnext}}. $1 is the number of items shown per page.\n{{Identical|Newer}}",
"pager-older-n": "This is part of the navigation message on the top and bottom of Special pages which are lists of things in date order, e.g. the User's contributions page. It is passed as the first argument of {{msg-mw|Viewprevnext}}. $1 is the number of items shown per page.",
"suppress": "{{Identical|Suppress}}",
diff --git a/tests/phpunit/includes/Storage/SqlBlobStoreTest.php b/tests/phpunit/includes/Storage/SqlBlobStoreTest.php
index ecf7bb75288a..30eaf89dd481 100644
--- a/tests/phpunit/includes/Storage/SqlBlobStoreTest.php
+++ b/tests/phpunit/includes/Storage/SqlBlobStoreTest.php
@@ -6,6 +6,7 @@ use ExternalStoreAccess;
use ExternalStoreFactory;
use HashBagOStuff;
use InvalidArgumentException;
+use MediaWiki\Storage\BadBlobException;
use MediaWiki\Storage\BlobAccessException;
use MediaWiki\Storage\SqlBlobStore;
use MediaWikiIntegrationTestCase;
@@ -214,7 +215,8 @@ class SqlBlobStoreTest extends MediaWikiIntegrationTestCase {
*/
public function testSimpleStoreGetBlobKnownBad() {
$store = $this->getBlobStore();
- $this->assertSame( '', $store->getBlob( 'bad:lost?bug=T12345' ) );
+ $this->expectException( BadBlobException::class );
+ $store->getBlob( 'bad:lost?bug=T12345' );
}
/**
diff --git a/tests/phpunit/unit/includes/Revision/MutableRevisionRecordTest.php b/tests/phpunit/unit/includes/Revision/MutableRevisionRecordTest.php
index 16fc8f4d603e..b8e5ccd07955 100644
--- a/tests/phpunit/unit/includes/Revision/MutableRevisionRecordTest.php
+++ b/tests/phpunit/unit/includes/Revision/MutableRevisionRecordTest.php
@@ -7,6 +7,7 @@ use DummyContentForTesting;
use InvalidArgumentException;
use MediaWiki\Page\PageIdentity;
use MediaWiki\Page\PageIdentityValue;
+use MediaWiki\Revision\BadRevisionException;
use MediaWiki\Revision\MutableRevisionRecord;
use MediaWiki\Revision\MutableRevisionSlots;
use MediaWiki\Revision\RevisionAccessException;
@@ -434,4 +435,33 @@ class MutableRevisionRecordTest extends MediaWikiUnitTestCase {
);
$record->audienceCan( RevisionRecord::DELETED_TEXT, RevisionRecord::FOR_THIS_USER );
}
+
+ public function testGetContent_bad() {
+ $record = new MutableRevisionRecord(
+ new PageIdentityValue( 1, NS_MAIN, 'Foo', PageIdentity::LOCAL )
+ );
+ $slot = new SlotRecord(
+ (object)[
+ 'slot_id' => 1,
+ 'slot_revision_id' => null,
+ 'slot_content_id' => 1,
+ 'content_address' => null,
+ 'model_name' => 'x',
+ 'role_name' => 'main',
+ 'slot_origin' => null
+ ],
+ static function () {
+ throw new BadRevisionException( 'bad' );
+ }
+ );
+ $record->setSlot( $slot );
+
+ $exception = null;
+ try {
+ $record->getContentOrThrow( SlotRecord::MAIN );
+ } catch ( BadRevisionException $exception ) {
+ }
+ $this->assertNotNull( $exception );
+ $this->assertNull( $record->getContent( SlotRecord::MAIN ) );
+ }
}
diff --git a/tests/phpunit/unit/includes/Revision/RevisionRecordTests.php b/tests/phpunit/unit/includes/Revision/RevisionRecordTests.php
index 185e88b893f9..e39388d118cc 100644
--- a/tests/phpunit/unit/includes/Revision/RevisionRecordTests.php
+++ b/tests/phpunit/unit/includes/Revision/RevisionRecordTests.php
@@ -284,6 +284,38 @@ trait RevisionRecordTests {
);
}
+ /**
+ * @dataProvider provideGetSlot_audience
+ */
+ public function testGetContentOrThrow_audience( $visibility, $permissions, $userCan,
+ $publicCan
+ ) {
+ $performer = $this->mockRegisteredAuthorityWithPermissions( $permissions );
+ $rev = $this->newRevision( [ 'rev_deleted' => $visibility ] );
+
+ $exception = null;
+ try {
+ $rev->getContentOrThrow( SlotRecord::MAIN, RevisionRecord::RAW );
+ } catch ( SuppressedDataException $exception ) {
+ }
+ $this->assertNull( $exception, 'raw can' );
+
+ $exception = null;
+ try {
+ $rev->getContentOrThrow( SlotRecord::MAIN, RevisionRecord::FOR_PUBLIC );
+ } catch ( SuppressedDataException $exception ) {
+ }
+ $this->assertSame( $publicCan, $exception === null, 'public can' );
+
+ $exception = null;
+ try {
+ $rev->getContentOrThrow( SlotRecord::MAIN,
+ RevisionRecord::FOR_THIS_USER, $performer );
+ } catch ( SuppressedDataException $exception ) {
+ }
+ $this->assertSame( $userCan, $exception === null, 'user can' );
+ }
+
public function testGetSlot() {
$rev = $this->newRevision();