cleanup and fixes for secondary data updates
authordaniel <daniel.kinzler@wikimedia.de>
Thu, 7 Jun 2012 12:57:43 +0000 (14:57 +0200)
committerdaniel <daniel.kinzler@wikimedia.de>
Thu, 7 Jun 2012 12:57:43 +0000 (14:57 +0200)
includes/Content.php
includes/ContentHandler.php
includes/LinksUpdate.php
includes/Revision.php
includes/WikiPage.php
includes/api/ApiPurge.php
includes/job/RefreshLinksJob.php
includes/parser/ParserOutput.php
maintenance/refreshLinks.php
tests/phpunit/includes/ContentHandlerTest.php

index 6c23b09..490bb8c 100644 (file)
@@ -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.
index e797048..1495eb1 100644 (file)
@@ -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 );
        }
 
 
index d6db70f..3e8e362 100644 (file)
@@ -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?" );
+               }
        }
 
        /**
index 50aba9f..48c34e9 100644 (file)
@@ -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__ );
index fdb0792..1612cc7 100644 (file)
@@ -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'] ) );
index 5ca3fa8..66aa3b0 100644 (file)
@@ -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'] = '';
index 7ccf00d..fa3a607 100644 (file)
@@ -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' );
index 67eb049..0687049 100644 (file)
@@ -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?
         *
index 602874b..e20071b 100644 (file)
@@ -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__ );
index 4c9ce06..47f228a 100644 (file)
@@ -243,7 +243,112 @@ class ContentHandlerTest extends MediaWikiTestCase {
 
        }
 
+       public function dataGetParserOutput() {
+               return array(
+                       array("ContentHandlerTest_testGetParserOutput", "hello ''world''\n", "<p>hello <i>world</i>\n</p>"),
+                       // @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 {