GWicke has submitted this change and it was merged.
Change subject: Various small code cleanups
......................................................................
Various small code cleanups
* Use JobQueueGroup::push() in updateTitle().
* Made CurlMultiClient::request() return an array if no urls are given.
Also cleaned up the options and error checking code a bit.
* Code style formatting.
* Added some type hints and make use of the 'self' keyword.
* Added some TODO/FIXME comments.
Change-Id: Iad4de0f0017b3f079971d76523d3a5a5487e9155
---
M php/CurlMultiClient.php
M php/Parsoid.hooks.php
M php/Parsoid.php
M php/ParsoidCacheUpdateJob.php
4 files changed, 100 insertions(+), 92 deletions(-)
Approvals:
GWicke: Verified; Looks good to me, approved
diff --git a/php/CurlMultiClient.php b/php/CurlMultiClient.php
index 0ad2619..582a419 100644
--- a/php/CurlMultiClient.php
+++ b/php/CurlMultiClient.php
@@ -1,6 +1,8 @@
<?php
+
/**
* A simple parallel CURL client helper class
+ * @TODO: name this ParsoidCurlMultiClient or move to core
*/
class CurlMultiClient {
@@ -10,10 +12,10 @@
* @static
* @returns array default options
*/
- public static function getDefaultOptions () {
+ public static function getDefaultOptions() {
return array(
- CURLOPT_HEADER=>0,
- CURLOPT_RETURNTRANSFER=>1
+ CURLOPT_HEADER => 0,
+ CURLOPT_RETURNTRANSFER => 1
);
}
@@ -23,11 +25,11 @@
*
* @static
* @param $requests array requests, each with an url and an optional
- * 'headers' member:
- * array(
- * 'url' => 'http://server.com/foo',
- * 'headers' => array( 'X-Foo: Bar' )
- * )
+ * 'headers' member:
+ * array(
+ * 'url' => 'http://server.com/foo',
+ * 'headers' => array( 'X-Foo: Bar' )
+ * )
* @param $options array curl options used for each request, default
* {CurlMultiClient::getDefaultOptions}.
* @returns array An array of arrays containing 'error' and 'data'
@@ -35,45 +37,44 @@
* errors, the error member will be null and data will contain the
* response data as a string.
*/
- public static function request($requests, $options=""){
-
-
- if( !count( $requests ) ) return false;
+ public static function request( $requests, array $options = null ) {
+ if ( !count( $requests ) ) {
+ return array();
+ }
$handles = array();
- if( !$options ) // add default options
+ if ( $options === null ) { // add default options
$options = CurlMultiClient::getDefaultOptions();
+ }
// add curl options to each handle
- foreach( $requests as $k => $row ){
+ foreach ( $requests as $k => $row ) {
$handle = curl_init();
- $options[CURLOPT_URL] = $row['url'];
- wfDebug("adding url: " . $row['url']);
- if ( array_key_exists( 'headers', $row ) ) {
- $options[CURLOPT_HTTPHEADER] = $row['headers'];
- } else if ( array_key_exists( CURLOPT_HTTPHEADER,
$options ) ) {
- unset( $options[CURLOPT_HTTPHEADER] );
+ $reqOptions = array( CURLOPT_URL => $row['url'] ) +
$options;
+ wfDebug( "adding url: " . $row['url'] );
+ if ( isset( $row['headers'] ) ) {
+ $reqOptions[CURLOPT_HTTPHEADER] =
$row['headers'];
}
- curl_setopt_array($handle, $options);
+ curl_setopt_array( $handle, $reqOptions );
$handles[$k] = $handle;
}
$mh = curl_multi_init();
- foreach( $handles as $handle ){
- curl_multi_add_handle($mh,$handle);
+ foreach ( $handles as $handle ) {
+ curl_multi_add_handle( $mh, $handle );
}
$running_handles = null;
//execute the handles
do {
- $status_cme = curl_multi_exec($mh, $running_handles);
+ $status_cme = curl_multi_exec( $mh, $running_handles );
} while ( $status_cme == CURLM_CALL_MULTI_PERFORM );
while ( $running_handles && $status_cme == CURLM_OK ) {
- if ( curl_multi_select($mh) != -1 ) {
+ if ( curl_multi_select( $mh ) != -1 ) {
do {
$status_cme = curl_multi_exec( $mh,
$running_handles );
} while ( $status_cme ==
CURLM_CALL_MULTI_PERFORM );
@@ -81,14 +82,14 @@
}
$res = array();
- foreach( $requests as $k => $row ){
+ foreach ( $requests as $k => $row ) {
$res[$k] = array();
$res[$k]['error'] = curl_error( $handles[$k] );
- if( !empty( $res[$k]['error'] ) ) {
- $res[$k]['data'] = null;
+ if ( strlen( $res[$k]['error'] ) ) {
+ $res[$k]['data'] = null;
} else {
$res[$k]['error'] = null;
- $res[$k]['data'] = curl_multi_getcontent(
$handles[$k] ); // get results
+ $res[$k]['data'] = curl_multi_getcontent(
$handles[$k] ); // get results
}
// close current handler
@@ -99,4 +100,5 @@
#wfDebug(serialize($res));
return $res; // return response
}
+
}
diff --git a/php/Parsoid.hooks.php b/php/Parsoid.hooks.php
index 0b41c23..7e689ae 100644
--- a/php/Parsoid.hooks.php
+++ b/php/Parsoid.hooks.php
@@ -4,15 +4,17 @@
* Hooks for events that should trigger Parsoid cache updates.
*/
class ParsoidHooks {
+
/**
* Schedule an async update job in the job queue. The params passed here
* are empty. They are dynamically created in ParsoidCacheUpdateJob
* depending on title namespace etc.
+ *
+ * @param Title $title
+ * @param string $action (@TODO: unused)
*/
- private static function updateTitle ( $title, $action ) {
- $jobs = array();
- $jobs[] = new ParsoidCacheUpdateJob( $title, array() );
- Job::batchInsert( $jobs );
+ private static function updateTitle( Title $title, $action ) {
+ JobQueueGroup::singleton()->push( new ParsoidCacheUpdateJob(
$title, array() ) );
}
/**
@@ -23,9 +25,9 @@
* @param bool $changed
* @return bool
*/
- public static function onArticleEditUpdates ( &$article, &$editInfo,
$changed ) {
+ public static function onArticleEditUpdates( $article, $editInfo,
$changed ) {
if ( $changed ) {
- ParsoidHooks::updateTitle( $article->getTitle(), 'edit'
);
+ self::updateTitle( $article->getTitle(), 'edit' );
}
return true;
}
@@ -39,17 +41,16 @@
* @param int $id the page id
* @return bool
*/
- public static function onArticleDeleteComplete ( &$article, User
&$user, $reason, $id )
- {
- ParsoidHooks::updateTitle( $article->getTitle(), 'delete' );
+ public static function onArticleDeleteComplete( $article, User $user,
$reason, $id ) {
+ self::updateTitle( $article->getTitle(), 'delete' );
return true;
}
/**
* Callback for article undeletion. See specials/SpecialUndelete.php.
*/
- public static function onArticleUndelete ( $title, $created, $comment )
{
- ParsoidHooks::updateTitle( $title, 'edit' );
+ public static function onArticleUndelete( Title $title, $created,
$comment ) {
+ self::updateTitle( $title, 'edit' );
return true;
}
@@ -57,24 +58,24 @@
* Callback for article revision changes. See
* revisiondelete/RevisionDelete.php.
*/
- public static function onArticleRevisionVisibilitySet ( &$title ) {
+ public static function onArticleRevisionVisibilitySet( Title $title ) {
# We treat all visibility update as deletions for now. That is
safe,
# as it will always clear the cache. VE requests might be slow
after a
# restore, but they will return the correct result.
- ParsoidHooks::updateTitle( $title, 'delete' );
+ self::updateTitle( $title, 'delete' );
return true;
}
/**
* Title move callback. See Title.php.
*/
- public static function onTitleMoveComplete ( Title &$title, Title
&$newtitle,
- User &$user, $oldid, $newid )
- {
+ public static function onTitleMoveComplete(
+ Title $title, Title $newtitle, User $user, $oldid, $newid
+ ) {
# Simply update both old and new title. ParsoidCacheUpdateJob
will
- # do the right thing for both.
- ParsoidHooks::updateTitle( $title, $oldid, 'delete' );
- ParsoidHooks::updateTitle( $newtitle, $newid, 'edit' );
+ # do the right thing for both. @FIXME: this passes extra
parameters
+ self::updateTitle( $title, 'delete', $oldid );
+ self::updateTitle( $newtitle, 'edit', $newid );
return true;
}
@@ -89,10 +90,8 @@
* data. Those edits tend to happen not long after an upload, at which
* point the image is likely not used in many pages.
*/
- public static function onFileUpload ( $file ) {
- $title = $file->getTitle();
- ParsoidHooks::updateTitle( $title, 'file' );
+ public static function onFileUpload( File $file ) {
+ self::updateTitle( $file->getTitle(), 'file' );
return true;
}
-
}
diff --git a/php/Parsoid.php b/php/Parsoid.php
index efa06a7..34f0f45 100644
--- a/php/Parsoid.php
+++ b/php/Parsoid.php
@@ -1,18 +1,18 @@
<?php
+
/**
* Basic cache invalidation for Parsoid
*/
-
if ( !defined( 'MEDIAWIKI' ) ) {
echo "Parsoid extension\n";
exit( 1 );
}
-
/**
* Class containing basic setup functions.
*/
class ParsoidSetup {
+
/**
* Register hook handlers.
* This function must NOT depend on any config vars.
@@ -22,7 +22,7 @@
public static function setUnconditionalHooks() {
global $wgHooks, $wgAutoloadClasses, $wgJobClasses;
- $dir = dirname( __FILE__ );
+ $dir = __DIR__;
# Set up class autoloading
$wgAutoloadClasses['ParsoidHooks'] = "$dir/Parsoid.hooks.php";
@@ -44,8 +44,8 @@
# File upload
$wgHooks['FileUpload'][] = 'ParsoidHooks::onFileUpload';
}
-}
+}
# Load hooks that are always set
ParsoidSetup::setUnconditionalHooks();
diff --git a/php/ParsoidCacheUpdateJob.php b/php/ParsoidCacheUpdateJob.php
index 229d1f2..c38c86c 100644
--- a/php/ParsoidCacheUpdateJob.php
+++ b/php/ParsoidCacheUpdateJob.php
@@ -1,10 +1,13 @@
<?php
+
/**
* HTML cache refreshing and -invalidation job for the Parsoid varnish caches.
* See
*
http://www.mediawiki.org/wiki/Parsoid/Minimal_performance_strategy_for_July_release
+ * @TODO: eventually extend some generic backlink job base class in core
*/
class ParsoidCacheUpdateJob extends HTMLCacheUpdateJob {
+
/**
* Construct a job
* @param $title Title: the title linked to
@@ -12,7 +15,7 @@
* @param $id Integer: job id
*/
function __construct( $title, $params, $id = 0 ) {
- wfDebug("ParsoidCacheUpdateJob.__construct\n");
+ wfDebug( "ParsoidCacheUpdateJob.__construct\n" );
global $wgUpdateRowsPerJob;
Job::__construct( 'ParsoidCacheUpdateJob', $title, $params, $id
);
@@ -56,16 +59,17 @@
* @param bool $prev use previous revision id if true
* @return string an absolute URL for the article on the given server
*/
- protected function getParsoidURL( $title, $server, $prev = false ) {
+ protected function getParsoidURL( Title $title, $server, $prev = false
) {
global $wgLanguageCode;
- $oldid = $prev ?
$title->getPreviousRevisionID($title->getLatestRevID())
- :
$title->getLatestRevID();
+ $oldid = $prev
+ ? $title->getPreviousRevisionID(
$title->getLatestRevID() )
+ : $title->getLatestRevID();
+ // Construct Parsoid web service URL
return 'http://' . $server . '/' . $wgLanguageCode . '/' .
$title->getPrefixedDBkey() . '?oldid=' . $oldid;
}
-
/**
* Invalidate a single title object after an edit. Send headers that let
@@ -74,19 +78,19 @@
*/
protected function invalidateTitle( $title ) {
global $wgParsoidCacheServers;
- if( !isset( $wgParsoidCacheServers ) ) {
- $wgParsoidCacheServers = array( 'localhost' );
+ if ( !isset( $wgParsoidCacheServers ) ) {
+ $wgParsoidCacheServers = array( 'localhost' ); //
@FIXME: test code?
}
# First request the new version
$parsoidInfo = array();
- $parsoidInfo['cacheID'] =
$title->getPreviousRevisionID($title->getLatestRevID());
+ $parsoidInfo['cacheID'] = $title->getPreviousRevisionID(
$title->getLatestRevID() );
$parsoidInfo['changedTitle'] = $this->title->getPrefixedDBkey();
- $requests = array ();
+ $requests = array();
foreach ( $wgParsoidCacheServers as $server ) {
$requests[] = array(
- 'url' => $this->getParsoidURL( $title, $server
),
+ 'url' => $this->getParsoidURL( $title,
$server ),
'headers' => array(
'X-Parsoid: ' . json_encode(
$parsoidInfo ),
// Force implicit cache refresh similar
to
@@ -95,15 +99,16 @@
)
);
};
- wfDebug("ParsoidCacheUpdateJob::invalidateTitle: " .
- serialize($requests) . "\n");
+ wfDebug( "ParsoidCacheUpdateJob::invalidateTitle: " .
serialize( $requests ) . "\n" );
CurlMultiClient::request( $requests );
+ // @TODO: maybe call $this->setLastError() if something went
wrong?
# And now purge the previous revision so that we make efficient
use of
# the Varnish cache space without relying on LRU. Since the URL
# differs we can't use implicit refresh.
- $requests = array ();
+ $requests = array();
foreach ( $wgParsoidCacheServers as $server ) {
+ // @TODO: this triggers a getPreviousRevisionID() query
per server
$requests[] = array(
'url' => $this->getParsoidURL( $title, $server,
true )
);
@@ -111,6 +116,7 @@
$options = CurlMultiClient::getDefaultOptions();
$options[CURLOPT_CUSTOMREQUEST] = "PURGE";
CurlMultiClient::request( $requests, $options );
+ // @TODO: maybe call $this->setLastError() if something went
wrong?
}
/**
@@ -121,10 +127,9 @@
*/
protected function invalidateTitles( $titleArray ) {
global $wgParsoidCacheServers, $wgLanguageCode;
- if( !isset( $wgParsoidCacheServers ) ) {
+ if ( !isset( $wgParsoidCacheServers ) ) {
$wgParsoidCacheServers = array( 'localhost' );
}
-
# Re-render
$parsoidInfo = array();
@@ -146,7 +151,7 @@
$parsoidInfo['cacheID'] =
$title->getLatestRevID();
$requests[] = array(
- 'url' => $url,
+ 'url' => $url,
'headers' => array(
'X-Parsoid: ' . json_encode(
$parsoidInfo ),
// Force implicit cache refresh
similar to
@@ -159,32 +164,34 @@
// Now send off all those update requests
CurlMultiClient::request( $requests );
+ // @TODO: maybe call $this->setLastError() if something went
wrong?
- wfDebug('ParsoidCacheUpdateJob::invalidateTitles update: ' .
- serialize($requests) . "\n" );
+ wfDebug( 'ParsoidCacheUpdateJob::invalidateTitles update: ' .
+ serialize( $requests ) . "\n" );
/*
- # PURGE
- # Not needed with implicit updates (see above)
- # Build an array of purge requests
- $requests = array();
- foreach ( $wgParsoidCacheServers as $server ) {
- foreach ( $titleArray as $title ) {
- $url = $this->getParsoidURL( $title, $server,
false );
+ # PURGE
+ # Not needed with implicit updates (see above)
+ # Build an array of purge requests
+ $requests = array();
+ foreach ( $wgParsoidCacheServers as $server ) {
+ foreach ( $titleArray as $title ) {
+ $url = $this->getParsoidURL( $title, $server, false );
- $requests[] = array(
- 'url' => $url
- );
- }
- }
+ $requests[] = array(
+ 'url' => $url
+ );
+ }
+ }
- $options = CurlMultiClient::getDefaultOptions();
- $options[CURLOPT_CUSTOMREQUEST] = "PURGE";
- // Now send off all those purge requests
- CurlMultiClient::request( $requests, $options );
+ $options = CurlMultiClient::getDefaultOptions();
+ $options[CURLOPT_CUSTOMREQUEST] = "PURGE";
+ // Now send off all those purge requests
+ CurlMultiClient::request( $requests, $options );
- wfDebug('ParsoidCacheUpdateJob::invalidateTitles purge: ' .
- serialize($requests) . "\n" );
+ wfDebug('ParsoidCacheUpdateJob::invalidateTitles purge: ' .
+ serialize($requests) . "\n" );
*/
}
+
}
--
To view, visit https://gerrit.wikimedia.org/r/65869
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Iad4de0f0017b3f079971d76523d3a5a5487e9155
Gerrit-PatchSet: 3
Gerrit-Project: mediawiki/extensions/Parsoid
Gerrit-Branch: master
Gerrit-Owner: Aaron Schulz <[email protected]>
Gerrit-Reviewer: Aaron Schulz <[email protected]>
Gerrit-Reviewer: GWicke <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
