Fix ApiStashEdit wrt custom DataUpdates.

My previous patch broke this: ApiStashEdit would stash ParserOutput
with no custom DataUpdates, but calling getSecondaryDataUpdates still
failed after unserialization. This patch should fix that.

Bug: T86305
Change-Id: Ic114e521c5dfd0d3c028ea7d16e93eace758deef

diff --git a/includes/api/ApiStashEdit.php b/includes/api/ApiStashEdit.php
index 4d181d6..09489e4 100644 (file)
--- a/includes/api/ApiStashEdit.php
+++ b/includes/api/ApiStashEdit.php
@@ -335,10 +335,8 @@ class ApiStashEdit extends ApiBase {
                $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 ParserOtput::__sleep).
-               // The first data update returned by getSecondaryDataUpdates() is always a LinksUpdate
-               // instance generated on the fly, so it can be ignored in this context.
-               $hasCustomDataUpdates = count( $parserOutput->getSecondaryDataUpdates() ) > 1;
+               // stashed, since the callbacks are not serializable (see ParserOutput::__sleep).
+               $hasCustomDataUpdates = $parserOutput->hasCustomDataUpdates();
 
                if ( $ttl > 0 && !$parserOutput->getFlag( 'vary-revision' ) && !$hasCustomDataUpdates ) {
                        // Only store what is actually needed

diff --git a/includes/parser/ParserOutput.php b/includes/parser/ParserOutput.php
index f155312..117e04a 100644 (file)
--- a/includes/parser/ParserOutput.php
+++ b/includes/parser/ParserOutput.php
@@ -53,7 +53,8 @@ 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 = null; # List of DataUpdate, used to save info from the page somewhere else.
+       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()
@@ -70,8 +71,6 @@ class ParserOutput extends CacheTime {
                $this->mCategories = $categoryLinks;
                $this->mContainsOldMagic = $containsOldMagic;
                $this->mTitleText = $titletext;
-
-               $this->mSecondaryDataUpdates = array();
        }
 
        public function getText() {
@@ -682,12 +681,30 @@ class ParserOutput extends CacheTime {
         * 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.
         *
+        * @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 $wgAjaxStashEdit is enabled. For this
+        * reason, ApiStashEdit will skip any ParserOutput that has custom DataUpdates.
+        *
         * @since 1.20
         *
         * @param DataUpdate $update
         */
        public function addSecondaryDataUpdate( DataUpdate $update ) {
                $this->mSecondaryDataUpdates[] = $update;
+               $this->mCustomDataUpdateCount = count( $this->mSecondaryDataUpdates );
+       }
+
+       /**
+        * Whether this ParserOutput contains custom DataUpdate objects that may not survive
+        * serialization of the ParserOutput.
+        *
+        * @see __sleep()
+        *
+        * @return bool
+        */
+       public function hasCustomDataUpdates() {
+               return ( $this->mCustomDataUpdateCount > 0 );
        }
 
        /**
@@ -714,10 +731,10 @@ class ParserOutput extends CacheTime {
                        $title = Title::newFromText( $this->getTitleText() );
                }
 
-               if ( $this->mSecondaryDataUpdates === null ) {
-                       //NOTE: This happens when mSecondaryDataUpdates are lost during serialization
+               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, and should throw an exception.
+                       // 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.' );
                }
 

diff --git a/tests/phpunit/includes/parser/ParserOutputTest.php b/tests/phpunit/includes/parser/ParserOutputTest.php
index c024cee..a6b4880 100644 (file)
--- a/tests/phpunit/includes/parser/ParserOutputTest.php
+++ b/tests/phpunit/includes/parser/ParserOutputTest.php
@@ -1,5 +1,9 @@
 <?php
 
+/**
+ * @group Database
+ *        ^--- trigger DB shadowing because we are using Title magic
+ */
 class ParserOutputTest extends MediaWikiTestCase {
 
        public static function provideIsLinkInternal() {
@@ -84,4 +88,66 @@ class ParserOutputTest extends MediaWikiTestCase {
                $this->assertEquals( $po->getProperty( 'foo' ), false );
                $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::getSecondaryDataUpdate
+        * @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::getSecondaryDataUpdate
+        * @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 );
+       }
+
 }