Last modified: 2013-08-28 23:29:04 UTC
In order to streamline the review of recent changes on a wiki, Wikimedia Austria has funded a new extension called "Collaborative Watchlist" which can be found on Wikimedia SVN: * http://svn.wikimedia.org/svnroot/mediawiki/trunk/extensions/CollabWatchlist/ Description page: * http://www.mediawiki.org/wiki/Extension:CollaborativeWatchlist The software has been set up and used by several MediaWiki developers on their own wikis in meantime to be tested. The goal is that this extension is available on the Wikimedia cluster, for this it was designed and funded. Therefore I hereby ask for a code review in order to make it possible that this extension will be enabled on the clusters in the near future. Thanks! Manuel
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
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 ;)
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.
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
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?
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.
(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?
