Remove getSecondaryDataUpdates and friends from ParserOutput.
authordaniel <daniel.kinzler@wikimedia.de>
Sun, 15 Feb 2015 19:34:43 +0000 (20:34 +0100)
committerdaniel <daniel.kinzler@wikimedia.de>
Tue, 24 Feb 2015 10:01:16 +0000 (11:01 +0100)
This is a hard deprecation, with getSecondaryDataUpdates returning an
empty array and addSecondaryDataUpdate throwing an exception. This seems
prudent since there are no known users of these methods, and they
interfere with the parser cache:

DataUpdates are basically jobs, they need access to services to
function. That makes them inherently non-serializable. This interferes
with the function of the parser cache, which serializes ParserOutput
objects in order to persist them.

This could be solved by splitting DataUpdates into DataUpdateDefinitions
and DataUpdateHandlers, similar to how JobSpecification works with
wgJobClasses. That however seems pointless and overkill, since
ParserOutput already has a mechanism for storing arbitrary data,
including any info needed by an UpdateJob: the setExtensionData method.

After this change, the preferred method to introduce custom data updates
is to store any relevant data using setExtensionData and
implement Content::getSecondaryDataUpdates() if possible. If not,
use the 'SecondaryDataUpdates' hook to construct the necessary update
objects from the info stored using setExtensionData.

Change-Id: I0f6f49e61fa3d8904e55f42c99f342a3dc357495

RELEASE-NOTES-1.25
docs/hooks.txt
includes/api/ApiStashEdit.php
includes/content/AbstractContent.php
includes/content/Content.php
includes/parser/ParserOutput.php
tests/phpunit/includes/parser/ParserOutputTest.php

index 18b7f61..83a569a 100644 (file)
@@ -373,6 +373,11 @@ changes to languages because of Bugzilla reports.
 * Removed maintenance script dumpSisterSites.php.
 * DatabaseBase class constructors must be called using the array argument style.
   Ideally, DatabaseBase:factory() should be used instead in most cases.
+* Deprecated ParserOutput::addSecondaryDataUpdate and ParserOutput::getSecondaryDataUpdates.
+  This is a hard deprecation, with getSecondaryDataUpdates returning an empty array and
+  addSecondaryDataUpdate throwing an exception. These functions will be removed in 1.26,
+  since they interfere with caching of ParserOutput objects.
+* Introduced new hook 'SecondaryDataUpdates' that allows extensions to inject custom updates.
 
 == Compatibility ==
 
index 1d211af..cf2e3dd 100644 (file)
@@ -2370,6 +2370,18 @@ $title : Current Title object being displayed in search results.
 'SearchableNamespaces': An option to modify which namespaces are searchable.
 &$arr : Array of namespaces ($nsId => $name) which will be used.
 
+'SecondaryDataUpdates': Allows modification of the list of DataUpdates to
+perform when page content is modified. Currently called by
+AbstractContent::getSecondaryDataUpdates.
+$title: Title of the page that is being edited.
+$oldContent: Content object representing the page's content before the edit.
+$recursive: bool indicating whether DataUpdates should trigger recursive
+  updates (relevant mostly for LinksUpdate).
+$parserOutput: ParserOutput representing the rendered version of the page
+  after the edit.
+&$updates: a list of DataUpdate objects, to be modified or replaced by
+  the hook handler.
+
 'SelfLinkBegin': Called before a link to the current article is displayed to
 allow the display of the link to be customized.
 $nt: the Title object
index 09489e4..3457670 100644 (file)
@@ -334,11 +334,7 @@ class ApiStashEdit extends ApiBase {
                $since = time() - wfTimestamp( TS_UNIX, $parserOutput->getTimestamp() );
                $ttl = min( $parserOutput->getCacheExpiry() - $since, 5 * 60 );
 
-               // Note: ParserOutput with that contains secondary data update callbacks can not be
-               // stashed, since the callbacks are not serializable (see ParserOutput::__sleep).
-               $hasCustomDataUpdates = $parserOutput->hasCustomDataUpdates();
-
-               if ( $ttl > 0 && !$parserOutput->getFlag( 'vary-revision' ) && !$hasCustomDataUpdates ) {
+               if ( $ttl > 0 && !$parserOutput->getFlag( 'vary-revision' ) ) {
                        // Only store what is actually needed
                        $stashInfo = (object)array(
                                'pstContent' => $pstContent,
index 816572c..d8fca4d 100644 (file)
@@ -204,13 +204,13 @@ abstract class AbstractContent implements Content {
         * Returns a list of DataUpdate objects for recording information about this
         * Content in some secondary data store.
         *
-        * This default implementation calls
-        * $this->getParserOutput( $content, $title, null, null, false ),
-        * and then calls getSecondaryDataUpdates( $title, $recursive ) on the
-        * resulting ParserOutput object.
+        * This default implementation returns a LinksUpdate object and calls the
+        * SecondaryDataUpdates hook.
         *
         * Subclasses may override this to determine the secondary data updates more
         * efficiently, preferably without the need to generate a parser output object.
+        * They should however make sure to call SecondaryDataUpdates to give extensions
+        * a chance to inject additional updates.
         *
         * @since 1.21
         *
@@ -224,12 +224,19 @@ abstract class AbstractContent implements Content {
         * @see Content::getSecondaryDataUpdates()
         */
        public function getSecondaryDataUpdates( Title $title, Content $old = null,
-               $recursive = true, ParserOutput $parserOutput = null ) {
+               $recursive = true, ParserOutput $parserOutput = null
+       ) {
                if ( $parserOutput === null ) {
                        $parserOutput = $this->getParserOutput( $title, null, null, false );
                }
 
-               return $parserOutput->getSecondaryDataUpdates( $title, $recursive );
+               $updates = array(
+                       new LinksUpdate( $title, $parserOutput, $recursive )
+               );
+
+               Hooks::run( 'SecondaryDataUpdates', array( $title, $old, $recursive, $parserOutput, &$updates ) );
+
+               return $updates;
        }
 
        /**
index 61b9254..5823a9a 100644 (file)
@@ -292,6 +292,9 @@ interface Content {
         * Subclasses may implement this to determine the necessary updates more
         * efficiently, or make use of information about the old content.
         *
+        * @note Implementations should call the SecondaryDataUpdates hook, like
+        *   AbstractContent does.
+        *
         * @param Title $title The context for determining the necessary updates
         * @param Content $old An optional Content object representing the
         *    previous content, i.e. the content being replaced by this Content
index 6b9cb05..f249017 100644 (file)
@@ -53,8 +53,6 @@ class ParserOutput extends CacheTime {
                $mTOCEnabled = true;          # Whether TOC should be shown, can't override __NOTOC__
        private $mIndexPolicy = '';       # 'index' or 'noindex'?  Any other value will result in no change.
        private $mAccessedOptions = array(); # List of ParserOptions (stored in the keys)
-       private $mSecondaryDataUpdates = array(); # List of DataUpdate, used to save info from the page somewhere else.
-       private $mCustomDataUpdateCount = 0; # Number of custom updaters in $mSecondaryDataUpdates.
        private $mExtensionData = array(); # extra data used by extensions
        private $mLimitReportData = array(); # Parser limit report data
        private $mParseStartTime = array(); # Timestamps for getTimeSinceStart()
@@ -677,73 +675,57 @@ class ParserOutput extends CacheTime {
        }
 
        /**
-        * Adds an update job to the output. Any update jobs added to the output will
-        * eventually be executed in order to store any secondary information extracted
-        * from the page's content. This is triggered by calling getSecondaryDataUpdates()
-        * and is used for forward links updates on edit and backlink updates by jobs.
+        * @deprecated since 1.25. Instead, store any relevant data using setExtensionData,
+        *    and implement Content::getSecondaryDataUpdates() if possible, or use the
+        *    'SecondaryDataUpdates' hook to construct the necessary update objects.
         *
-        * @note: custom DataUpdates do not survive serialization of the ParserOutput!
-        * This is especially relevant when using a cached ParserOutput for updating
-        * the database, as WikiPage does if $wgAjaxEditStash is enabled. For this
-        * reason, ApiStashEdit will skip any ParserOutput that has custom DataUpdates.
+        * @note Hard deprecation and removal without long deprecation period, since there are no
+        *       known users, but known conceptual issues.
         *
-        * @since 1.20
+        * @todo remove in 1.26
         *
         * @param DataUpdate $update
+        *
+        * @throws MWException
         */
        public function addSecondaryDataUpdate( DataUpdate $update ) {
-               $this->mSecondaryDataUpdates[] = $update;
-               $this->mCustomDataUpdateCount = count( $this->mSecondaryDataUpdates );
+               wfDeprecated( __METHOD__, '1.25' );
+               throw new MWException( 'ParserOutput::addSecondaryDataUpdate() is no longer supported. Override Content::getSecondaryDataUpdates() or use the SecondaryDataUpdates hook instead.' );
        }
 
        /**
-        * Whether this ParserOutput contains custom DataUpdate objects that may not survive
-        * serialization of the ParserOutput.
+        * @deprecated since 1.25.
         *
-        * @see __sleep()
+        * @note Hard deprecation and removal without long deprecation period, since there are no
+        *       known users, but known conceptual issues.
         *
-        * @return bool
+        * @todo remove in 1.26
+        *
+        * @return bool false (since 1.25)
         */
        public function hasCustomDataUpdates() {
-               return ( $this->mCustomDataUpdateCount > 0 );
+               wfDeprecated( __METHOD__, '1.25' );
+               return false;
        }
 
        /**
-        * Returns any DataUpdate jobs to be executed in order to store secondary information
-        * extracted from the page's content, including a LinksUpdate object for all links stored in
-        * this ParserOutput object.
+        * @deprecated since 1.25. Instead, store any relevant data using setExtensionData,
+        *    and implement Content::getSecondaryDataUpdates() if possible, or use the
+        *    'SecondaryDataUpdates' hook to construct the necessary update objects.
         *
-        * @note Avoid using this method directly, use ContentHandler::getSecondaryDataUpdates()
-        *   instead! The content handler may provide additional update objects.
+        * @note Hard deprecation and removal without long deprecation period, since there are no
+        *       known users, but known conceptual issues.
         *
-        * @since 1.20
+        * @todo remove in 1.26
         *
-        * @param Title $title The title of the page we're updating. If not given, a title object will
-        *    be created based on $this->getTitleText()
-        * @param bool $recursive Queue jobs for recursive updates?
-        *
-        * @throws MWException if called on a ParserOutput instance that was restored from serialization.
-        *         DataUpdates are generally not serializable, so after serialization, they are undefined.
+        * @param Title $title
+        * @param bool $recursive
         *
         * @return array An array of instances of DataUpdate
         */
        public function getSecondaryDataUpdates( Title $title = null, $recursive = true ) {
-               if ( is_null( $title ) ) {
-                       $title = Title::newFromText( $this->getTitleText() );
-               }
-
-               if ( count( $this->mSecondaryDataUpdates ) !== $this->mCustomDataUpdateCount ) {
-                       // NOTE: This happens when mSecondaryDataUpdates are lost during serialization
-                       // (see __sleep below). After (un)serialization, getSecondaryDataUpdates()
-                       // has no defined behavior in that case, and should throw an exception.
-                       throw new MWException( 'getSecondaryDataUpdates() must not be called on ParserOutput restored from serialization.' );
-               }
-
-               // NOTE: ApiStashEdit knows about this "magic" update object. If this goes away,
-               // ApiStashEdit::buildStashValue needs to be adjusted.
-               $linksUpdate = new LinksUpdate( $title, $this, $recursive );
-
-               return array_merge( $this->mSecondaryDataUpdates, array( $linksUpdate ) );
+               wfDeprecated( __METHOD__, '1.25' );
+               return array();
        }
 
        /**
@@ -913,7 +895,7 @@ class ParserOutput extends CacheTime {
        public function __sleep() {
                return array_diff(
                        array_keys( get_object_vars( $this ) ),
-                       array( 'mSecondaryDataUpdates', 'mParseStartTime' )
+                       array( 'mParseStartTime' )
                );
        }
 }
index fe46f2c..e660e09 100644 (file)
@@ -89,65 +89,4 @@ class ParserOutputTest extends MediaWikiTestCase {
                $this->assertArrayNotHasKey( 'foo', $properties );
        }
 
-       /**
-        * @covers ParserOutput::hasCustomDataUpdates
-        * @covers ParserOutput::addSecondaryDataUpdate
-        */
-       public function testHasCustomDataUpdates() {
-               $po = new ParserOutput();
-               $this->assertFalse( $po->hasCustomDataUpdates() );
-
-               $dataUpdate = $this->getMock( 'DataUpdate' );
-               $po->addSecondaryDataUpdate( $dataUpdate );
-               $this->assertTrue( $po->hasCustomDataUpdates() );
-       }
-
-       /**
-        * @covers ParserOutput::getSecondaryDataUpdates
-        * @covers ParserOutput::addSecondaryDataUpdate
-        */
-       public function testGetSecondaryDataUpdates() {
-               // NOTE: getSecondaryDataUpdates always returns a LinksUpdate object
-               // in addition to the DataUpdates registered via addSecondaryDataUpdate().
-
-               $title = Title::makeTitle( NS_MAIN, 'Dummy' );
-               $title->resetArticleID( 7777777 );
-
-               $po = new ParserOutput();
-               $this->assertCount( 1, $po->getSecondaryDataUpdates( $title ) );
-
-               $dataUpdate = $this->getMock( 'DataUpdate' );
-               $po->addSecondaryDataUpdate( $dataUpdate );
-               $this->assertCount( 2, $po->getSecondaryDataUpdates( $title ) );
-
-               // Test Fallback to getTitleText
-               $this->insertPage( 'Project:ParserOutputTestDummyPage' );
-               $po->setTitleText( 'Project:ParserOutputTestDummyPage' );
-               $this->assertCount( 2, $po->getSecondaryDataUpdates() );
-       }
-
-       /**
-        * @covers ParserOutput::getSecondaryDataUpdates
-        * @covers ParserOutput::__sleep
-        */
-       public function testGetSecondaryDataUpdates_serialization() {
-               $title = Title::makeTitle( NS_MAIN, 'Dummy' );
-               $title->resetArticleID( 7777777 );
-
-               $po = new ParserOutput();
-
-               // Serializing is fine with no custom DataUpdates.
-               $po = unserialize( serialize( $po ) );
-               $this->assertCount( 1, $po->getSecondaryDataUpdates( $title ) );
-
-               // If there are custom DataUpdates, getSecondaryDataUpdates
-               // should fail after serialization.
-               $dataUpdate = $this->getMock( 'DataUpdate' );
-               $po->addSecondaryDataUpdate( $dataUpdate );
-               $po = unserialize( serialize( $po ) );
-
-               $this->setExpectedException( 'MWException' );
-               $po->getSecondaryDataUpdates( $title );
-       }
-
 }