From c2172d45e4cd10a7b7d1474ce06f04592e00b913 Mon Sep 17 00:00:00 2001 From: daniel Date: Wed, 24 Oct 2012 15:00:13 +0200 Subject: [PATCH] Beware that getContent() may return null Before the introduction of the content handler, missing content was signified by getText() returning null instead of a string. null will work much like an empty string in most contexts, so in many places, it was not checked explcitely whether the conent was null. Now, when getContent() returns null, this often caused a fatal error, because the code would access whatever getContent() returned as an object, without checking whether it was null (because no such check was performed previously, when the content was represented as a string). This check introduces explicite checks for getContent() returning null in the most essential core classes. Change-Id: I551a90b0b67b8edc7570ca5d252ecc1de903f097 --- includes/Article.php | 14 +++--- includes/Revision.php | 7 +-- includes/Title.php | 2 +- includes/WikiPage.php | 44 +++++++++++-------- includes/diff/DifferenceEngine.php | 12 ++--- includes/job/jobs/DoubleRedirectJob.php | 2 +- includes/job/jobs/RefreshLinksJob.php | 17 ++++--- includes/parser/Parser.php | 2 +- .../ResourceLoaderWikiModule.php | 8 +++- includes/search/SearchEngine.php | 2 +- 10 files changed, 64 insertions(+), 46 deletions(-) diff --git a/includes/Article.php b/includes/Article.php index 0eb0c68b08..8403bc6c98 100644 --- a/includes/Article.php +++ b/includes/Article.php @@ -688,7 +688,7 @@ class Article extends Page { $outputDone = true; } else { $content = $this->getContentObject(); - $rt = $content->getRedirectChain(); + $rt = $content ? $content->getRedirectChain() : null; if ( $rt ) { wfDebug( __METHOD__ . ": showing redirect=no page\n" ); # Viewing a redirect page (e.g. with parameter redirect=no) @@ -842,10 +842,14 @@ class Article extends Page { 'clearyourcache' ); } - // Give hooks a chance to customise the output - if ( ContentHandler::runLegacyHooks( 'ShowRawCssJs', array( $this->fetchContentObject(), $this->getTitle(), $outputPage ) ) ) { - $po = $this->mContentObject->getParserOutput( $this->getTitle() ); - $outputPage->addHTML( $po->getText() ); + $this->fetchContentObject(); + + if ( $this->mContentObject ) { + // Give hooks a chance to customise the output + if ( ContentHandler::runLegacyHooks( 'ShowRawCssJs', array( $this->mContentObject, $this->getTitle(), $outputPage ) ) ) { + $po = $this->mContentObject->getParserOutput( $this->getTitle() ); + $outputPage->addHTML( $po->getText() ); + } } } diff --git a/includes/Revision.php b/includes/Revision.php index 3de8303621..df02b16db3 100644 --- a/includes/Revision.php +++ b/includes/Revision.php @@ -999,7 +999,8 @@ class Revision implements IDBAccessObject { $this->mText = $this->loadText(); } - $this->mContent = is_null( $this->mText ) ? null : $handler->unserializeContent( $this->mText, $format ); + $this->mContent = ( $this->mText === null || $this->mText === false ) ? null + : $handler->unserializeContent( $this->mText, $format ); } return $this->mContent->copy(); // NOTE: copy() will return $this for immutable content objects @@ -1377,7 +1378,7 @@ class Revision implements IDBAccessObject { $content = $this->getContent( Revision::RAW ); - if ( !$content->isValid() ) { + if ( !$content || !$content->isValid() ) { $t = $title->getPrefixedDBkey(); throw new MWException( "Content of $t is not valid! Content model is $model" ); @@ -1397,7 +1398,7 @@ class Revision implements IDBAccessObject { * Lazy-load the revision's text. * Currently hardcoded to the 'text' table storage engine. * - * @return String + * @return String|boolean the revision text, or false on failure */ protected function loadText() { wfProfileIn( __METHOD__ ); diff --git a/includes/Title.php b/includes/Title.php index c1e2ccf8a7..0f02dc7939 100644 --- a/includes/Title.php +++ b/includes/Title.php @@ -3981,7 +3981,7 @@ class Title { $content = $rev->getContent(); # Does the redirect point to the source? # Or is it a broken self-redirect, usually caused by namespace collisions? - $redirTitle = $content->getRedirectTarget(); + $redirTitle = $content ? $content->getRedirectTarget() : null; if ( $redirTitle ) { if ( $redirTitle->getPrefixedDBkey() != $this->getPrefixedDBkey() && diff --git a/includes/WikiPage.php b/includes/WikiPage.php index df3086a550..a4bc6ee824 100644 --- a/includes/WikiPage.php +++ b/includes/WikiPage.php @@ -1237,8 +1237,8 @@ class WikiPage extends Page implements IDBAccessObject { wfProfileIn( __METHOD__ ); $content = $revision->getContent(); - $len = $content->getSize(); - $rt = $content->getUltimateRedirectTarget(); + $len = $content ? $content->getSize() : 0; + $rt = $content ? $content->getUltimateRedirectTarget() : null; $conditions = array( 'page_id' => $this->getId() ); @@ -1469,11 +1469,6 @@ class WikiPage extends Page implements IDBAccessObject { // Bug 30711: always use current version when adding a new section if ( is_null( $edittime ) || $section == 'new' ) { $oldContent = $this->getContent(); - if ( ! $oldContent ) { - wfDebug( __METHOD__ . ": no page text\n" ); - wfProfileOut( __METHOD__ ); - return null; - } } else { $dbw = wfGetDB( DB_MASTER ); $rev = Revision::loadFromTimestamp( $dbw, $this->mTitle, $edittime ); @@ -1488,6 +1483,13 @@ class WikiPage extends Page implements IDBAccessObject { $oldContent = $rev->getContent(); } + if ( ! $oldContent ) { + wfDebug( __METHOD__ . ": no page text\n" ); + wfProfileOut( __METHOD__ ); + return null; + } + + //FIXME: $oldContent might be null? $newContent = $oldContent->replaceSection( $section, $sectionContent, $sectionTitle ); } @@ -1852,6 +1854,10 @@ class WikiPage extends Page implements IDBAccessObject { # Bug 37225: use accessor to get the text as Revision may trim it $content = $revision->getContent(); // sanity; get normalized version + if ( $content ) { + $newsize = $content->getSize(); + } + # Update the page record with revision data $this->updateRevisionOn( $dbw, $revision, 0 ); @@ -1864,7 +1870,7 @@ class WikiPage extends Page implements IDBAccessObject { $this->mTitle->getUserPermissionsErrors( 'autopatrol', $user ) ); # Add RC row to the DB $rc = RecentChange::notifyNew( $now, $this->mTitle, $isminor, $user, $summary, $bot, - '', $content->getSize(), $revisionId, $patrolled ); + '', $newsize, $revisionId, $patrolled ); # Log auto-patrolled edits if ( $patrolled ) { @@ -1956,7 +1962,7 @@ class WikiPage extends Page implements IDBAccessObject { * @since 1.21 */ public function prepareContentForEdit( Content $content, $revid = null, User $user = null, $serialization_format = null ) { - global $wgParser, $wgContLang, $wgUser; + global $wgContLang, $wgUser; $user = is_null( $user ) ? $wgUser : $user; //XXX: check $user->getId() here??? @@ -1977,23 +1983,21 @@ class WikiPage extends Page implements IDBAccessObject { $edit = (object)array(); $edit->revid = $revid; - $edit->pstContent = $content->preSaveTransform( $this->mTitle, $user, $popts ); - $edit->pst = $edit->pstContent->serialize( $serialization_format ); #XXX: do we need this?? - $edit->format = $serialization_format; + $edit->pstContent = $content ? $content->preSaveTransform( $this->mTitle, $user, $popts ) : null; + $edit->format = $serialization_format; $edit->popts = $this->makeParserOptions( 'canonical' ); - - $edit->output = $edit->pstContent->getParserOutput( $this->mTitle, $revid, $edit->popts ); + $edit->output = $edit->pstContent ? $edit->pstContent->getParserOutput( $this->mTitle, $revid, $edit->popts ) : null; $edit->newContent = $content; $edit->oldContent = $this->getContent( Revision::RAW ); #NOTE: B/C for hooks! don't use these fields! - $edit->newText = ContentHandler::getContentText( $edit->newContent ); + $edit->newText = $edit->newContent ? ContentHandler::getContentText( $edit->newContent ) : ''; $edit->oldText = $edit->oldContent ? ContentHandler::getContentText( $edit->oldContent ) : ''; + $edit->pst = $edit->pstContent ? $edit->pstContent->serialize( $serialization_format ) : ''; $this->mPreparedEdit = $edit; - return $edit; } @@ -2038,8 +2042,10 @@ class WikiPage extends Page implements IDBAccessObject { } # Update the links tables and other secondary data - $updates = $content->getSecondaryDataUpdates( $this->getTitle(), null, true, $editInfo->output ); - DataUpdate::runUpdates( $updates ); + if ( $content ) { + $updates = $content->getSecondaryDataUpdates( $this->getTitle(), null, true, $editInfo->output ); + DataUpdate::runUpdates( $updates ); + } wfRunHooks( 'ArticleEditUpdates', array( &$this, &$editInfo, $options['changed'] ) ); @@ -2111,7 +2117,7 @@ class WikiPage extends Page implements IDBAccessObject { if ( $this->mTitle->getNamespace() == NS_MEDIAWIKI ) { #XXX: could skip pseudo-messages like js/css here, based on content model. - $msgtext = $content->getWikitextForTransclusion(); + $msgtext = $content ? $content->getWikitextForTransclusion() : null; if ( $msgtext === false || $msgtext === null ) $msgtext = ''; MessageCache::singleton()->replace( $shortTitle, $msgtext ); diff --git a/includes/diff/DifferenceEngine.php b/includes/diff/DifferenceEngine.php index f3dc5a3a00..0295f776d1 100644 --- a/includes/diff/DifferenceEngine.php +++ b/includes/diff/DifferenceEngine.php @@ -522,8 +522,10 @@ class DifferenceEngine extends ContextSource { if ( ContentHandler::runLegacyHooks( 'ShowRawCssJs', array( $this->mNewContent, $this->mNewPage, $out ) ) ) { // NOTE: deprecated hook, B/C only // use the content object's own rendering - $po = $this->mNewRev->getContent()->getParserOutput( $this->mNewRev->getTitle(), $this->mNewRev->getId() ); - $out->addHTML( $po->getText() ); + $cnt = $this->mNewRev->getContent(); + $po = $cnt ? $cnt->getParserOutput( $this->mNewRev->getTitle(), $this->mNewRev->getId() ) : null; + $txt = $po ? $po->getText() : ''; + $out->addHTML( $txt ); } } elseif( !wfRunHooks( 'ArticleContentViewCustom', array( $this->mNewContent, $this->mNewPage, $out ) ) ) { // Handled by extension @@ -545,7 +547,7 @@ class DifferenceEngine extends ContextSource { $parserOutput = $this->getParserOutput( $wikiPage, $this->mNewRev ); # Also try to load it as a redirect - $rt = $this->mNewContent->getRedirectTarget(); + $rt = $this->mNewContent ? $this->mNewContent->getRedirectTarget() : null; if ( $rt ) { $article = Article::newFromTitle( $this->mNewPage, $this->getContext() ); @@ -1169,13 +1171,13 @@ class DifferenceEngine extends ContextSource { } if ( $this->mOldRev ) { $this->mOldContent = $this->mOldRev->getContent( Revision::FOR_THIS_USER, $this->getUser() ); - if ( $this->mOldContent === false ) { + if ( $this->mOldContent === null ) { return false; } } if ( $this->mNewRev ) { $this->mNewContent = $this->mNewRev->getContent( Revision::FOR_THIS_USER, $this->getUser() ); - if ( $this->mNewContent === false ) { + if ( $this->mNewContent === null ) { return false; } } diff --git a/includes/job/jobs/DoubleRedirectJob.php b/includes/job/jobs/DoubleRedirectJob.php index b1b96b62ab..ddd4fcca15 100644 --- a/includes/job/jobs/DoubleRedirectJob.php +++ b/includes/job/jobs/DoubleRedirectJob.php @@ -94,7 +94,7 @@ class DoubleRedirectJob extends Job { return true; } $content = $targetRev->getContent(); - $currentDest = $content->getRedirectTarget(); + $currentDest = $content ? $content->getRedirectTarget() : null; if ( !$currentDest || !$currentDest->equals( $this->redirTitle ) ) { wfDebug( __METHOD__.": Redirect has changed since the job was queued\n" ); return true; diff --git a/includes/job/jobs/RefreshLinksJob.php b/includes/job/jobs/RefreshLinksJob.php index a29f29fe64..384244fbad 100644 --- a/includes/job/jobs/RefreshLinksJob.php +++ b/includes/job/jobs/RefreshLinksJob.php @@ -70,18 +70,17 @@ class RefreshLinksJob extends Job { } public static function runForTitleInternal( Title $title, Revision $revision, $fname ) { - global $wgContLang; + wfProfileIn( $fname ); + $content = $revision->getContent( Revision::RAW ); - wfProfileIn( $fname . '-parse' ); - $options = ParserOptions::newFromUserAndLang( new User, $wgContLang ); - $content = $revision->getContent(); - $parserOutput = $content->getParserOutput( $title, $revision->getId(), $options, false ); - wfProfileOut( $fname . '-parse' ); + if ( !$content ) { + // if there is no content, pretend the content is empty + $content = $revision->getContentHandler()->makeEmptyContent(); + } - wfProfileIn( $fname . '-update' ); - $updates = $content->getSecondaryDataUpdates( $title, null, false, $parserOutput ); + $updates = $content->getSecondaryDataUpdates( $title, null, false ); DataUpdate::runUpdates( $updates ); - wfProfileOut( $fname . '-update' ); + wfProfileOut( $fname ); } } diff --git a/includes/parser/Parser.php b/includes/parser/Parser.php index b31288f58d..9dad1e5be9 100644 --- a/includes/parser/Parser.php +++ b/includes/parser/Parser.php @@ -3617,7 +3617,7 @@ class Parser { if ( $rev ) { $content = $rev->getContent(); - $text = $content->getWikitextForTransclusion(); + $text = $content ? $content->getWikitextForTransclusion() : null; if ( $text === false || $text === null ) { $text = false; diff --git a/includes/resourceloader/ResourceLoaderWikiModule.php b/includes/resourceloader/ResourceLoaderWikiModule.php index 28c3426c33..1e61a3e846 100644 --- a/includes/resourceloader/ResourceLoaderWikiModule.php +++ b/includes/resourceloader/ResourceLoaderWikiModule.php @@ -77,10 +77,16 @@ abstract class ResourceLoaderWikiModule extends ResourceLoaderModule { } $content = $revision->getContent( Revision::RAW ); + + if ( !$content ) { + wfDebug( __METHOD__ . "failed to load content of JS/CSS page!\n" ); + return null; + } + $model = $content->getModel(); if ( $model !== CONTENT_MODEL_CSS && $model !== CONTENT_MODEL_JAVASCRIPT ) { - wfDebug( __METHOD__ . "bad content model #$model for JS/CSS page!\n" ); + wfDebug( __METHOD__ . "bad content model $model for JS/CSS page!\n" ); return null; } diff --git a/includes/search/SearchEngine.php b/includes/search/SearchEngine.php index f8f5fa59f6..ec542a612d 100644 --- a/includes/search/SearchEngine.php +++ b/includes/search/SearchEngine.php @@ -795,7 +795,7 @@ class SearchResult { //TODO: if we could plug in some code that knows about special content models *and* about // special features of the search engine, the search could benefit. $content = $this->mRevision->getContent(); - $this->mText = $content->getTextForSearchIndex(); + $this->mText = $content ? $content->getTextForSearchIndex() : ''; } else { // TODO: can we fetch raw wikitext for commons images? $this->mText = ''; } -- 2.20.1