From 48d0bedd785157508a884e5640c198fc0162f77a Mon Sep 17 00:00:00 2001 From: daniel Date: Thu, 7 Jun 2012 14:57:43 +0200 Subject: [PATCH] cleanup and fixes for secondary data updates --- includes/Content.php | 20 +--- includes/ContentHandler.php | 20 +++- includes/LinksUpdate.php | 5 +- includes/Revision.php | 4 + includes/WikiPage.php | 3 +- includes/api/ApiPurge.php | 2 +- includes/job/RefreshLinksJob.php | 8 +- includes/parser/ParserOutput.php | 3 + maintenance/refreshLinks.php | 5 +- tests/phpunit/includes/ContentHandlerTest.php | 105 ++++++++++++++++++ 10 files changed, 142 insertions(+), 33 deletions(-) diff --git a/includes/Content.php b/includes/Content.php index 6c23b09c44..490bb8c8ce 100644 --- a/includes/Content.php +++ b/includes/Content.php @@ -287,6 +287,9 @@ abstract class Content { * Convenience method, shorthand for * $this->getContentHandler()->getParserOutput( $this, $title, $revId, $options, $generateHtml ) * + * @note: subclasses should NOT override this to provide custom rendering. + * Override ContentHandler::getParserOutput() instead! + * * @param Title $title * @param null $revId * @param null|ParserOptions $options @@ -302,23 +305,6 @@ abstract class Content { return $this->getContentHandler()->getParserOutput( $this, $title, $revId, $options, $generateHtml ); } - /** - * Convenience method, shorthand for - * $this->getContentHandler()->getSecondaryDataUpdates( $this, $title, $old, $recursive ) - * - * @param Title $title the context for determining the necessary updates - * @param Content|null $old a Content object representing the previous content, i.e. the content being - * replaced by this Content object. - * @param bool $recursive whether to include recursive updates (default: false). - * - * @return Array. A list of DataUpdate objects for putting information about this content object somewhere. - * - * @since WD.1 - */ - public function getSecondaryDataUpdates( Title $title, Content $old = null, $recursive = false ) { #TODO: remove! - return $this->getContentHandler()->getSecondaryDataUpdates( $this, $title, $old, $recursive ); - } - /** * Construct the redirect destination from this content and return an * array of Titles, or null if this content doesn't represent a redirect. diff --git a/includes/ContentHandler.php b/includes/ContentHandler.php index e7970489cc..1495eb196b 100644 --- a/includes/ContentHandler.php +++ b/includes/ContentHandler.php @@ -734,24 +734,32 @@ abstract class ContentHandler { * data store. If the optional second argument, $old, is given, the updates may model only the changes that * need to be made to replace information about the old content with information about the new content. * - * This default implementation calls $this->getParserOutput( $title, null, null, false ), and then + * This default implementation calls $this->getParserOutput( $content, $title, null, null, false ), and then * calls getSecondaryDataUpdates( $title, $recursive ) on the resulting ParserOutput object. * * Subclasses may implement this to determine the necessary updates more efficiently, or make use of information * about the old content. * + * @param Content $content the content for determining the necessary updates * @param Title $title the context for determining the necessary updates - * @param Content|null $old a Content object representing the previous content, i.e. the content being + * @param Content|null $old an optional Content object representing the previous content, i.e. the content being * replaced by this Content object. - * @param bool $recursive whether to include recursive updates (default: false). + * @param boolean $recursive whether to include recursive updates (default: false). + * @param ParserOutput|null $parserOutput optional ParserOutput object. Provide if you have one handy, to avoid re-parsing + * of the content. * * @return Array. A list of DataUpdate objects for putting information about this content object somewhere. * * @since WD.1 */ - public function getSecondaryDataUpdates( Content $content, Title $title, Content $old = null, $recursive = false ) { - $po = $this->getParserOutput( $content, $title, null, null, false ); - return $po->getSecondaryDataUpdates( $title, $recursive ); + public function getSecondaryDataUpdates( Content $content, Title $title, Content $old = null, + $recursive = true, ParserOutput $parserOutput = null ) { + + if ( !$parserOutput ) { + $parserOutput = $this->getParserOutput( $content, $title, null, null, false ); + } + + return $parserOutput->getSecondaryDataUpdates( $title, $recursive ); } diff --git a/includes/LinksUpdate.php b/includes/LinksUpdate.php index d6db70f91f..3e8e362c7a 100644 --- a/includes/LinksUpdate.php +++ b/includes/LinksUpdate.php @@ -63,7 +63,6 @@ class LinksUpdate extends SqlDataUpdate { $this->mTitle = $title; $this->mId = $title->getArticleID(); - assert( $this->mId > 0 ); if ( !$this->mId ) { throw new MWException( "The Title object did not provide an article ID. Perhaps the page doesn't exist?" ); @@ -830,6 +829,10 @@ class LinksDeletionUpdate extends SqlDataUpdate { parent::__construct( ); $this->mTitle = $title; + + if ( !$title->getArticleID() ) { + throw new MWException( "The Title object did not provide an article ID. Perhaps the page doesn't exist?" ); + } } /** diff --git a/includes/Revision.php b/includes/Revision.php index 50aba9f97e..48c34e9088 100644 --- a/includes/Revision.php +++ b/includes/Revision.php @@ -1204,6 +1204,10 @@ class Revision { if ( $wgContentHandlerUseDB ) { $row[ 'rev_content_model' ] = $this->getContentModel(); $row[ 'rev_content_format' ] = $this->getContentFormat(); + } else { + // @todo: Make sure the revision is using the default model and format. + // Since we don't store the actual model and format, we won't have any way to determine it later + // if it's not the default. } $dbw->insert( 'revision', $row, __METHOD__ ); diff --git a/includes/WikiPage.php b/includes/WikiPage.php index fdb079280d..1612cc7600 100644 --- a/includes/WikiPage.php +++ b/includes/WikiPage.php @@ -1983,7 +1983,8 @@ class WikiPage extends Page { } # Update the links tables and other secondary data - $updates = $editInfo->output->getSecondaryDataUpdates( $this->getTitle() ); #FIXME: call ContentHandler::getSecondaryLinkUpdates. Don't parse iuf not needed! But don't parse too early either, only after saving, so we have an article ID! + $contentHandler = $revision->getContentHandler(); + $updates = $contentHandler->getSecondaryDataUpdates( $content, $this->getTitle(), null, true, $editInfo->output ); DataUpdate::runUpdates( $updates ); wfRunHooks( 'ArticleEditUpdates', array( &$this, &$editInfo, $options['changed'] ) ); diff --git a/includes/api/ApiPurge.php b/includes/api/ApiPurge.php index 5ca3fa8d51..66aa3b0ed2 100644 --- a/includes/api/ApiPurge.php +++ b/includes/api/ApiPurge.php @@ -94,7 +94,7 @@ class ApiPurge extends ApiBase { true, true, $page->getLatest() ); #FIXME: content! # Update the links tables - $updates = $p_result->getSecondaryDataUpdates( $title ); + $updates = $p_result->getSecondaryDataUpdates( $title ); #FIXME: content handler DataUpdate::runUpdates( $updates ); $r['linkupdate'] = ''; diff --git a/includes/job/RefreshLinksJob.php b/includes/job/RefreshLinksJob.php index 7ccf00ddfc..fa3a607840 100644 --- a/includes/job/RefreshLinksJob.php +++ b/includes/job/RefreshLinksJob.php @@ -58,11 +58,11 @@ class RefreshLinksJob extends Job { wfProfileIn( __METHOD__.'-parse' ); $options = ParserOptions::newFromUserAndLang( new User, $wgContLang ); - $parserOutput = $wgParser->parse( $revision->getText(), $this->title, $options, true, true, $revision->getId() ); + $parserOutput = $wgParser->parse( $revision->getText(), $this->title, $options, true, true, $revision->getId() ); #FIXME: content wfProfileOut( __METHOD__.'-parse' ); wfProfileIn( __METHOD__.'-update' ); - $updates = $parserOutput->getSecondaryDataUpdates( $this->title, false ); + $updates = $parserOutput->getSecondaryDataUpdates( $this->title, false ); #FIXME: content handler DataUpdate::runUpdates( $updates ); wfProfileOut( __METHOD__.'-update' ); @@ -132,11 +132,11 @@ class RefreshLinksJob2 extends Job { return false; } wfProfileIn( __METHOD__.'-parse' ); - $parserOutput = $wgParser->parse( $revision->getText(), $title, $options, true, true, $revision->getId() ); + $parserOutput = $wgParser->parse( $revision->getText(), $title, $options, true, true, $revision->getId() ); #FIXME: content handler wfProfileOut( __METHOD__.'-parse' ); wfProfileIn( __METHOD__.'-update' ); - $updates = $parserOutput->getSecondaryDataUpdates( $title, false ); + $updates = $parserOutput->getSecondaryDataUpdates( $title, false ); #FIXME: content handler DataUpdate::runUpdates( $updates ); wfProfileOut( __METHOD__.'-update' ); diff --git a/includes/parser/ParserOutput.php b/includes/parser/ParserOutput.php index 67eb049a13..06870498dd 100644 --- a/includes/parser/ParserOutput.php +++ b/includes/parser/ParserOutput.php @@ -480,6 +480,9 @@ class ParserOutput extends CacheTime { * extracted from the page's content, including a LinksUpdate object for all links stored in * this ParserOutput object. * + * @note: Avoid using this method directly, use ContentHandler::getSecondaryDataUpdates() instead! The content + * handler may provide additional update objects. + * * @param $title Title of the page we're updating. If not given, a title object will be created based on $this->getTitleText() * @param $recursive Boolean: queue jobs for recursive updates? * diff --git a/maintenance/refreshLinks.php b/maintenance/refreshLinks.php index 602874b7f7..e20071b4eb 100644 --- a/maintenance/refreshLinks.php +++ b/maintenance/refreshLinks.php @@ -218,9 +218,8 @@ class RefreshLinks extends Maintenance { $dbw = wfGetDB( DB_MASTER ); $dbw->begin( __METHOD__ ); - $context = RequestContext::getMain(); - - $updates = $parserOutput->getSecondaryDataUpdates( $page->getTitle(), false ); + $contentHandler = $content->getContentHandler(); + $updates = $contentHandler->getSecondaryDataUpdates( $content, $page->getTitle() ); DataUpdate::runUpdates( $updates ); $dbw->commit( __METHOD__ ); diff --git a/tests/phpunit/includes/ContentHandlerTest.php b/tests/phpunit/includes/ContentHandlerTest.php index 4c9ce06265..47f228a02b 100644 --- a/tests/phpunit/includes/ContentHandlerTest.php +++ b/tests/phpunit/includes/ContentHandlerTest.php @@ -243,7 +243,112 @@ class ContentHandlerTest extends MediaWikiTestCase { } + public function dataGetParserOutput() { + return array( + array("ContentHandlerTest_testGetParserOutput", "hello ''world''\n", "

hello world\n

"), + // @todo: more...? + ); + } + + /** + * @dataProvider dataGetParserOutput + */ + public function testGetParserOutput( $title, $text, $expectedHtml ) { + $title = Title::newFromText( $title ); + $handler = ContentHandler::getForModelID( $title->getContentModel() ); + $content = ContentHandler::makeContent( $text, $title ); + + $po = $handler->getParserOutput( $content, $title ); + + $this->assertEquals( $expectedHtml, $po->getText() ); + // @todo: assert more properties + } + + public function dataGetSecondaryDataUpdates() { + return array( + array("ContentHandlerTest_testGetSecondaryDataUpdates_1", "hello ''world''\n", + array( 'LinksUpdate' => array( 'mRecursive' => true, + 'mLinks' => array() ) ) + ), + array("ContentHandlerTest_testGetSecondaryDataUpdates_2", "hello [[world test 21344]]\n", + array( 'LinksUpdate' => array( 'mRecursive' => true, + 'mLinks' => array( array( 'World_test_21344' => 0 ) ) ) ) + ), + // @todo: more...? + ); + } + + /** + * @dataProvider dataGetSecondaryDataUpdates + */ + public function testGetSecondaryDataUpdates( $title, $text, $expectedStuff ) { + $title = Title::newFromText( $title ); + $title->resetArticleID( 2342 ); //dummy id. fine as long as we don't try to execute the updates! + + $handler = ContentHandler::getForModelID( $title->getContentModel() ); + $content = ContentHandler::makeContent( $text, $title ); + + $updates = $handler->getSecondaryDataUpdates( $content, $title ); + + // make updates accessible by class name + foreach ( $updates as $update ) { + $class = get_class( $update ); + $updates[ $class ] = $update; + } + foreach ( $expectedStuff as $class => $fieldValues ) { + $this->assertArrayHasKey( $class, $updates, "missing an update of type $class" ); + + $update = $updates[ $class ]; + + foreach ( $fieldValues as $field => $value ) { + $v = $update->$field; #if the field doesn't exist, just crash and burn + $this->assertEquals( $value, $v, "unexpected value for field $field in instance of $class" ); + } + } + } + + public function dataGetDeletionUpdates() { + return array( + array("ContentHandlerTest_testGetSecondaryDataUpdates_1", "hello ''world''\n", + array( 'LinksDeletionUpdate' => array( ) ) + ), + array("ContentHandlerTest_testGetSecondaryDataUpdates_2", "hello [[world test 21344]]\n", + array( 'LinksDeletionUpdate' => array( ) ) + ), + // @todo: more...? + ); + } + + /** + * @dataProvider dataGetDeletionUpdates + */ + public function testDeletionUpdates( $title, $text, $expectedStuff ) { + $title = Title::newFromText( $title ); + $title->resetArticleID( 2342 ); //dummy id. fine as long as we don't try to execute the updates! + + $handler = ContentHandler::getForModelID( $title->getContentModel() ); + $content = ContentHandler::makeContent( $text, $title ); + + $updates = $handler->getDeletionUpdates( $content, $title ); + + // make updates accessible by class name + foreach ( $updates as $update ) { + $class = get_class( $update ); + $updates[ $class ] = $update; + } + + foreach ( $expectedStuff as $class => $fieldValues ) { + $this->assertArrayHasKey( $class, $updates, "missing an update of type $class" ); + + $update = $updates[ $class ]; + + foreach ( $fieldValues as $field => $value ) { + $v = $update->$field; #if the field doesn't exist, just crash and burn + $this->assertEquals( $value, $v, "unexpected value for field $field in instance of $class" ); + } + } + } } class DummyContentHandlerForTesting extends ContentHandler { -- 2.20.1