User:Catrope/Extension review/Translate/old

Base revision: r95391

Done Translate.php

• The efFoo() global functions should be in a static class, e.g. TranslateHooks

  Done rev:95755

• What does efTranslateCheckPT() do? The way it interacts with the revtag_type is not really documented or intuitive
  • 'pt' isn't exactly a good memcached key. It should be something unique and descriptive, e.g. wfMemcKey( 'translate', 'pageTranslationVersion' )

  Done Method was removed in rev:95754

translate.sql

• The index on (trs_page) is completely useless given that the primary key is (trs_page, trs_key)

  Can I just remove it?

  Yes. You probably want an index on (trs_page, trs_order) instead because of the query in MessageGroups.php lines 752-755

• What is this table for?

  Stores the extracted sections for translatable pages.

Done TranslateUtils.php

• Line 49: can't you just use list( $key, $code ) = explode( '/', $text, 2 ); or something along those lines?

  It doesn't necessarily return array with two elements

  Hmm, right.

• Line 132: you're mixing ASC and DESC in an ORDER BY, and one of the fields you're ordering by is a derivative field (result of a function). That in combination with the large size of the recentchanges table on most WMF wikis (5M rows on enwiki) means this feature will have to be behind $wgMiserMode or otherwise disableable

  Let's say target wikis are mediawiki.org, meta and commons. How does this hold up then? At translatewiki we have an RC table that is about 2.5M rows.

  Doesn't the namespace and timestamp conditions filter the resultset to small enough?

  Anyway, this code is only callable from command line scripts, so it should be safe.

  Hmm, OK.

• Line 136: what batch existence query?

  Done No longer used as of rev:95759. Preparing for deletion of this code.

• Line 139: you can also use $rows = iterator_to_array( $res );

  Done I didn't know it exists. rev:95758

• Line 170: you may want to use a strict comparison here, otherwise simpleSelector( 'foo', array( '0', '00', '1' ), '00' ) will produce strange results

  Done Fixed in rev:95760

• Line 202, 207: ZOMG, hardcoded English. Of all extensions... :)

  Done Broken code removed in rev:95761

• Line 256, 280: what is so performance-sensitive here that you have to factor out a simple function call? That makes little sense to me

  Done Simplified in rev:95760

Done TranslateTasks.php

• Line 361: $data is not escaped

  Done rev:95762.

• Line 462-468: can't you just use array_diff() here?

  Done rev:95765.

TranslateEditAddons.php

• Line 44, 49: you're using strtr here to replace spaces/underscores and str_replace elsewhere. Also, don't you have a function lying around that does lowercasing and space/underscore stuff?

  strtr/str replace do the same in this case, right? Function: no I don't think so.

• Line 53: -666 ?!?!?

  Any index which doesn't exist and which doesn't have existing neighbours.

  OK, very clever, but add a comment to explain what the weird choice of number is about.

• Generally, you should mention what hook each function is hooked into in a doc comment, that makes understanding the code easier without having to go back to look up the $wgHooks assignments each time
• Line 193: eww, onclick

  I don't know how to fix in the best way

  Done rev:95769.

  Rev doesn't address this at all. I can help you with the JS

• Line 98: why would $next equal true?

  Done No idea, removed. I don't think anything will break, but we will find out. rev:95769

• Line 111: what is SkinChihuahua?

  Done Clarified in rev:95752

• Line 122: $object seems to be an EditPage, document that

  Done rev:95769.

• Line 254-258: what does this logic do and why? There are no comments

  Done It tries to figure when it can replace the contents of the editing area with stuff from elsewhere. MediaWiki doesn't make it easy.

• Line 294: don't pass an array as the second parameter to selectField(). The function supports this by accident only

  Done rev:95769.

• Line 296: don't you really just mean to check that the second query didn't return zero results? If you meant it, say it, with something like $res !== false , (bool)$res , $dbr->numRows() > 0 or whatever. As currently written this function will return true in the (admittedly strange) case where 'fuzzy' isn't in the revtag_type table because false === false is true

  Done rev:95769.

MessageGroups.php

I added this stuff earlier but never saved it, may be out of date

• Line 760: the regex seems to match stuff like bar . That syntax is kinda strange, is that intended?

  Yes it is the horrible syntax I came up. Fortunately I don't think anybody uses it so I could change it to something better.

• Line 881-884: doesn't CACHE_ANYTHING do what you want?

  I was pretty sure that I had some reason. IRCC it was not implemented in a good way until recently. This also relates to the point that some data needs permanent storage (throttles etc) to work, while some data can be always reconstructed. I think it is safe to use CACHE_ANYTHING now.

• Line 928: this query filesorts. Looking up the ID of tp:mark in a separate query, then doing the other query (on page and revtag) but grouping by rt_page instead of page_id should work better

  Was fixed already with the new schema.

utils/TranslationHelpers.php

• Line 1224: ??? makeVariablesScript shouldn't be used this way. You should export variables through a MakeGlobalVariablesScript hook, and you shouldn't just export the return value of wfMsg(), use the normal message exporting system instead

  Do you mean resource loader? This code was written way before RL.

  D'oh, of course. Yes, I guess it just never got RL-ified then, that should be done.

  Done r96007 and follow-ups r96008 and r96009.