jenkins-bot has submitted this change and it was merged.
Change subject: Remove getSecondaryDataUpdates and friends from ParserOutput.
......................................................................
Remove getSecondaryDataUpdates and friends from ParserOutput.
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
---
M RELEASE-NOTES-1.25
M docs/hooks.txt
M includes/api/ApiStashEdit.php
M includes/content/AbstractContent.php
M includes/content/Content.php
M includes/parser/ParserOutput.php
M tests/phpunit/includes/parser/ParserOutputTest.php
7 files changed, 63 insertions(+), 119 deletions(-)
Approvals:
Aaron Schulz: Looks good to me, approved
jenkins-bot: Verified
diff --git a/RELEASE-NOTES-1.25 b/RELEASE-NOTES-1.25
index 18b7f61..83a569a 100644
--- a/RELEASE-NOTES-1.25
+++ b/RELEASE-NOTES-1.25
@@ -373,6 +373,11 @@
* 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 ==
diff --git a/docs/hooks.txt b/docs/hooks.txt
index 1d211af..cf2e3dd 100644
--- a/docs/hooks.txt
+++ b/docs/hooks.txt
@@ -2370,6 +2370,18 @@
'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
diff --git a/includes/api/ApiStashEdit.php b/includes/api/ApiStashEdit.php
index 09489e4..3457670 100644
--- a/includes/api/ApiStashEdit.php
+++ b/includes/api/ApiStashEdit.php
@@ -334,11 +334,7 @@
$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,
diff --git a/includes/content/AbstractContent.php
b/includes/content/AbstractContent.php
index 816572c..d8fca4d 100644
--- a/includes/content/AbstractContent.php
+++ b/includes/content/AbstractContent.php
@@ -204,13 +204,13 @@
* 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 @@
* @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;
}
/**
diff --git a/includes/content/Content.php b/includes/content/Content.php
index 61b9254..5823a9a 100644
--- a/includes/content/Content.php
+++ b/includes/content/Content.php
@@ -292,6 +292,9 @@
* 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
diff --git a/includes/parser/ParserOutput.php b/includes/parser/ParserOutput.php
index 6b9cb05..f249017 100644
--- a/includes/parser/ParserOutput.php
+++ b/includes/parser/ParserOutput.php
@@ -53,8 +53,6 @@
$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 @@
}
/**
- * 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 @@
public function __sleep() {
return array_diff(
array_keys( get_object_vars( $this ) ),
- array( 'mSecondaryDataUpdates', 'mParseStartTime' )
+ array( 'mParseStartTime' )
);
}
}
diff --git a/tests/phpunit/includes/parser/ParserOutputTest.php
b/tests/phpunit/includes/parser/ParserOutputTest.php
index fe46f2c..e660e09 100644
--- a/tests/phpunit/includes/parser/ParserOutputTest.php
+++ b/tests/phpunit/includes/parser/ParserOutputTest.php
@@ -89,65 +89,4 @@
$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 );
- }
-
}
--
To view, visit https://gerrit.wikimedia.org/r/190808
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I0f6f49e61fa3d8904e55f42c99f342a3dc357495
Gerrit-PatchSet: 4
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: Daniel Kinzler <[email protected]>
Gerrit-Reviewer: Jackmcbarn <[email protected]>
Gerrit-Reviewer: Nikerabbit <[email protected]>
Gerrit-Reviewer: Ori.livneh <[email protected]>
Gerrit-Reviewer: Tim Starling <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
