jenkins-bot has submitted this change and it was merged.
Change subject: Fix ApiStashEdit wrt custom DataUpdates.
......................................................................
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
---
M includes/api/ApiStashEdit.php
M includes/parser/ParserOutput.php
M tests/phpunit/includes/parser/ParserOutputTest.php
3 files changed, 91 insertions(+), 10 deletions(-)
Approvals:
Aaron Schulz: Looks good to me, approved
jenkins-bot: Verified
diff --git a/includes/api/ApiStashEdit.php b/includes/api/ApiStashEdit.php
index 4d181d6..09489e4 100644
--- a/includes/api/ApiStashEdit.php
+++ b/includes/api/ApiStashEdit.php
@@ -335,10 +335,8 @@
$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
--- a/includes/parser/ParserOutput.php
+++ b/includes/parser/ParserOutput.php
@@ -53,7 +53,8 @@
$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 @@
$this->mCategories = $categoryLinks;
$this->mContainsOldMagic = $containsOldMagic;
$this->mTitleText = $titletext;
-
- $this->mSecondaryDataUpdates = array();
}
public function getText() {
@@ -682,12 +681,30 @@
* 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 @@
$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
--- 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 @@
$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 );
+ }
+
}
--
To view, visit https://gerrit.wikimedia.org/r/183890
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic114e521c5dfd0d3c028ea7d16e93eace758deef
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Daniel Kinzler <[email protected]>
Gerrit-Reviewer: Aaron Schulz <[email protected]>
Gerrit-Reviewer: Anomie <[email protected]>
Gerrit-Reviewer: Cscott <[email protected]>
Gerrit-Reviewer: Jackmcbarn <[email protected]>
Gerrit-Reviewer: Ori.livneh <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
