For starters, the code has quite a lot of raw SQL building, there seems to be some nested SQL statements in SpecialCollabWatchlist.php, which are a no-no while we still have MySQL 4 boxes in the cluster

while( $row = $res->fetchObject() ) {

is old coding style, use

foreach( $res as $row ) {

$dbr->freeResult( $res );

isn't needed

CollabWatchlist.js doesn't seem to be used anywhere, and hence doesn't load it using the ResourceLoader

The extension doesn't follow our Coding Style guidelines [1]

The code could benefit from having proper fleshed out function level comment blocks

[1] http://www.mediawiki.org/wiki/Manual:Coding_conventions

Oh it also seems to be using pre 1.17 LoadExtensionSchemaUpdates, which isn't a problem for the cluster, but would be useful for forwards compatability

$updater = null in the parameters, then just do if $updater === null, do old style, else do new style

Plenty of examples in SVN

Line 889 of CollabWatchlistEditor.php

wfRunHooks( 'UnwatchArticleComplete', array( &$user, &$article ) );

$user is undefined

f.hackenberger wrote:

Reedy:

Thanks for your comments. I have solved the following issues right away:

• $user is undefined
• while( ... ) is old coding style

Regarding the CollabWatchlist.js, how come you think it is unused? It's included in includes/CollabWatchlistChangesList.php and the function "onCollabWatchlistSelection" is set for onclick in includes/CollabWatchlistChangesList.php

Regarding the function level comment blocks, those are just missing in includes/SpecialCollabWatchlist.php. In all other php files there is documentation for all public (and even most private) methods. For SpecialCollabWatchlist.php the undocumented methods are just internal helper methods to structure the code. However, I have added that documentation to SpecialCollabWatchlist.php.

For the coding style, the major guidelines are all used. Tabs for indentation and alignment, Encoding, curly braces at the correct place etc. I could only spot several glitches regarding spaces between parenthesis. I'd assume those are not showstoppers :-).

Those only major problem remaining are the MySQL 4 incompatible SQL statements you have mentioned. Could you please elaborate on that. I need to know which type of query is affected and why and which version of MySQL we need to support. I have tried to find that information on wikimedia.org, but only found [1], which points to a custom version of MySQL.

[1] http://wikitech.wikimedia.org/view/MySQL

PS: I just noticed that you already corrected the DB fetchObject issues in SVN, thanks for that!

Thanks for the quick fixes.

For the coding style please refer to

• http://www.mediawiki.org/wiki/Manual:Coding_conventions

as Reedy has linked in his comment.

Here is an example how to use RessourceLoader for JS and CSS files used in extensions:

• http://www.mediawiki.org/wiki/ResourceLoader/Migration_guide_for_extension_developers

Additionally, please make sure you have set the correct auto-probs in your SVN client:

• http://www.mediawiki.org/wiki/Subversion/auto-props

to avoid issues like this:

• http://svn.wikimedia.org/viewvc/mediawiki?view=revision&revision=91121

The mysql4 issue is subqueries

ie SELECT * FROM bar where foo in ( SELECT * FROM foo )

Although most of the cluster has been upgraded, from memory we do still have some machines still running MySQL 4 still.

For the JS, I could find no reference to it being included (and hence loaded) anywhere in your files.

Just found it

		if ( isset( $tagElementIdBase ) ) {
			$jsPath = "$wgScriptPath/extensions/CollabWatchlist/js";
			$ret .= Xml::element( 'script',
				array(
					'type' => $wgJsMimeType,
					'src' => "$jsPath/CollabWatchlist.js",
				),
				'', false
			);
		}

As per above, you should be using the ResourceLoader to load it, and then get it added onto the pages when you want it. Minification, and such gets done for you, and makes it nicer to be loaded into MediaWiki

Oh, and null should be lowercase ;)

f.hackenberger wrote:

Ok, JS and the accidential NULLs are fixed. My autopros are now set for PHP as well and I'll have a look at the SQL issues.

I'm still missing information about the version of MySQL you are using.

f.hackenberger wrote:

I had a look at the subselect. That would be pretty hard to rewrite into a JOIN query. Could you please check if you really need to be compatible to MySQL 4.0? That version was released over 8 years ago. Subselect support is included in 4.1, which was released about 7 years ago...

(In reply to comment #7)

I had a look at the subselect. That would be pretty hard to rewrite into a JOIN
query. Could you please check if you really need to be compatible to MySQL 4.0?
That version was released over 8 years ago. Subselect support is included in
4.1, which was released about 7 years ago...

<Reedy> Does anyone know know many mysql4 boxes have we still got in production on the cluster? domas?
<domas> few!

You don't have to rewrite it, but we'd just have to be careful enabling the extension (if reviewed to be otherwise ok) on wikis with still 4.1 in the cluster. Those are more likely to go away when we've got EQIAD up, and have extra database capacity so we can finish the upgrades.

I thought we had some documentation about this, but seemingly not.

Quick checking around, it seems the dewiki (s5) cluster (at least) has a mysql4 master. It is more likely that all slaves are on mysql 5.1, as they can work with older mysql masters.

Same for s6 with frwiki, jawiki, ruwiki

reedy@fenari:~$ sql dewiki
Reading table information for completion of table and column names
You can turn off this feature to get a quicker startup with -A

Welcome to the MySQL monitor. Commands end with ; or \g.
Your MySQL connection id is 4008271479
Server version: 4.0.40-wikimedia-log

The raw sql (granted, a lot of it is done correctly) being fixed up, and any reinventing of the wheel with query building things

f.hackenberger wrote:

Reedy: Thanks for your continued support!

The raw sql (granted, a lot of it is done correctly) being fixed up, and any
reinventing of the wheel with query building things

Could you please point out specific things you would like to have changed? The query with the subselect in SpecialCollabWatchlist.php is AFAIK the only one which does not use the simplest form of the $dbr->select() API.

Which part do you consider to be re-inventing the wheel?

f.hackenberger wrote:

I just had another look at the sub-select. To me it seems that there is no way to rewrite that as a JOIN, because it's not a simple sub-select, but an 'EXISTS', 'NOT EXISTS' sub-select, which IMHO is impossible to rewrite into a JOIN. I'm open to suggestions for how to solve that differently in SQL, though.

Initial review notes at https://secure.wikimedia.org/wikipedia/mediawiki/wiki/User:Catrope/Extension_review/CollabWatchlist

(In reply to comment #10)

I just had another look at the sub-select. To me it seems that there is no way
to rewrite that as a JOIN, because it's not a simple sub-select, but an
'EXISTS', 'NOT EXISTS' sub-select, which IMHO is impossible to rewrite into a
JOIN. I'm open to suggestions for how to solve that differently in SQL, though.

NOT EXISTS can be rewritten into a LEFT JOIN with a WHERE something IS NULL. The EXISTS counterpart would be harder to do in this case because there are multiple tags involved.

f.hackenberger wrote:

(In reply to comment #11)

Thanks for your review, I'll address your comments (might be post 3rd Sept., I'm on holidays next week)! Where would you like me to add my comments/ACKs? On the wiki page, I suppose?

NOT EXISTS can be rewritten into a LEFT JOIN with a WHERE something IS NULL.
The EXISTS counterpart would be harder to do in this case because there are
multiple tags involved.

I'll wait for you feedback regarding the performance, before I rewrite that.

(In reply to comment #12)

(In reply to comment #11)

Thanks for your review, I'll address your comments (might be post 3rd Sept.,
I'm on holidays next week)! Where would you like me to add my comments/ACKs? On
the wiki page, I suppose?

Yeah, replying inline on the wiki page will be fine.

(In reply to comment #12)
Florian: Did you have time to address the comments?

Ping?

Hello, this is a quasi-automated-but-not-really message:

I am reviewing all tracking bugs for extensions to review and deploy to WMF servers. See the list here:
https://bugzilla.wikimedia.org/showdependencytree.cgi?id=31235&hide_resolved=1

The [[mw:Review queue]] page lists the steps necessary to complete the review. I have copied them below and done some initial filling out based on what I can easily gleen from this bug and any linked to sources that are obvious. If I miss something/state something false, please do correct me.

Also, if you haven't yet done so, please review the information on and linked to from:
https://www.mediawiki.org/wiki/Writing_an_extension_for_deployment

TODO/Check list

Extension page on mediawiki.org:
Bugzilla component: no?
Extension in Gerrit:
Design Review: no
Archeticecture/Performance Review: not done
Security Review: not done
Screencast (if applicable): no
Community support: maybe?