From 4835a75ec56444c61c5d0cfbc9e98ceb420fc513 Mon Sep 17 00:00:00 2001 From: daniel Date: Tue, 14 Aug 2018 18:37:30 +0200 Subject: [PATCH] Use RevisionRenderer for rendering ParserOutput Bug: T174035 Bug: T174036 Change-Id: I1085b05d635dd954c143c8a398fae909632ba0a9 --- docs/hooks.txt | 25 +- includes/content/ContentHandler.php | 10 +- includes/diff/DifferenceEngine.php | 29 +- includes/filerepo/file/LocalFile.php | 13 +- .../jobs/CategoryMembershipChangeJob.php | 6 +- includes/page/Article.php | 394 +++++++++++++----- includes/page/ImagePage.php | 12 +- includes/poolcounter/PoolWorkArticleView.php | 95 +++-- includes/specials/SpecialUndelete.php | 21 +- maintenance/compareParserCache.php | 10 +- .../phpunit/includes/page/ArticleViewTest.php | 43 ++ .../poolcounter/PoolWorkArticleViewTest.php | 171 ++++++++ 12 files changed, 664 insertions(+), 165 deletions(-) create mode 100644 tests/phpunit/includes/poolcounter/PoolWorkArticleViewTest.php diff --git a/docs/hooks.txt b/docs/hooks.txt index 436131cdd8..cce50e058c 100644 --- a/docs/hooks.txt +++ b/docs/hooks.txt @@ -624,8 +624,8 @@ a chance to hide their (unrelated) log entries. AND in the final query) $logTypes: Array of log types being queried -'ArticleAfterFetchContentObject': After fetching content of an article from the -database. +'ArticleAfterFetchContentObject': DEPRECATED since 1.32, use ArticleRevisionViewCustom +to control output. After fetching content of an article from the database. &$article: the article (object) being loaded from the database &$content: the content of the article, as a Content object @@ -640,12 +640,21 @@ this to change the content in this area or how it is loaded. $diffEngine: the DifferenceEngine $output: the OutputPage object -'ArticleContentViewCustom': Allows to output the text of the article in a -different format than wikitext. Note that it is preferable to implement proper -handing for a custom data type using the ContentHandler facility. +'ArticleRevisionViewCustom': Allows custom rendering of an article's content. +Note that it is preferable to implement proper handing for a custom data type using +the ContentHandler facility. +$revision: content of the page, as a RevisionRecord object, or null if the revision + could not be loaded. May also be a fake that wraps content supplied by an extension. +$title: title of the page +$oldid: the requested revision id, or 0 for the currrent revision. +$output: a ParserOutput object + +'ArticleContentViewCustom': DEPRECATED since 1.32, use ArticleRevisionViewCustom instead, +or provide an appropriate ContentHandler. Allows to output the text of the article in a +different format than wikitext. $content: content of the page, as a Content object $title: title of the page -$output: reference to $wgOut +$output: a ParserOutput object 'ArticleDelete': Before an article is deleted. &$wikiPage: the WikiPage (object) being deleted @@ -775,8 +784,8 @@ $article: the article $article: Article object $patrolFooterShown: boolean whether patrol footer is shown -'ArticleViewHeader': Before the parser cache is about to be tried for article -viewing. +'ArticleViewHeader': Control article output. Called before the parser cache is about +to be tried for article viewing. &$article: the article &$pcache: whether to try the parser cache or not &$outputDone: whether the output for this page finished or not. Set to diff --git a/includes/content/ContentHandler.php b/includes/content/ContentHandler.php index 344d0406bd..7a378b3bea 100644 --- a/includes/content/ContentHandler.php +++ b/includes/content/ContentHandler.php @@ -1389,14 +1389,20 @@ abstract class ContentHandler { * @return ParserOutput */ public function getParserOutputForIndexing( WikiPage $page, ParserCache $cache = null ) { + // TODO: MCR: ContentHandler should be called per slot, not for the whole page. + // See T190066. $parserOptions = $page->makeParserOptions( 'canonical' ); - $revId = $page->getRevision()->getId(); if ( $cache ) { $parserOutput = $cache->get( $page, $parserOptions ); } + if ( empty( $parserOutput ) ) { + $renderer = MediaWikiServices::getInstance()->getRevisionRenderer(); $parserOutput = - $page->getContent()->getParserOutput( $page->getTitle(), $revId, $parserOptions ); + $renderer->getRenderedRevision( + $page->getRevision()->getRevisionRecord(), + $parserOptions + )->getRevisionParserOutput(); if ( $cache ) { $cache->save( $parserOutput, $page, $parserOptions ); } diff --git a/includes/diff/DifferenceEngine.php b/includes/diff/DifferenceEngine.php index 1d69f12ec6..387e9e3c18 100644 --- a/includes/diff/DifferenceEngine.php +++ b/includes/diff/DifferenceEngine.php @@ -822,8 +822,20 @@ class DifferenceEngine extends ContextSource { /** * Show the new revision of the page. + * + * @note Not supported after calling setContent(). */ public function renderNewRevision() { + if ( $this->isContentOverridden ) { + // The code below only works with a Revision object. We could construct a fake revision + // (here or in setContent), but since this does not seem needed at the moment, + // we'll just fail for now. + throw new LogicException( + __METHOD__ + . ' is not supported after calling setContent(). Use setRevisions() instead.' + ); + } + $out = $this->getOutput(); $revHeader = $this->getRevisionHeader( $this->mNewRev ); # Add "current version as of X" title @@ -842,10 +854,16 @@ class DifferenceEngine extends ContextSource { $out->setRevisionTimestamp( $this->mNewRev->getTimestamp() ); $out->setArticleFlag( true ); - if ( !Hooks::run( 'ArticleContentViewCustom', - [ $this->mNewContent, $this->mNewPage, $out ] ) + if ( !Hooks::run( 'ArticleRevisionViewCustom', + [ $this->mNewRev->getRevisionRecord(), $this->mNewPage, $out ] ) ) { // Handled by extension + // NOTE: sync with hooks called in Article::view() + } elseif ( !Hooks::run( 'ArticleContentViewCustom', + [ $this->mNewContent, $this->mNewPage, $out ], '1.32' ) + ) { + // Handled by extension + // NOTE: sync with hooks called in Article::view() } else { // Normal page if ( $this->getTitle()->equals( $this->mNewPage ) ) { @@ -889,6 +907,13 @@ class DifferenceEngine extends ContextSource { * @return ParserOutput|bool False if the revision was not found */ protected function getParserOutput( WikiPage $page, Revision $rev ) { + if ( !$rev->getId() ) { + // WikiPage::getParserOutput wants a revision ID. Passing 0 will incorrectly show + // the current revision, so fail instead. If need be, WikiPage::getParserOutput + // could be made to accept a Revision or RevisionRecord instead of the id. + return false; + } + $parserOptions = $page->makeParserOptions( $this->getContext() ); $parserOutput = $page->getParserOutput( $parserOptions, $rev->getId() ); diff --git a/includes/filerepo/file/LocalFile.php b/includes/filerepo/file/LocalFile.php index 7920e9cb4b..fa6e180ee1 100644 --- a/includes/filerepo/file/LocalFile.php +++ b/includes/filerepo/file/LocalFile.php @@ -2116,16 +2116,21 @@ class LocalFile extends File { * @return string|false */ function getDescriptionText( Language $lang = null ) { - $revision = Revision::newFromTitle( $this->title, false, Revision::READ_NORMAL ); + $store = MediaWikiServices::getInstance()->getRevisionStore(); + $revision = $store->getRevisionByTitle( $this->title, 0, Revision::READ_NORMAL ); if ( !$revision ) { return false; } - $content = $revision->getContent(); - if ( !$content ) { + + $renderer = MediaWikiServices::getInstance()->getRevisionRenderer(); + $rendered = $renderer->getRenderedRevision( $revision, new ParserOptions( null, $lang ) ); + + if ( !$rendered ) { + // audience check failed return false; } - $pout = $content->getParserOutput( $this->title, null, new ParserOptions( null, $lang ) ); + $pout = $rendered->getRevisionParserOutput(); return $pout->getText(); } diff --git a/includes/jobqueue/jobs/CategoryMembershipChangeJob.php b/includes/jobqueue/jobs/CategoryMembershipChangeJob.php index a0c70abe71..c39823ff9b 100644 --- a/includes/jobqueue/jobs/CategoryMembershipChangeJob.php +++ b/includes/jobqueue/jobs/CategoryMembershipChangeJob.php @@ -232,12 +232,14 @@ class CategoryMembershipChangeJob extends Job { * @return string[] category names */ private function getCategoriesAtRev( WikiPage $page, Revision $rev, $parseTimestamp ) { - $content = $rev->getContent(); + $renderer = MediaWikiServices::getInstance()->getRevisionRenderer(); $options = $page->makeParserOptions( 'canonical' ); $options->setTimestamp( $parseTimestamp ); + // This could possibly use the parser cache if it checked the revision ID, // but that's more complicated than it's worth. - $output = $content->getParserOutput( $page->getTitle(), $rev->getId(), $options ); + $output = $renderer->getRenderedRevision( $rev->getRevisionRecord(), $options ) + ->getRevisionParserOutput(); // array keys will cast numeric category names to ints // so we need to cast them back to strings to avoid breaking things! diff --git a/includes/page/Article.php b/includes/page/Article.php index e90334fce6..464bb608ce 100644 --- a/includes/page/Article.php +++ b/includes/page/Article.php @@ -20,6 +20,8 @@ * @file */ use MediaWiki\MediaWikiServices; +use MediaWiki\Storage\MutableRevisionRecord; +use MediaWiki\Storage\RevisionRecord; /** * Class for viewing MediaWiki article and history. @@ -35,11 +37,11 @@ use MediaWiki\MediaWikiServices; class Article implements Page { /** * @var IContextSource|null The context this Article is executed in. - * If null, REquestContext::getMain() is used. + * If null, RequestContext::getMain() is used. */ protected $mContext; - /** @var WikiPage The WikiPage object of this instance */ + /** @var WikiPage|null The WikiPage object of this instance */ protected $mPage; /** @@ -49,22 +51,28 @@ class Article implements Page { public $mParserOptions; /** - * @var string|null Text of the revision we are working on - * @todo BC cruft - */ - public $mContent; - - /** - * @var Content|null Content of the revision we are working on. - * Initialized by fetchContentObject(). + * @var Content|null Content of the main slot of $this->mRevision. + * @note This variable is read only, setting it has no effect. + * Extensions that wish to override the output of Article::view should use a hook. + * @todo MCR: Remove in 1.33 + * @deprecated since 1.32 * @since 1.21 */ public $mContentObject; - /** @var bool Is the content ($mContent) already loaded? */ + /** + * @var bool Is the target revision loaded? Set by fetchRevisionRecord(). + * + * @deprecated since 1.32. Whether content has been loaded should not be relevant to + * code outside this class. + */ public $mContentLoaded = false; - /** @var int|null The oldid of the article that is to be shown, 0 for the current revision */ + /** + * @var int|null The oldid of the article that was requested to be shown, + * 0 for the current revision. + * @see $mRevIdFetched + */ public $mOldId; /** @var Title|null Title from which we were redirected here, if any. */ @@ -73,20 +81,38 @@ class Article implements Page { /** @var string|bool URL to redirect to or false if none */ public $mRedirectUrl = false; - /** @var int Revision ID of revision we are working on */ + /** + * @var int Revision ID of revision that was loaded. + * @see $mOldId + * @deprecated since 1.32, use getRevIdFetched() instead. + */ public $mRevIdFetched = 0; /** - * @var Revision|null Revision we are working on. Initialized by getOldIDFromRequest() - * or fetchContentObject(). + * @var Status|null represents the outcome of fetchRevisionRecord(). + * $fetchResult->value is the RevisionRecord object, if the operation was successful. + * + * The information in $fetchResult is duplicated by the following deprecated public fields: + * $mRevIdFetched, $mContentLoaded. $mRevision (and $mContentObject) also typically duplicate + * information of the loaded revision, but may be overwritten by extensions or due to errors. + */ + private $fetchResult = null; + + /** + * @var Revision|null Revision to be shown. Initialized by getOldIDFromRequest() + * or fetchContentObject(). Normally loaded from the database, but may be replaced + * by an extension, or be a fake representing an error message or some such. + * While the output of Article::view is typically based on this revision, + * it may be overwritten by error messages or replaced by extensions. */ public $mRevision = null; /** * @var ParserOutput|null|false The ParserOutput generated for viewing the page, * initialized by view(). If no ParserOutput could be generated, this is set to false. + * @deprecated since 1.32 */ - public $mParserOutput; + public $mParserOutput = null; /** * @var bool Whether render() was called. With the way subclasses work @@ -132,7 +158,7 @@ class Article implements Page { */ public static function newFromTitle( $title, IContextSource $context ) { if ( NS_MEDIA == $title->getNamespace() ) { - // FIXME: where should this go? + // XXX: This should not be here, but where should it go? $title = Title::makeTitle( NS_FILE, $title->getDBkey() ); } @@ -214,6 +240,11 @@ class Article implements Page { $this->mRedirectedFrom = null; # Title object if set $this->mRevIdFetched = 0; $this->mRedirectUrl = false; + $this->mRevision = null; + $this->mContentObject = null; + $this->fetchResult = null; + + // TODO hard-deprecate direct access to public fields $this->mPage->clear(); } @@ -229,25 +260,15 @@ class Article implements Page { * This function has side effects! Do not use this function if you * only want the real revision text if any. * - * @return Content Return the content of this revision + * @deprecated since 1.32, use getRevisionFetched() or fetchRevisionRecord() instead. + * + * @return Content * * @since 1.21 */ protected function getContentObject() { if ( $this->mPage->getId() === 0 ) { - # If this is a MediaWiki:x message, then load the messages - # and return the message value for x. - if ( $this->getTitle()->getNamespace() == NS_MEDIAWIKI ) { - $text = $this->getTitle()->getDefaultMessageText(); - if ( $text === false ) { - $text = ''; - } - - $content = ContentHandler::makeContent( $text, $this->getTitle() ); - } else { - $message = $this->getContext()->getUser()->isLoggedIn() ? 'noarticletext' : 'noarticletextanon'; - $content = new MessageContent( $message, null, 'parsemag' ); - } + $content = $this->getSubstituteContent(); } else { $this->fetchContentObject(); $content = $this->mContentObject; @@ -257,7 +278,49 @@ class Article implements Page { } /** - * @return int The oldid of the article that is to be shown, 0 for the current revision + * Returns Content object to use when the page does not exist. + * + * @return Content + */ + private function getSubstituteContent() { + # If this is a MediaWiki:x message, then load the messages + # and return the message value for x. + if ( $this->getTitle()->getNamespace() == NS_MEDIAWIKI ) { + $text = $this->getTitle()->getDefaultMessageText(); + if ( $text === false ) { + $text = ''; + } + + $content = ContentHandler::makeContent( $text, $this->getTitle() ); + } else { + $message = $this->getContext()->getUser()->isLoggedIn() ? 'noarticletext' : 'noarticletextanon'; + $content = new MessageContent( $message, null, 'parsemag' ); + } + + return $content; + } + + /** + * Returns ParserOutput to use when a page does not exist. In some cases, we still want to show + * "virtual" content, e.g. in the MediaWiki namespace, or in the File namespace for non-local + * files. + * + * @param ParserOptions $options + * + * @return ParserOutput + */ + protected function getEmptyPageParserOutput( ParserOptions $options ) { + $content = $this->getSubstituteContent(); + + return $content->getParserOutput( $this->getTitle(), 0, $options ); + } + + /** + * @see getOldIDFromRequest() + * @see getRevIdFetched() + * + * @return int The oldid of the article that is was requested in the constructor or via the + * context's WebRequest. */ public function getOldID() { if ( is_null( $this->mOldId ) ) { @@ -315,6 +378,8 @@ class Article implements Page { } } + $this->mRevIdFetched = $this->mRevision ? $this->mRevision->getId() : 0; + return $oldid; } @@ -322,6 +387,7 @@ class Article implements Page { * Get text content object * Does *NOT* follow redirects. * @todo When is this null? + * @deprecated since 1.32, use fetchRevisionRecord() instead. * * @note Code that wants to retrieve page content from the database should * use WikiPage::getContent(). @@ -331,74 +397,139 @@ class Article implements Page { * @since 1.21 */ protected function fetchContentObject() { - if ( $this->mContentLoaded ) { - return $this->mContentObject; + if ( !$this->mContentLoaded ) { + $this->fetchRevisionRecord(); + } + + return $this->mContentObject; + } + + /** + * Fetches the revision to work on. + * The revision is typically loaded from the database, but may also be a fake representing + * an error message or content supplied by an extension. Refer to $this->fetchResult for + * the revision actually loaded from the database, and any errors encountered while doing + * that. + * + * @return RevisionRecord|null + */ + protected function fetchRevisionRecord() { + if ( $this->fetchResult ) { + return $this->mRevision ? $this->mRevision->getRevisionRecord() : null; } $this->mContentLoaded = true; - $this->mContent = null; + $this->mContentObject = null; $oldid = $this->getOldID(); - # Pre-fill content with error message so that if something - # fails we'll have something telling us what we intended. - // XXX: this isn't page content but a UI message. horrible. - $this->mContentObject = new MessageContent( 'missing-revision', [ $oldid ] ); + // $this->mRevision might already be fetched by getOldIDFromRequest() + if ( !$this->mRevision ) { + if ( !$oldid ) { + $this->mRevision = $this->mPage->getRevision(); + + if ( !$this->mRevision ) { + wfDebug( __METHOD__ . " failed to find page data for title " . + $this->getTitle()->getPrefixedText() . "\n" ); - if ( $oldid ) { - # $this->mRevision might already be fetched by getOldIDFromRequest() - if ( !$this->mRevision ) { + // Just for sanity, output for this case is done by showMissingArticle(). + $this->fetchResult = Status::newFatal( 'noarticletext' ); + $this->applyContentOverride( $this->makeFetchErrorContent() ); + return null; + } + } else { $this->mRevision = Revision::newFromId( $oldid ); + if ( !$this->mRevision ) { - wfDebug( __METHOD__ . " failed to retrieve specified revision, id $oldid\n" ); - return false; + wfDebug( __METHOD__ . " failed to load revision, rev_id $oldid\n" ); + + $this->fetchResult = Status::newFatal( 'missing-revision', $oldid ); + $this->applyContentOverride( $this->makeFetchErrorContent() ); + return null; } } - } else { - $oldid = $this->mPage->getLatest(); - if ( !$oldid ) { - wfDebug( __METHOD__ . " failed to find page data for title " . - $this->getTitle()->getPrefixedText() . "\n" ); - return false; - } + } + + $this->mRevIdFetched = $this->mRevision->getId(); + $this->fetchResult = Status::newGood( $this->mRevision ); + + if ( !$this->mRevision->userCan( Revision::DELETED_TEXT, $this->getContext()->getUser() ) ) { + wfDebug( __METHOD__ . " failed to retrieve content of revision " . + $this->mRevision->getId() . "\n" ); + + // Just for sanity, output for this case is done by showDeletedRevisionHeader(). + $this->fetchResult = Status::newFatal( 'rev-deleted-text-permission' ); + $this->applyContentOverride( $this->makeFetchErrorContent() ); + return null; + } + + if ( Hooks::isRegistered( 'ArticleAfterFetchContentObject' ) ) { + $contentObject = $this->mRevision->getContent( + Revision::FOR_THIS_USER, + $this->getContext()->getUser() + ); - # Update error message with correct oldid - $this->mContentObject = new MessageContent( 'missing-revision', [ $oldid ] ); + $hookContentObject = $contentObject; - $this->mRevision = $this->mPage->getRevision(); + // Avoid PHP 7.1 warning of passing $this by reference + $articlePage = $this; + + Hooks::run( + 'ArticleAfterFetchContentObject', + [ &$articlePage, &$hookContentObject ], + '1.32' + ); - if ( !$this->mRevision ) { - wfDebug( __METHOD__ . " failed to retrieve current page, rev_id $oldid\n" ); - return false; + if ( $hookContentObject !== $contentObject ) { + // A hook handler is trying to override the content + $this->applyContentOverride( $hookContentObject ); } } - // @todo FIXME: Horrible, horrible! This content-loading interface just plain sucks. - // We should instead work with the Revision object when we need it... - // Loads if user is allowed - $content = $this->mRevision->getContent( + // For B/C only + $this->mContentObject = $this->mRevision->getContent( Revision::FOR_THIS_USER, $this->getContext()->getUser() ); - if ( !$content ) { - wfDebug( __METHOD__ . " failed to retrieve content of revision " . - $this->mRevision->getId() . "\n" ); - return false; + return $this->mRevision->getRevisionRecord(); + } + + /** + * Returns a Content object representing any error in $this->fetchContent, or null + * if there is no such error. + * + * @return Content|null + */ + private function makeFetchErrorContent() { + if ( !$this->fetchResult || $this->fetchResult->isOK() ) { + return null; } - $this->mContentObject = $content; - $this->mRevIdFetched = $this->mRevision->getId(); + return new MessageContent( $this->fetchResult->getMessage() ); + } - // Avoid PHP 7.1 warning of passing $this by reference - $articlePage = $this; + /** + * Applies a content override by constructing a fake Revision object and assigning + * it to mRevision. The fake revision will not have a user, timestamp or summary set. + * + * This mechanism exists mainly to accommodate extensions that use the + * ArticleAfterFetchContentObject. Once that hook has been removed, there should no longer + * be a need for a fake revision object. fetchRevisionRecord() presently also uses this mechanism + * to report errors, but that could be changed to use $this->fetchResult instead. + * + * @param Content $override Content to be used instead of the actual page content, + * coming from an extension or representing an error message. + */ + private function applyContentOverride( Content $override ) { + // Construct a fake revision + $rev = new MutableRevisionRecord( $this->getTitle() ); + $rev->setContent( 'main', $override ); - Hooks::run( - 'ArticleAfterFetchContentObject', - [ &$articlePage, &$this->mContentObject ] - ); + $this->mRevision = new Revision( $rev ); - return $this->mContentObject; + // For B/C only + $this->mContentObject = $override; } /** @@ -417,25 +548,32 @@ class Article implements Page { /** * Get the fetched Revision object depending on request parameters or null - * on failure. + * on failure. The revision returned may be a fake representing an error message or + * wrapping content supplied by an extension. Refer to $this->fetchResult for the + * revision actually loaded from the database. * * @since 1.19 * @return Revision|null */ public function getRevisionFetched() { - $this->fetchContentObject(); + $this->fetchRevisionRecord(); - return $this->mRevision; + if ( $this->fetchResult->isOK() ) { + return $this->mRevision; + } } /** * Use this to fetch the rev ID used on page views * + * Before fetchRevisionRecord was called, this returns the page's latest revision, + * regardless of what getOldID() returns. + * * @return int Revision ID of last article revision */ public function getRevIdFetched() { - if ( $this->mRevIdFetched ) { - return $this->mRevIdFetched; + if ( $this->fetchResult && $this->fetchResult->isOK() ) { + return $this->fetchResult->value->getId(); } else { return $this->mPage->getLatest(); } @@ -571,11 +709,9 @@ class Article implements Page { } break; case 3: - # This will set $this->mRevision if needed - $this->fetchContentObject(); - # Are we looking at an old revision - if ( $oldid && $this->mRevision ) { + $rev = $this->fetchRevisionRecord(); + if ( $oldid && $this->fetchResult->isOK() ) { $this->setOldSubtitle( $oldid ); if ( !$this->showDeletedRevisionHeader() ) { @@ -599,10 +735,21 @@ class Article implements Page { "
\n$1\n
", 'clearyourcache' ); + } elseif ( !Hooks::run( 'ArticleRevisionViewCustom', [ + $rev, + $this->getTitle(), + $oldid, + $outputPage, + ] ) + ) { + // NOTE: sync with hooks called in DifferenceEngine::renderNewRevision() + // Allow extensions do their own custom view for certain pages + $outputDone = true; } elseif ( !Hooks::run( 'ArticleContentViewCustom', - [ $this->fetchContentObject(), $this->getTitle(), $outputPage ] ) + [ $this->fetchContentObject(), $this->getTitle(), $outputPage ], '1.32' ) ) { - # Allow extensions do their own custom view for certain pages + // NOTE: sync with hooks called in DifferenceEngine::renderNewRevision() + // Allow extensions do their own custom view for certain pages $outputDone = true; } break; @@ -610,12 +757,32 @@ class Article implements Page { # Run the parse, protected by a pool counter wfDebug( __METHOD__ . ": doing uncached parse\n" ); - $content = $this->getContentObject(); - $poolArticleView = new PoolWorkArticleView( $this->getPage(), $parserOptions, - $this->getRevIdFetched(), $useParserCache, $content ); + $rev = $this->fetchRevisionRecord(); + $error = null; - if ( !$poolArticleView->execute() ) { + if ( $rev ) { + $poolArticleView = new PoolWorkArticleView( + $this->getPage(), + $parserOptions, + $this->getRevIdFetched(), + $useParserCache, + $rev + ); + $ok = $poolArticleView->execute(); $error = $poolArticleView->getError(); + $this->mParserOutput = $poolArticleView->getParserOutput() ?: null; + + # Don't cache a dirty ParserOutput object + if ( $poolArticleView->getIsDirty() ) { + $outputPage->setCdnMaxage( 0 ); + $outputPage->addHTML( "\n" ); + } + } else { + $ok = false; + } + + if ( !$ok ) { if ( $error ) { $outputPage->clearHTML(); // for release() errors $outputPage->enableClientCache( false ); @@ -628,18 +795,13 @@ class Article implements Page { return; } - $this->mParserOutput = $poolArticleView->getParserOutput(); - $outputPage->addParserOutput( $this->mParserOutput, $poOptions ); - if ( $content->getRedirectTarget() ) { - $outputPage->addSubtitle( "" . - $this->getContext()->msg( 'redirectpagesub' )->parse() . "" ); + if ( $this->mParserOutput ) { + $outputPage->addParserOutput( $this->mParserOutput, $poOptions ); } - # Don't cache a dirty ParserOutput object - if ( $poolArticleView->getIsDirty() ) { - $outputPage->setCdnMaxage( 0 ); - $outputPage->addHTML( "\n" ); + if ( $rev && $this->getRevisionRedirectTarget( $rev ) ) { + $outputPage->addSubtitle( "" . + $this->getContext()->msg( 'redirectpagesub' )->parse() . "" ); } $outputDone = true; @@ -650,8 +812,10 @@ class Article implements Page { } } - # Get the ParserOutput actually *displayed* here. - # Note that $this->mParserOutput is the *current*/oldid version output. + // Get the ParserOutput actually *displayed* here. + // Note that $this->mParserOutput is the *current*/oldid version output. + // Note that the ArticleViewHeader hook is allowed to set $outputDone to a + // ParserOutput instance. $pOutput = ( $outputDone instanceof ParserOutput ) ? $outputDone // object fetched by hook : $this->mParserOutput ?: null; // ParserOutput or null, avoid false @@ -677,12 +841,12 @@ class Article implements Page { $outputPage->adaptCdnTTL( $this->mPage->getTimestamp(), IExpiringStore::TTL_DAY ); # Check for any __NOINDEX__ tags on the page using $pOutput - $policy = $this->getRobotPolicy( 'view', $pOutput ); + $policy = $this->getRobotPolicy( 'view', $pOutput ?: null ); $outputPage->setIndexPolicy( $policy['index'] ); - $outputPage->setFollowPolicy( $policy['follow'] ); + $outputPage->setFollowPolicy( $policy['follow'] ); // FIXME: test this $this->showViewFooter(); - $this->mPage->doViewUpdates( $user, $oldid ); + $this->mPage->doViewUpdates( $user, $oldid ); // FIXME: test this # Load the postEdit module if the user just saved this revision # See also EditPage::setPostEditCookie @@ -693,10 +857,22 @@ class Article implements Page { # Clear the cookie. This also prevents caching of the response. $request->response()->clearCookie( $cookieKey ); $outputPage->addJsConfigVars( 'wgPostEdit', $postEdit ); - $outputPage->addModules( 'mediawiki.action.view.postEdit' ); + $outputPage->addModules( 'mediawiki.action.view.postEdit' ); // FIXME: test this } } + /** + * @param RevisionRecord $revision + * @return null|Title + */ + private function getRevisionRedirectTarget( RevisionRecord $revision ) { + // TODO: find a *good* place for the code that determines the redirect target for + // a given revision! + // NOTE: Use main slot content. Compare code in DerivedPageDataUpdater::revisionIsRedirect. + $content = $revision->getContent( 'main' ); + return $content ? $content->getRedirectTarget() : null; + } + /** * Adjust title for pages with displaytitle, -{T|}- or language conversion * @param ParserOutput $pOutput @@ -1261,7 +1437,9 @@ class Article implements Page { # Show error message $oldid = $this->getOldID(); if ( !$oldid && $title->getNamespace() === NS_MEDIAWIKI && $title->hasSourceText() ) { - $outputPage->addParserOutput( $this->getContentObject()->getParserOutput( $title ) ); + // use fake Content object for system message + $parserOptions = ParserOptions::newCanonical( 'canonical' ); + $outputPage->addParserOutput( $this->getEmptyPageParserOutput( $parserOptions ) ); } else { if ( $oldid ) { $text = wfMessage( 'missing-revision', $oldid )->plain(); @@ -1670,7 +1848,7 @@ class Article implements Page { __METHOD__ ); - // @todo FIXME: i18n issue/patchwork message + // @todo i18n issue/patchwork message $context->getOutput()->addHTML( '' . $context->msg( 'historywarning' )->numParams( $revisions )->parse() . @@ -1697,7 +1875,7 @@ class Article implements Page { /** * Output deletion confirmation dialog - * @todo FIXME: Move to another file? + * @todo Move to another file? * @param string $reason Prefilled reason */ public function confirmDelete( $reason ) { diff --git a/includes/page/ImagePage.php b/includes/page/ImagePage.php index 58f25d4650..d3d6da7935 100644 --- a/includes/page/ImagePage.php +++ b/includes/page/ImagePage.php @@ -272,18 +272,20 @@ class ImagePage extends Article { } /** - * Overloading Article's getContentObject method. + * Overloading Article's getEmptyPageParserOutput method. * * Omit noarticletext if sharedupload; text will be fetched from the * shared upload server if possible. - * @return string + * + * @param ParserOptions $options + * @return ParserOutput */ - public function getContentObject() { + public function getEmptyPageParserOutput( ParserOptions $options ) { $this->loadFile(); if ( $this->mPage->getFile() && !$this->mPage->getFile()->isLocal() && 0 == $this->getId() ) { - return null; + return new ParserOutput(); } - return parent::getContentObject(); + return parent::getEmptyPageParserOutput( $options ); } private function getLanguageForRendering( WebRequest $request, File $file ) { diff --git a/includes/poolcounter/PoolWorkArticleView.php b/includes/poolcounter/PoolWorkArticleView.php index 4af86ae03d..286494efa4 100644 --- a/includes/poolcounter/PoolWorkArticleView.php +++ b/includes/poolcounter/PoolWorkArticleView.php @@ -17,7 +17,12 @@ * * @file */ + use MediaWiki\MediaWikiServices; +use MediaWiki\Revision\RevisionRenderer; +use MediaWiki\Storage\MutableRevisionRecord; +use MediaWiki\Storage\RevisionRecord; +use MediaWiki\Storage\RevisionStore; class PoolWorkArticleView extends PoolCounterWork { /** @var WikiPage */ @@ -35,8 +40,14 @@ class PoolWorkArticleView extends PoolCounterWork { /** @var ParserOptions */ private $parserOptions; - /** @var Content|null */ - private $content = null; + /** @var RevisionRecord|null */ + private $revision = null; + + /** @var RevisionStore */ + private $revisionStore = null; + + /** @var RevisionRenderer */ + private $renderer = null; /** @var ParserOutput|bool */ private $parserOutput = false; @@ -53,26 +64,52 @@ class PoolWorkArticleView extends PoolCounterWork { * @param int $revid ID of the revision being parsed. * @param bool $useParserCache Whether to use the parser cache. * operation. - * @param Content|string|null $content Content to parse or null to load it; may - * also be given as a wikitext string, for BC. + * @param RevisionRecord|Content|string|null $revision Revision to render, or null to load it; + * may also be given as a wikitext string, or a Content object, for BC. */ public function __construct( WikiPage $page, ParserOptions $parserOptions, - $revid, $useParserCache, $content = null + $revid, $useParserCache, $revision = null ) { - if ( is_string( $content ) ) { // BC: old style call + if ( is_string( $revision ) ) { // BC: very old style call $modelId = $page->getRevision()->getContentModel(); $format = $page->getRevision()->getContentFormat(); - $content = ContentHandler::makeContent( $content, $page->getTitle(), $modelId, $format ); + $revision = ContentHandler::makeContent( $revision, $page->getTitle(), $modelId, $format ); + } + + if ( $revision instanceof Content ) { // BC: old style call + $content = $revision; + $revision = new MutableRevisionRecord( $page->getTitle() ); + $revision->setId( $revid ); + $revision->setPageId( $page->getId() ); + $revision->setContent( 'main', $content ); } + if ( $revision ) { + // Check that the RevisionRecord matches $revid and $page, but still allow + // fake RevisionRecords coming from errors or hooks in Article to be rendered. + if ( $revision->getId() && $revision->getId() !== $revid ) { + throw new InvalidArgumentException( '$revid parameter mismatches $revision parameter' ); + } + if ( $revision->getPageId() + && $revision->getPageId() !== $page->getTitle()->getArticleID() + ) { + throw new InvalidArgumentException( '$page parameter mismatches $revision parameter' ); + } + } + + // TODO: DI: inject services + $this->renderer = MediaWikiServices::getInstance()->getRevisionRenderer(); + $this->revisionStore = MediaWikiServices::getInstance()->getRevisionStore(); + $this->parserCache = MediaWikiServices::getInstance()->getParserCache(); + $this->page = $page; $this->revid = $revid; $this->cacheable = $useParserCache; $this->parserOptions = $parserOptions; - $this->content = $content; - $this->parserCache = MediaWikiServices::getInstance()->getParserCache(); + $this->revision = $revision; $this->cacheKey = $this->parserCache->getKey( $page, $parserOptions ); $keyPrefix = $this->cacheKey ?: wfMemcKey( 'articleview', 'missingcachekey' ); + parent::__construct( 'ArticleView', $keyPrefix . ':revid:' . $revid ); } @@ -114,23 +151,33 @@ class PoolWorkArticleView extends PoolCounterWork { $isCurrent = $this->revid === $this->page->getLatest(); - if ( $this->content !== null ) { - $content = $this->content; + // Bypass audience check for current revision + $audience = $isCurrent ? RevisionRecord::RAW : RevisionRecord::FOR_PUBLIC; + + if ( $this->revision !== null ) { + $rev = $this->revision; } elseif ( $isCurrent ) { - // XXX: why use RAW audience here, and PUBLIC (default) below? - $content = $this->page->getContent( Revision::RAW ); + $rev = $this->page->getRevision() + ? $this->page->getRevision()->getRevisionRecord() + : null; } else { - $rev = Revision::newFromTitle( $this->page->getTitle(), $this->revid ); + $rev = $this->revisionStore->getRevisionByTitle( $this->page->getTitle(), $this->revid ); + } - if ( $rev === null ) { - $content = null; - } else { - // XXX: why use PUBLIC audience here (default), and RAW above? - $content = $rev->getContent(); - } + if ( !$rev ) { + // couldn't load + return false; } - if ( $content === null ) { + $renderedRevision = $this->renderer->getRenderedRevision( + $rev, + $this->parserOptions, + null, + [ 'audience' => $audience ] + ); + + if ( !$renderedRevision ) { + // audience check failed return false; } @@ -138,11 +185,7 @@ class PoolWorkArticleView extends PoolCounterWork { $cacheTime = wfTimestampNow(); $time = - microtime( true ); - $this->parserOutput = $content->getParserOutput( - $this->page->getTitle(), - $this->revid, - $this->parserOptions - ); + $this->parserOutput = $renderedRevision->getRevisionParserOutput(); $time += microtime( true ); // Timing hack diff --git a/includes/specials/SpecialUndelete.php b/includes/specials/SpecialUndelete.php index 3069bd8654..a92982040b 100644 --- a/includes/specials/SpecialUndelete.php +++ b/includes/specials/SpecialUndelete.php @@ -21,6 +21,8 @@ * @ingroup SpecialPage */ +use MediaWiki\MediaWikiServices; +use MediaWiki\Storage\RevisionRecord; use Wikimedia\Rdbms\IResultWrapper; /** @@ -421,8 +423,9 @@ class SpecialUndelete extends SpecialPage { $t = $lang->userTime( $timestamp, $user ); $userLink = Linker::revUserTools( $rev ); - $content = $rev->getContent( Revision::FOR_THIS_USER, $user ); + $content = $rev->getContent( RevisionRecord::FOR_THIS_USER, $user ); + // TODO: MCR: this will have to become something like $hasTextSlots and $hasNonTextSlots $isText = ( $content instanceof TextContent ); if ( $this->mPreview || $isText ) { @@ -447,12 +450,23 @@ class SpecialUndelete extends SpecialPage { return; } - if ( ( $this->mPreview || !$isText ) && $content ) { + if ( $this->mPreview || !$isText ) { // NOTE: non-text content has no source view, so always use rendered preview $popts = $out->parserOptions(); + $renderer = MediaWikiServices::getInstance()->getRevisionRenderer(); + + $rendered = $renderer->getRenderedRevision( + $rev->getRevisionRecord(), + $popts, + $user, + [ 'audience' => RevisionRecord::FOR_THIS_USER ] + ); + + // Fail hard if the audience check fails, since we already checked + // at the beginning of this method. + $pout = $rendered->getRevisionParserOutput(); - $pout = $content->getParserOutput( $this->mTargetObj, $rev->getId(), $popts, true ); $out->addParserOutput( $pout, [ 'enableSectionEditLinks' => false, ] ); @@ -462,6 +476,7 @@ class SpecialUndelete extends SpecialPage { $buttonFields = []; if ( $isText ) { + // TODO: MCR: make this work for multiple slots // source view for textual content $sourceView = Xml::element( 'textarea', [ 'readonly' => 'readonly', diff --git a/maintenance/compareParserCache.php b/maintenance/compareParserCache.php index b12974b06e..2cafc1b31a 100644 --- a/maintenance/compareParserCache.php +++ b/maintenance/compareParserCache.php @@ -44,6 +44,7 @@ class CompareParserCache extends Maintenance { $withcache = 0; $withdiff = 0; $parserCache = MediaWikiServices::getInstance()->getParserCache(); + $renderer = MediaWikiServices::getInstance()->getRevisionRenderer(); while ( $pages-- > 0 ) { $row = $dbr->selectRow( 'page', // @todo Title::selectFields() or Title::getQueryInfo() or something @@ -69,17 +70,16 @@ class CompareParserCache extends Maintenance { $title = Title::newFromRow( $row ); $page = WikiPage::factory( $title ); - $revision = $page->getRevision(); - $content = $revision->getContent( Revision::RAW ); - + $revision = $page->getRevision()->getRevisionRecord(); $parserOptions = $page->makeParserOptions( 'canonical' ); $parserOutputOld = $parserCache->get( $page, $parserOptions ); if ( $parserOutputOld ) { $t1 = microtime( true ); - $parserOutputNew = $content->getParserOutput( - $title, $revision->getId(), $parserOptions, false ); + $parserOutputNew = $renderer->getRenderedRevision( $revision, $parserOptions ) + ->getRevisionParserOutput(); + $sec = microtime( true ) - $t1; $totalsec += $sec; diff --git a/tests/phpunit/includes/page/ArticleViewTest.php b/tests/phpunit/includes/page/ArticleViewTest.php index d7212746b8..68cddd6cc9 100644 --- a/tests/phpunit/includes/page/ArticleViewTest.php +++ b/tests/phpunit/includes/page/ArticleViewTest.php @@ -425,6 +425,44 @@ class ArticleViewTest extends MediaWikiTestCase { } ); + $this->hideDeprecated( + 'ArticleContentViewCustom hook (used in hook-ArticleContentViewCustom-closure)' + ); + + $article->view(); + + $output = $article->getContext()->getOutput(); + $this->assertNotContains( 'Test A', $this->getHtml( $output ) ); + $this->assertContains( 'Hook Text', $this->getHtml( $output ) ); + } + + public function testArticleRevisionViewCustomHook() { + $page = $this->getPage( __METHOD__, [ 1 => 'Test A' ] ); + + $article = new Article( $page->getTitle(), 0 ); + $article->getContext()->getOutput()->setTitle( $page->getTitle() ); + + // use ArticleViewHeader hook to bypass the parser cache + $this->setTemporaryHook( + 'ArticleViewHeader', + function ( Article $articlePage, &$outputDone, &$useParserCache ) use ( $article ) { + $useParserCache = false; + } + ); + + $this->setTemporaryHook( + 'ArticleRevisionViewCustom', + function ( RevisionRecord $rev, Title $title, $oldid, OutputPage $output ) use ( $page ) { + $content = $rev->getContent( 'main' ); + + $this->assertSame( $page->getTitle(), $title, '$title' ); + $this->assertSame( 'Test A', $content->getNativeData(), '$content' ); + + $output->addHTML( 'Hook Text' ); + return false; + } + ); + $article->view(); $output = $article->getContext()->getOutput(); @@ -456,6 +494,11 @@ class ArticleViewTest extends MediaWikiTestCase { } ); + $this->hideDeprecated( + 'ArticleAfterFetchContentObject hook' + . ' (used in hook-ArticleAfterFetchContentObject-closure)' + ); + $article->view(); $output = $article->getContext()->getOutput(); diff --git a/tests/phpunit/includes/poolcounter/PoolWorkArticleViewTest.php b/tests/phpunit/includes/poolcounter/PoolWorkArticleViewTest.php new file mode 100644 index 0000000000..61eb316c55 --- /dev/null +++ b/tests/phpunit/includes/poolcounter/PoolWorkArticleViewTest.php @@ -0,0 +1,171 @@ +getTestUser()->getUser(); + $updater = $page->newPageUpdater( $user ); + + $updater->setContent( 'main', new WikitextContent( $text ) ); + return $updater->saveRevision( CommentStoreComment::newUnsavedComment( 'testing' ) ); + } + + public function testDoWorkLoadRevision() { + $options = ParserOptions::newCanonical( 'canonical' ); + $page = $this->getExistingTestPage( __METHOD__ ); + $rev1 = $this->makeRevision( $page, 'First!' ); + $rev2 = $this->makeRevision( $page, 'Second!' ); + + $work = new PoolWorkArticleView( $page, $options, $rev1->getId(), false ); + $work->execute(); + $this->assertContains( 'First', $work->getParserOutput()->getText() ); + + $work = new PoolWorkArticleView( $page, $options, $rev2->getId(), false ); + $work->execute(); + $this->assertContains( 'Second', $work->getParserOutput()->getText() ); + } + + public function testDoWorkParserCache() { + $options = ParserOptions::newCanonical( 'canonical' ); + $page = $this->getExistingTestPage( __METHOD__ ); + $rev1 = $this->makeRevision( $page, 'First!' ); + + $work = new PoolWorkArticleView( $page, $options, $rev1->getId(), true ); + $work->execute(); + + $cache = MediaWikiServices::getInstance()->getParserCache(); + $out = $cache->get( $page, $options ); + + $this->assertNotNull( $out ); + $this->assertNotFalse( $out ); + $this->assertContains( 'First', $out->getText() ); + } + + public function testDoWorkWithExplicitRevision() { + $options = ParserOptions::newCanonical( 'canonical' ); + $page = $this->getExistingTestPage( __METHOD__ ); + $rev = $this->makeRevision( $page, 'NOPE' ); + + // make a fake revision with different content, so we know it's actually being used! + $fakeRev = new MutableRevisionRecord( $page->getTitle() ); + $fakeRev->setId( $rev->getId() ); + $fakeRev->setPageId( $page->getId() ); + $fakeRev->setContent( 'main', new WikitextContent( 'YES!' ) ); + + $work = new PoolWorkArticleView( $page, $options, $rev->getId(), false, $fakeRev ); + $work->execute(); + + $text = $work->getParserOutput()->getText(); + $this->assertContains( 'YES!', $text ); + $this->assertNotContains( 'NOPE', $text ); + } + + public function testDoWorkWithContent() { + $options = ParserOptions::newCanonical( 'canonical' ); + $page = $this->getExistingTestPage( __METHOD__ ); + + $content = new WikitextContent( 'YES!' ); + + $work = new PoolWorkArticleView( $page, $options, $page->getLatest(), false, $content ); + $work->execute(); + + $text = $work->getParserOutput()->getText(); + $this->assertContains( 'YES!', $text ); + } + + public function testDoWorkWithString() { + $options = ParserOptions::newCanonical( 'canonical' ); + $page = $this->getExistingTestPage( __METHOD__ ); + + $work = new PoolWorkArticleView( $page, $options, $page->getLatest(), false, 'YES!' ); + $work->execute(); + + $text = $work->getParserOutput()->getText(); + $this->assertContains( 'YES!', $text ); + } + + public function provideMagicWords() { + yield 'PAGEID' => [ + 'Test {{PAGEID}} Test', + function ( RevisionRecord $rev ) { + return $rev->getPageId(); + } + ]; + yield 'REVISIONID' => [ + 'Test {{REVISIONID}} Test', + function ( RevisionRecord $rev ) { + return $rev->getId(); + } + ]; + yield 'REVISIONUSER' => [ + 'Test {{REVISIONUSER}} Test', + function ( RevisionRecord $rev ) { + return $rev->getUser()->getName(); + } + ]; + yield 'REVISIONTIMESTAMP' => [ + 'Test {{REVISIONTIMESTAMP}} Test', + function ( RevisionRecord $rev ) { + return $rev->getTimestamp(); + } + ]; + } + /** + * @dataProvider provideMagicWords + */ + public function testMagicWords( $wikitext, $callback ) { + $options = ParserOptions::newCanonical( 'canonical' ); + $page = $this->getExistingTestPage( __METHOD__ ); + $rev = $page->getRevision()->getRevisionRecord(); + + // NOTE: provide the input as a string and let the PoolWorkArticleView create a fake + // revision internally, to see if the magic words work with that fake. They should + // work if the Parser causes the actual revision to be loaded when needed. + $work = new PoolWorkArticleView( $page, $options, $page->getLatest(), false, $wikitext ); + $work->execute(); + + $expected = strval( $callback( $rev ) ); + $output = $work->getParserOutput(); + + $this->assertContains( $expected, $output->getText() ); + } + + public function testDoWorkMissingPage() { + $options = ParserOptions::newCanonical( 'canonical' ); + $page = $this->getNonexistingTestPage(); + + $work = new PoolWorkArticleView( $page, $options, '667788', false ); + $this->assertFalse( $work->execute() ); + } + + public function testDoWorkDeletedContent() { + $options = ParserOptions::newCanonical( 'canonical' ); + $page = $this->getExistingTestPage( __METHOD__ ); + $rev1 = $page->getRevision()->getRevisionRecord(); + + // make another revision, since the latest revision cannot be deleted. + $rev2 = $this->makeRevision( $page, 'Next' ); + + // make a fake revision with deleted different content + $fakeRev = new MutableRevisionRecord( $page->getTitle() ); + $fakeRev->setId( $rev1->getId() ); + $fakeRev->setPageId( $page->getId() ); + $fakeRev->setContent( 'main', new WikitextContent( 'SECRET' ) ); + $fakeRev->setVisibility( RevisionRecord::DELETED_TEXT ); + + $work = new PoolWorkArticleView( $page, $options, $rev1->getId(), false, $fakeRev ); + $this->assertFalse( $work->execute() ); + + // a deleted current revision should still be show + $fakeRev->setId( $rev2->getId() ); + $work = new PoolWorkArticleView( $page, $options, $rev2->getId(), false, $fakeRev ); + $this->assertNotFalse( $work->execute() ); + } + +} -- 2.20.1