r58370 MediaWiki - Code Review archive

What is this for?

It's an extension requested for the fundraising effort; it will retrieve a deisgnated page (for now just from wikimediafoundation.org) and display the text on the user's current wiki.

What's wrong with a link?

it's supposed to display the text, not be a link to it. the point is instead of moving people to the foundation wiki we keep them on their home wiki for a page longer.

Well, put me down as skeptical. But besides that, please allow me to make a few comments on the code.

Your functions have a bit of a problem with not doing what their name says they do, and with inelegant data flow. And some of your variables don't contain what their name says they contain.

constructURL() has a problem of odd data flow, since it doesn't just return the URL, it stores it into a member variable and expects the caller to somehow work out where to find it (despite the lack of documentation). It would be easier to understand if you used the usual lazy-initialisation convention:

function getPageURL() {
   if ( is_null( $this->mPageURL ) ) {
       ...
   }
   return $this->mPageURL;
}

And then instead of

private $mPageURL = '';

you would have

private $mPageURL;

which is a nice habit to get into since PHP considers strings like "0.0" to be false, breaking code like yours that converts strings to boolean if such strings happen to be input by the user.

cacheHeaders() doesn't do what it says it does. It actually sets the cache headers, it doesn't cache the headers itself. It should probably be called setCacheHeaders().

Similarly, getPageFromCache() should return a page, not true or false. This is a weakly typed language you're working in now, so it's not necessary to dedicate the return value to a success/failure value and nothing else. You can return the page if it works, or false if it doesn't. Then instead of:

$this->getPageFromCache();
if ( !$this->mPageText ) {
	$serializedText = Http::get( $this->mPageURL );

you will have (avoiding conversion from string to boolean again):

$pageText = $this->getPageFromCache();
if ( $pageText === false ) {
	$serializedText = Http::get( $this->mPageURL );

$absurl = '<a href="http://' . $this->mProject . '/';

This is not a URL, it is half a link. Maybe it could be called $linkStart.

savePageToCache() should accept the text as a parameter rather than getting it from a member variable.

In ExternalPagesErrors::showError():

 $msg = wfMsg( $errorText, $args );
 
 $wgOut->addWikiText(
 	'<div class="errorbox" style="float:none;">' .
 	$msg .
 	'</div>'

This is incorrect, because wfMsg() has already parsed the message once, and addWikiText() will parse it again. The double parsing will screw up various kinds of markup. Instead, you should use wfMsgNoTrans(), or OutputPage::wrapWikiMsg():

 array_unshift( $args, $errorText );
 $wgOut->wrapWikiMsg( '<div class="errorbox" style="float:none;">$1</div>', $args );

I was skeptical of the idea when I first heard of it several weeks back. As I understood it, it was basically scary transclusion as an extension.

Instead of using regexes to parse XML, request the page data in format=php and unserialize() it, or if you don't trust the remote server in format=json and use json_decode()

Oh well, that was already done in r58636.

Rewritten in r62165.