r69723 MediaWiki - Code Review archive

+		global $wgEnableScaryTranscluding;
+
+		if ( !$wgEnableScaryTranscluding ) {
+			return wfMsg('scarytranscludedisabled');
+		}
[...]
+			if ( strlen( $url1 ) > 255 ) {
+				return wfMsg( 'Interwiki-transclusion-url-too-long' );
+			}
[...]
+		return "<h2>$fullTitle</h2>< pre>$finalText</ pre> List of templates: < pre>".$listSubTemplates.'</ pre>' . $list2;

Please maintain a clean separation between backend and frontend. In practical terms, this means the logic for fetching things should live in Interwiki and the logic for presenting/using them should live in Parser. Interwiki.php should contain code that fetches a template and simply returns that template; if it fails, it should return some kind of error code, not an error message in the user's language, and if it succeeds it should return the text it fetched, not some HTML.

Furthermore, I don't think you need to bother with querying templatelinks to find dependent templates as the parser will find out about them soon enough when it parses them. It may make sense to preload them and cache them in a variable in the API case, so the request can be batched.

+		try {
+			$dbr = wfGetDb( DB_SLAVE, array(), $wikiID );
+		} catch (Exception $e) {
+			return wfMsg( 'Failed-to-connect-the-distant-DB' );
+		}

I don't think this is needed: I've seen plenty of places where DB connections are obtained and just used without error checking. Quick testing shows that MediaWiki dies with an error upon connection failure, so wfGetDb() will either give you a connection or kill the process.

+		$fields = array('old_text', 'page_id');
+		$res = $dbr->select(
+			array( 'page', 'revision', 'text' ),
+			$fields,
+			array( 'rev_id=page_latest',
+			       'page_namespace' => $namespace,
+			       'page_title'     => $DBkey,
+			       'page_id=rev_page',
+			       'rev_text_id=old_id'),
+			null,
+			array( 'LIMIT' => 1 )
+			);
[...]
+			if( $row ) {
+				$res = new Revision( $row );
+				$articleID = $res->mTextRow->page_id;
+				$text = $articleID."\n".$res->mTextRow->old_text;
+			}

Instead of rolling your own query and using ->mTextRow, you should use Revision::loadFromTitle( $db, $title ) (notice the $db parameter) and use ->getText() and ->getTitle()->getArticleID() to get the text and the page ID.

You also don't need to use resultObject() and fetchObject(), because you can simply do:

$res = $dbr->select( ... ); // Select multiple rows
foreach ( $res as $row ) { do stuff }

$row = $dbr->selectRow( ... ); // Select single row
$val = $dbr->selectField( ... ); // Select value of a single field of a single row

+				$listTemplates[] = array( 'namespace' => $row->tl_namespace, 'title' => $row->tl_title );

Generally, titles are represented as Title objects rather than array( 'namespace' => ns, 'title' => title ). You can obtain a Title object from a DB row with Title::makeTitle( namespace, title ) (or Title::newFromRow( $row ) if the row contains fields from the page table, but that's not the case in this example).

The caching for API requests is still URL-based, which I don't like (I said before that I'd like it to be interwiki+title-based instead) and currently doesn't have any code to store things in the cache.

> Instead of rolling your own query and using ->mTextRow, you should use Revision::loadFromTitle( $db, $title ) (notice the $db parameter) and use ->getText().

My problem is that ->getText will call ->getRawText and then ->loadText() which will retrieve the old_text from the *local* DB! I need to have the Revision instance store the $db, so that it can ->loadText( $db ) from the *distant* DB...

> The caching for API requests is still URL-based, which I don't like (I said before that I'd like it to be interwiki+title-based instead)

I will change this, but I need to change the structure of the 'transcache' table. Do you allow me to do that?

> and currently doesn't have any code to store things in the cache.

Can/should I use something like the following?

$textid = $wikiID.':'.$title->$title->getNsText().':'.$title->getText();
$key = wfMemcKey( 'iwtransclustiontext', 'textid', $textId );
$text = $wgMemc->get( $key );

I missed that, good point. You'll need Revision to remember which DB it came from, falling back to the local DB if the DB field wasn't stored. That should be a fairly minor tweak to the Revision class.

Rather than changing the structure of transcache, create a new table. That's much less of a hassle, and it's a pattern that has been used in the past (e.g. with restructuring the cur and old tables a long, long time ago).

Your memcached code looks good, but note that:

• wfMemcKey() takes a variable number arguments and joins them with colons, so you don't need to do that yourself. Just call wfMemcKey( 'iwtransclusiontext', 'tetxid', $wikiID, $title->getNsText(), $title->getText() ) or something along those lines
• Passing in the namespace number (getNamespace()) is nicer than passing in the namespace text
• Interwiki titles always have namespace 0 and the namespace prefix in the text (so for en:Talk:Foo, getText() returns 'Talk:Foo'), so passing in the namespace is pointless anyway

Also, by "store things in the cache" I really meant the transcache table or whatever is gonna be used in its stead, not memcached: what I was trying to say is that while your code reads API requests from transcache before making an HTTP request, it doesn't write the results back to transcache. Upon re-reading the code, I see that I was wrong: it does write back to cache.

Minor nitpick:

+			'tc_time' => $dbw->timestamp( time() ),

Don't use time() in MediaWiki. If you want the current time, just call $dbw->timestamp() or wfTimestamp(TS_FORMAT_NAME) (whichever is appropriate) without the time argument, and it'll default to the current time.

On second thought, per the discussion on wikitech-l, using memcached is probably a better idea than using some reworked version of the transcache table. Sorry for being so confusing.

I still have a problem in storing the DB field in Revision.php:

• if I store the $db of loadFromTitle( $db, $title ), I don't store enough information to create a connexion to MASTER_DB (needed in loadText() and getTimestampFromId()) because I don't have the wikiID.
• if I store the wikiID, I need to pass it at the beginning, so, I cannot use loadFromTitle( $db, $title ) anymore.

What I propose is to create a new function, loadFromTitleForeignWiki( $wikiID, $title ), store the wikiID in mWikiID and create the $db for loadFromTitle(). This way, I can create a slave or master connexion any time it's needed. What do you think?

About the caching, if I understand well, I forget about all the DB caching and just use memcached in the case of the API request, right?

Yup, that's all good.

I just found out it gets a little bit more painful than that, even. loadText() tries the revision cache in memcached keyed by textid (and implicitly by wiki ID because it uses wfMemcKey()), so you'd have to write a counterpart to wfMemcKey() that'll generate a key for a different wiki ID (wfForeignMemcKey()?) and use that. If you don't do this, it might come up with a totally different revision from the local wiki because they happen to have the same text ID.

This is getting into the territory where I wanna run this by Tim to make sure he doesn't totally hate what you're doing, but it all sounds like small tweaks for now.